GH-303 Add support for status codes in packet reader #570

dshil · 2023-09-28T09:19:33Z

#303

gavv

Thanks for PR! First set of comments.

src/internal_modules/roc_audio/depacketizer.cpp

src/internal_modules/roc_fec/reader.cpp

src/internal_modules/roc_node/sender_encoder.cpp

src/internal_modules/roc_packet/ireader.h

src/tests/roc_audio/test_packetizer.cpp

github-actions · 2023-09-28T18:38:08Z

☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

src/internal_modules/roc_rtp/validator.cpp

src/internal_modules/roc_packet/sorted_queue.cpp

src/internal_modules/roc_packet/queue.cpp

src/internal_modules/roc_packet/ireader.h

src/internal_modules/roc_packet/delayed_reader.cpp

src/internal_modules/roc_packet/concurrent_queue.cpp

dshil · 2023-10-02T11:45:56Z

Hi @gavv, thanks for your comments. I've updated the PR. When you will be ready to accept these changes, please ping me and I'll link the task.

gavv · 2023-10-03T13:10:23Z

Huge work.

This PR implements new behavior for which we need new tests.

We should add tests how some readers handle ErrNoData and other errors returned from underlying readers.

The following classes should be covered:

Group 1:

These classes forward any error as-is.
- rtp::Populator
- rtp::Validator
- rtp::TimestampInjector

Group 2:

These classes have special handling for ErrNoData and forward other errors.
- packet::DelayedReader
- fec::Reader

Group 3:

This class does not forward any error (for now), but should correctly work with any error.
- audio::Depacketizer

Could you please either add these tests (in this or separate PR), or create an issue describing what tests to add?

src/internal_modules/roc_audio/depacketizer.cpp

src/internal_modules/roc_node/sender_encoder.cpp

src/tests/public_api/test_helpers/proxy.h

src/tests/roc_fec/test_helpers/packet_dispatcher.h

src/tests/roc_packet/test_concurrent_queue.cpp

src/tests/roc_pipeline/test_helpers/packet_sender.h

src/tests/roc_rtp/test_timestamp_injector.cpp

src/tests/roc_pipeline/test_sender_sink.cpp

src/internal_modules/roc_audio/depacketizer.cpp

src/internal_modules/roc_rtp/validator.cpp

gavv

Final set of comments.

src/internal_modules/roc_fec/reader.cpp

gavv

Thanks for update!

src/internal_modules/roc_status/status_code_to_str.h

src/tests/roc_fec/test_writer_reader.cpp

gavv · 2023-10-06T17:26:11Z

src/tests/roc_fec/test_writer_reader.cpp

