Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2101253002:
Unit test for media packet reordering in ForwardErrorCorrection. (Closed)
|
Created:
4 years, 5 months ago by brandtr Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@flexfec-pt1c_renames-and-fixes Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUnit test for media packet reordering in ForwardErrorCorrection.
This CL expands the test coverage by checking that the FEC can
handle reordered received media packets. Specifically, this checks
that |recovered_packets| is kept in sorted order.
BUG=webrtc:5654
Committed: https://crrev.com/d90fa0be29251e6f5db40613458926c40a5ba12c
Cr-Commit-Position: refs/heads/master@{#13693}
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Rebase on "1c". #
Total comments: 10
Patch Set 4 : Response to feedback. #
Total comments: 4
Patch Set 5 : Rebase on top of master. #Patch Set 6 : Loss masks are parameters to NetworkReceivedPackets. fec_ is not dynamically allocated. #
Total comments: 10
Patch Set 7 : Response to feedback. #
Total comments: 2
Patch Set 8 : git cl format. #Patch Set 9 : Rebase. #
Messages
Total messages: 25 (9 generated)
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
Description was changed from ========== FlexFEC pt. 1d: Unit test for media packet reordering in ForwardErrorCorrection. Several of the packet lists in ForwardErrorCorrection needs to be kept sorted. This CL expands the test coverage by checking that the parity erasure code can handle reordered received media packets, i.e. that |recovered_packet_list| is kept sorted. BUG=webrtc:5654 ========== to ========== Unit test for media packet reordering in ForwardErrorCorrection. This CL expands the test coverage by checking that the FEC can handle reordered received media packets. Specifically, this checks that |recovered_packets| is kept in sorted order. BUG=webrtc:5654 ==========
https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:286: const int kNumImportantPackets = 0; might want change to constexpr, but may leave as const for local consistency. https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:288: uint8_t kProtectionFactor = 20; const https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:300: EXPECT_EQ(1, static_cast<int>(fec_packet_list_.size())); EXPECT_EQ(1u, fec_packet_list_.size()); is cleaner. https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:310: auto it2 = received_packet_list_.begin(); it2++; do not keep two commands on one line https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:317: EXPECT_EQ(3, static_cast<int>(recovered_packet_list_.size())); EXPECT_EQ(3u, ...)
Thanks for quick feedback! https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:286: const int kNumImportantPackets = 0; On 2016/07/01 18:55:08, danilchap wrote: > might want change to constexpr, but may leave as const for local consistency. Done. https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:288: uint8_t kProtectionFactor = 20; On 2016/07/01 18:55:08, danilchap wrote: > const Done. https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:300: EXPECT_EQ(1, static_cast<int>(fec_packet_list_.size())); On 2016/07/01 18:55:08, danilchap wrote: > EXPECT_EQ(1u, fec_packet_list_.size()); is cleaner. Done. https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:310: auto it2 = received_packet_list_.begin(); it2++; On 2016/07/01 18:55:08, danilchap wrote: > do not keep two commands on one line Done. https://codereview.webrtc.org/2101253002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:317: EXPECT_EQ(3, static_cast<int>(recovered_packet_list_.size())); On 2016/07/01 18:55:08, danilchap wrote: > EXPECT_EQ(3u, ...) Done.
New test lgtm. More style cleanups better do in separate CL.
https://codereview.webrtc.org/2101253002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2101253002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:541: received_fec_packets_.sort(SortablePacket::LessThan()); Btw, what do you think of simply finding the right position manually and inserting the packet in order instead of calling sort after? https://codereview.webrtc.org/2101253002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:307: NetworkReceivedPackets(); I think it would have been nicer to pass media_loss_mask into this method instead.
Rebased on top of master, to reduce unnecessary dependencies between semi-unrelated CLs. Addressed comments. https://codereview.webrtc.org/2101253002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2101253002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:541: received_fec_packets_.sort(SortablePacket::LessThan()); On 2016/07/05 17:26:49, stefan-webrtc (holmer) wrote: > Btw, what do you think of simply finding the right position manually and > inserting the packet in order instead of calling sort after? Yes, that makes a lot of sense. If the packets arrive in order, we could then add them in constant time. When reordering happens, the insertion would be linear time instead. I'll consider this in a different CL. In a CL that I never sent out, I switched the backing data structure to a std::deque, which allowed for O(log n) binary search here. In an ad hoc benchmark, I saw ~15% performance improvement, but it turned out that the benchmark did not necessarily represent the real world usage of the FEC in a very good way. https://codereview.webrtc.org/2101253002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:307: NetworkReceivedPackets(); On 2016/07/05 17:26:49, stefan-webrtc (holmer) wrote: > I think it would have been nicer to pass media_loss_mask into this method > instead. Done.
https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:48: fec_(), initialization of members with default constructor might be cleaner to omit. Specially when there are other members with omitted default initialization. https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:876: void RtpFecTest::NetworkReceivedPackets(int* media_packet_list, Declaration name parameters ..._mask, but definition name parameters ..._packet_list. Ensure names (and meanings) match in declaration and definition. Either do not pass parameters or use them.
lgtm with nits fixed and once danil is happy. https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:551: // Note that |protected_pkt_list| is sorted by construction. Sorted by sequence number? https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:567: // by keeping it sorted we try to recover the oldest lost packets first. Perhaps mention how it's ordered too. https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:298: TEST_F(RtpFecTest, FecRecoveryWithMediaReordering) { WithMediaOutOfOrder to align with test below, or rename the other test.
Thanks for the feedback; comments have been addressed. Android and libfuzzer failures should be independent of this CL. https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:551: // Note that |protected_pkt_list| is sorted by construction. On 2016/07/20 09:33:30, stefan-webrtc (holmer) wrote: > Sorted by sequence number? Yep. https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:567: // by keeping it sorted we try to recover the oldest lost packets first. On 2016/07/20 09:33:30, stefan-webrtc (holmer) wrote: > Perhaps mention how it's ordered too. Done. https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:48: fec_(), On 2016/07/19 13:47:27, danilchap wrote: > initialization of members with default constructor might be cleaner to omit. > Specially when there are other members with omitted default initialization. Done. https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:298: TEST_F(RtpFecTest, FecRecoveryWithMediaReordering) { On 2016/07/20 09:33:30, stefan-webrtc (holmer) wrote: > WithMediaOutOfOrder to align with test below, or rename the other test. Renamed the test. The reason for the different names was that in the first test, two media packets were received out-of-order (i.e., there was a "Reordering"), whereas in the the second test only a single FEC packet was received out-of-order. No real need to make this distinction though, hence the changed name. https://codereview.webrtc.org/2101253002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:876: void RtpFecTest::NetworkReceivedPackets(int* media_packet_list, On 2016/07/19 13:47:27, danilchap wrote: > Declaration name parameters ..._mask, but definition name parameters > ..._packet_list. Ensure names (and meanings) match in declaration and > definition. > Either do not pass parameters or use them. This was sloppy by me, thanks for catching!
lgtm % nit https://codereview.webrtc.org/2101253002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:158: &recovered_packet_list_)); realign (or just call 'git cl format' since there are more realignment below)
https://codereview.webrtc.org/2101253002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2101253002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:158: &recovered_packet_list_)); On 2016/07/22 12:38:21, danilchap wrote: > realign (or just call 'git cl format' since there are more realignment below) Ran 'git cl format'.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2101253002/#ps140001 (title: "git cl format.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/1677) ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/1699) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/11717) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/11648) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/16958) mac_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/5110)
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2101253002/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Unit test for media packet reordering in ForwardErrorCorrection. This CL expands the test coverage by checking that the FEC can handle reordered received media packets. Specifically, this checks that |recovered_packets| is kept in sorted order. BUG=webrtc:5654 ========== to ========== Unit test for media packet reordering in ForwardErrorCorrection. This CL expands the test coverage by checking that the FEC can handle reordered received media packets. Specifically, this checks that |recovered_packets| is kept in sorted order. BUG=webrtc:5654 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Unit test for media packet reordering in ForwardErrorCorrection. This CL expands the test coverage by checking that the FEC can handle reordered received media packets. Specifically, this checks that |recovered_packets| is kept in sorted order. BUG=webrtc:5654 ========== to ========== Unit test for media packet reordering in ForwardErrorCorrection. This CL expands the test coverage by checking that the FEC can handle reordered received media packets. Specifically, this checks that |recovered_packets| is kept in sorted order. BUG=webrtc:5654 Committed: https://crrev.com/d90fa0be29251e6f5db40613458926c40a5ba12c Cr-Commit-Position: refs/heads/master@{#13693} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d90fa0be29251e6f5db40613458926c40a5ba12c Cr-Commit-Position: refs/heads/master@{#13693} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
