Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2282473002:
Add FlexfecPacketGenerator. (pt. 9) (Closed)
|
Created:
4 years, 3 months ago by brandtr Modified:
4 years, 2 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@generalize_ulpfec_packet_generator_into_augmented_packet_generator Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd FlexfecPacketGenerator.
Helper class for FlexFEC unit tests to come.
BUG=webrtc:5654
Committed: https://crrev.com/8d02ea73c0d10b2853cafd9d47e2324549aed1d6
Cr-Commit-Position: refs/heads/master@{#14487}
Patch Set 1 : Rebase. #
Total comments: 7
Patch Set 2 : Feedback response 1. #Patch Set 3 : Rebase. #Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 35 (16 generated)
Patchset #1 (id:1) has been deleted
Rebase.
Description was changed from
==========
Add FlexfecPacketGenerator to fec_test_helper.{h,cc}.
BUG=webrtc:5654
==========
to
==========
Add FlexfecPacketGenerator to fec_test_helper.{h,cc}.
BUG=webrtc:5654
==========
Rebase
Rebase.
Missing line.
Description was changed from
==========
Add FlexfecPacketGenerator to fec_test_helper.{h,cc}.
BUG=webrtc:5654
==========
to
==========
Add FlexfecPacketGenerator.
Helper class for FlexFEC unit tests to come.
BUG=webrtc:5654
==========
brandtr@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Rebase.
Patchset #1 (id:120001) has been deleted
In this followup CL, I'm adding a new class, to be used by the
Flexfec{Receiver,Sender} classes.
https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right): https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:156: flexfec_timestamp_ += 3000; constexpr int kMagicConstantWithSomeDescriptiveName = 3000; https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:161: memset(packet_with_rtp_header->data, 0, IP_PACKET_SIZE); Do you really need to zero this memory? https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right): https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:93: explicit FlexfecPacketGenerator(uint32_t media_ssrc, uint32_t flexfec_ssrc); Only use explicit if your ctor takes one argument.
Thanks for quick feedback; see addressed comments. https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right): https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:156: flexfec_timestamp_ += 3000; On 2016/09/21 11:15:27, philipel wrote: > constexpr int kMagicConstantWithSomeDescriptiveName = 3000; Done. https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:161: memset(packet_with_rtp_header->data, 0, IP_PACKET_SIZE); On 2016/09/21 11:15:27, philipel wrote: > Do you really need to zero this memory? No, this was overly cautious of me. Parts of the FEC code depends on the tail memory being zero (I will try to get rid of that assumption, later on), but the FlexfecReceiver that will be tested using this helper class should not depend on that. https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:180: memset(red_packet->data, 0, red_packet->length); This memset is also unnecessary, since the entire zeroed out memory is overwritten right below. https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right): https://codereview.webrtc.org/2282473002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:93: explicit FlexfecPacketGenerator(uint32_t media_ssrc, uint32_t flexfec_ssrc); On 2016/09/21 11:15:27, philipel wrote: > Only use explicit if your ctor takes one argument. Done.
Rebase.
lgtm
Rebase.
Waiting for quick look from holmer@, for this one too :)
lgtm
Rebase.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2282473002/#ps220001 (title: "Rebase.")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by brandtr@webrtc.org
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 ========== Add FlexfecPacketGenerator. Helper class for FlexFEC unit tests to come. BUG=webrtc:5654 ========== to ========== Add FlexfecPacketGenerator. Helper class for FlexFEC unit tests to come. BUG=webrtc:5654 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add FlexfecPacketGenerator. Helper class for FlexFEC unit tests to come. BUG=webrtc:5654 ========== to ========== Add FlexfecPacketGenerator. Helper class for FlexFEC unit tests to come. BUG=webrtc:5654 Committed: https://crrev.com/8d02ea73c0d10b2853cafd9d47e2324549aed1d6 Cr-Commit-Position: refs/heads/master@{#14487} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8d02ea73c0d10b2853cafd9d47e2324549aed1d6 Cr-Commit-Position: refs/heads/master@{#14487} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