+        const status::StatusCode codes[] = {
+            status::StatusUnknown,
+        };
+
+        for (size_t n_code = 0; n_code < ROC_ARRAY_SIZE(codes); ++n_code) {


Do you think we need a loop over codes in these tests?

It seems that all codes except NoData are handled the same way, so it should be enough to test NoData + any single code. NoData is covered by other tests (because Queue returns it), and I guess these tests can check just one another code.

(same for 2 tests below)

src/tests/roc_fec/test_writer_reader.cpp

src/tests/roc_rtp/test_helpers/status_reader.h

src/tests/roc_packet/test_delayed_reader.cpp

src/tests/roc_audio/test_depacketizer.cpp

dshil added the ready for review Pull request can be reviewed label Sep 28, 2023

dshil requested a review from gavv September 28, 2023 09:19

dshil force-pushed the dshil/gh-303 branch 3 times, most recently from cc4e462 to 352d6b7 Compare September 28, 2023 11:36

gavv reviewed Sep 28, 2023

View reviewed changes

gavv added review in progress Pull request is being reviewed and removed ready for review Pull request can be reviewed labels Sep 28, 2023

github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Sep 28, 2023

dshil force-pushed the dshil/gh-303 branch from 352d6b7 to f8fa454 Compare September 29, 2023 12:37

gavv removed the needs rebase Pull request has conflicts and should be rebased label Sep 29, 2023

dshil force-pushed the dshil/gh-303 branch from f8fa454 to 89e5e19 Compare September 29, 2023 14:12

gavv reviewed Sep 29, 2023

View reviewed changes

gavv added needs revision Pull request should be revised by its author and removed review in progress Pull request is being reviewed labels Sep 29, 2023

dshil force-pushed the dshil/gh-303 branch 2 times, most recently from 3652317 to b7f8311 Compare October 2, 2023 11:43

dshil requested a review from gavv October 2, 2023 11:44

dshil added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Oct 2, 2023

dshil marked this pull request as draft October 2, 2023 12:22

dshil force-pushed the dshil/gh-303 branch 2 times, most recently from e25d6e2 to 85182cb Compare October 2, 2023 12:55

dshil changed the title ~~Add support for error codes in packet reader~~ GH-303 Add support for error codes in packet reader Oct 2, 2023

dshil changed the title ~~GH-303 Add support for error codes in packet reader~~ Add support for error codes in packet reader Oct 2, 2023

dshil marked this pull request as ready for review October 2, 2023 12:57

dshil changed the title ~~Add support for error codes in packet reader~~ GH-303 Add support for error codes in packet reader Oct 3, 2023

gavv reviewed Oct 3, 2023

View reviewed changes

gavv added review in progress Pull request is being reviewed and removed ready for review Pull request can be reviewed labels Oct 3, 2023

gavv reviewed Oct 3, 2023

View reviewed changes

src/internal_modules/roc_audio/depacketizer.cpp Outdated Show resolved Hide resolved

src/internal_modules/roc_rtp/validator.cpp Outdated Show resolved Hide resolved

src/internal_modules/roc_rtp/validator.cpp Outdated Show resolved Hide resolved

gavv reviewed Oct 3, 2023

View reviewed changes

gavv added needs revision Pull request should be revised by its author and removed review in progress Pull request is being reviewed labels Oct 3, 2023

dshil force-pushed the dshil/gh-303 branch 3 times, most recently from ca07ba0 to e485f7d Compare October 5, 2023 14:17

gavv mentioned this pull request Oct 5, 2023

Support error codes and partial reads and writes in audio readers and writers #302

Closed

dshil changed the title ~~GH-303 Add support for error codes in packet reader~~ GH-303 Add support for status codes in packet reader Oct 6, 2023

dshil force-pushed the dshil/gh-303 branch from e485f7d to c9b26be Compare October 6, 2023 12:01

dshil added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Oct 6, 2023

dshil requested a review from gavv October 6, 2023 12:03

gavv reviewed Oct 6, 2023

View reviewed changes

gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Oct 6, 2023

dshil force-pushed the dshil/gh-303 branch from 459f4cc to f66444b Compare October 9, 2023 11:12

dshil added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Oct 9, 2023

roc-streamingGH-303 Add support for status codes in packet reader

9813f47

dshil force-pushed the dshil/gh-303 branch from f66444b to 9813f47 Compare October 9, 2023 11:37

gavv merged commit 3360e18 into roc-streaming:develop Oct 9, 2023
31 checks passed

gavv removed the ready for review Pull request can be reviewed label Oct 9, 2023

dshil deleted the dshil/gh-303 branch October 9, 2023 12:08

gavv added this to the 0.3.0 milestone Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-303 Add support for status codes in packet reader #570

GH-303 Add support for status codes in packet reader #570

dshil commented Sep 28, 2023 •

edited by gavv

Loading

gavv left a comment

github-actions bot commented Sep 28, 2023

dshil commented Oct 2, 2023

gavv commented Oct 3, 2023 •

edited

Loading

gavv left a comment

gavv left a comment

gavv Oct 6, 2023

gavv Oct 6, 2023

GH-303 Add support for status codes in packet reader #570

GH-303 Add support for status codes in packet reader #570

Conversation

dshil commented Sep 28, 2023 • edited by gavv Loading

gavv left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2023

dshil commented Oct 2, 2023

gavv commented Oct 3, 2023 • edited Loading

gavv left a comment

Choose a reason for hiding this comment

gavv left a comment

Choose a reason for hiding this comment

gavv Oct 6, 2023

Choose a reason for hiding this comment

gavv Oct 6, 2023

Choose a reason for hiding this comment

dshil commented Sep 28, 2023 •

edited by gavv

Loading

gavv commented Oct 3, 2023 •

edited

Loading