gh-739: StatutCode Usage in IFrameEncoder/Decoder (begin/end)_frame #798
Conversation
github-actions
bot
added
the
contribution
sayedMurtadha
changed the title
sayedMurtadha
changed the title
| if (decoded_samples < requested_samples) { | ||
| payload_decoder_.end_frame(); | ||
| const status::StatusCode status_code = payload_decoder_.end_frame(); | ||
| packet_ = NULL; | ||
| // TODO: what should be the flow in case of error? | ||
| roc_panic_if_not(status_code == status::StatusOK); |
There was a problem hiding this comment.
We should forward error to caller and return it from Depacketizer::read().
We can update Depacketizer::read_samples_(), Depacketizer::read_decoded_samples_(), and Depacketizer::read_missing_samples_() to return status codes instead of pointer to new end. We can replace sample_t* buff_end with sample_t** buff_end so that they can return new end via argument.
In places where Depacketizer::read_samples_() returns NULL, it can now return StatusDrain.
There was a problem hiding this comment.
Shouldn't it be buff_ptr that is being updated instead of buff_end?
| virtual void begin_frame(packet::stream_timestamp_t frame_position, | ||
| const void* frame_data, | ||
| size_t frame_size) = 0; | ||
| virtual status::StatusCode begin_frame(packet::stream_timestamp_t frame_position, | ||
| const void* frame_data, | ||
| size_t frame_size) = 0; |
There was a problem hiding this comment.
Please add ROC_NODISCARD to begin_frame/end_frame in IFrameEncoder, IFrameDecoder, PcmEncoder, PcmDecoder.
(Please rebase on fresh develop, ROC_ATTR_NODISCARD was recently renamed to ROC_NODISCARD).
| status::StatusCode PcmDecoder::begin_frame(packet::stream_timestamp_t frame_position, | ||
| const void* frame_data, | ||
| size_t frame_size) { | ||
| if (!frame_data) { | ||
| return status::StatusBadArg; | ||
| } | ||
|
|
||
| if (frame_data_) { | ||
| roc_panic("pcm decoder: unpaired begin/end"); | ||
| roc_log(LogError, "pcm decoder: unpaired begin/end"); | ||
| return status::StatusBadState; | ||
| } |
There was a problem hiding this comment.
It's better to keep these checks panicking.
Null argument and unpaired calls are considered bugs in our own code. They are not caused by external input. Hence we don't want to recover from such errors, instead we should fix the bogus usage.
Same for PcmEncoder.
| encoder.begin_frame(pp->rtp()->payload.data(), pp->rtp()->payload.size()); | ||
| LONGS_EQUAL( | ||
| status::StatusOK, | ||
| encoder.begin_frame(pp->rtp()->payload.data(), pp->rtp()->payload.size())); |
There was a problem hiding this comment.
Please add unit tests for packetizer and depacketizer, as described in the issue:
Add unit test for Packetizer to check that it forwards error from encoder to upper level.
Add unit test for Depacketizer to check that it forwards error from decoder to upper level.
The new tests would be similar to TEST(packetizer, forward_error) and TEST(depacketizer, forward_error), but instead of mock packet reader/writer (StatusReader and StatusWriter), I think you'll need to create and use mock encoder and decoder.
gavv
added
contribution
sayedMurtadha
force-pushed
the
gh-739-iframe-encoder-decoder-begin-end-fn-return-type
branch
from
d5e3582 to
5a6fe74
Compare
github-actions
bot
added
ready for review
Closes #739