Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 3 months ago by nisse-webrtc Modified:
3 years, 3 months ago Reviewers:
brandtr, stefan-webrtc, Stefan CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChange ForwardErrorCorrection class to accept one received packet at a time.
BUG=None
Review-Url: https://codereview.webrtc.org/3012243002
Cr-Commit-Position: refs/heads/master@{#19893}
Committed: https://webrtc.googlesource.com/src/+/a5f043f9cd201fff209e96415bebcb8445ba0176
Patch Set 1 #Patch Set 2 : Fix compilation errors, including tests. #
Total comments: 14
Patch Set 3 : Drop return value. #Patch Set 4 : Clear received_packets_ list between sub-tests. #Patch Set 5 : Fixes and TODO comments for fec tests related to seqno wraparound. #Patch Set 6 : Rebase after The Great Move. #
Total comments: 9
Patch Set 7 : Comment fixes. #Patch Set 8 : Rebased. #
Created: 3 years, 3 months ago
Messages
Total messages: 27 (15 generated)
nisse@webrtc.org changed reviewers: + brandtr@webrtc.org
Would you like to have an intitial look? Tests not yet updated. Possible follow up cls: Improve seqno-related logic. And delete the ReceivedPacket class, instead passing in a is_fec flag or just have separate methods for inserting media and fec packets into the machine.
On 2017/09/13 12:37:14, nisse-webrtc wrote: > Would you like to have an intitial look? Tests not yet updated. > > Possible follow up cls: Improve seqno-related logic. And delete the > ReceivedPacket class, instead passing in a is_fec flag or just have separate > methods for inserting media and fec packets into the machine. I have just had a quick look, but it looks like a good start. Will look more carefully tomorrow. Agree that it would be nicer to just pass the media and FEC packets separately into the decoder. Since the FlexfecReceiver and UlpfecReceiver already do the demuxing, why put the packets in a single list again...?
I think this looks good. Hard to say why the tests fail though by just looking at the log output. Added a question about the refcounted packets. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:42: std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> AddReceivedPacket( Maybe rename, since it only converts the packet now? https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:123: bool FlexfecReceiver::ProcessReceivedPacket( make void? https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (left): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:354: received_packet->pkt = nullptr; Just to verify: the refcount of |received_packet->pkt| will be decremented when the unique_ptr owning |received_packet| goes out of scope? https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:401: received_packet->pkt = nullptr; And here. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: // separate arguments. +1 for this. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // UlpfecReceiverImpl::AddReceivedRedPacket may add two packets to Right, I forgot about that! I guess it can be replaced by two calls to DecodeFec? I don't think we ever send RED packets with two payloads, and if we want to support receiving 2 payloads, why not support receiving N payloads? We just probably just try to get rid of Ulpfec at some point though.
https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:42: std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> AddReceivedPacket( On 2017/09/14 11:49:45, brandtr wrote: > Maybe rename, since it only converts the packet now? Any naming suggestion? It seems the only member variables it accesses are ssrcs and packet counters. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc:123: bool FlexfecReceiver::ProcessReceivedPacket( On 2017/09/14 11:49:45, brandtr wrote: > make void? Done. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (left): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:354: received_packet->pkt = nullptr; On 2017/09/14 11:49:45, brandtr wrote: > Just to verify: the refcount of |received_packet->pkt| will be decremented when > the unique_ptr owning |received_packet| goes out of scope? That's my understanding. And it would be more natural to accept a scoped_refptr<Packet> as input. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: // separate arguments. On 2017/09/14 11:49:45, brandtr wrote: > +1 for this. For a followup, then, it's not quite trivial. Might make change to replace the AddReceivedPacket to a method that just returns the is_fec flag depending on config. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // UlpfecReceiverImpl::AddReceivedRedPacket may add two packets to On 2017/09/14 11:49:45, brandtr wrote: > Right, I forgot about that! I guess it can be replaced by two calls to > DecodeFec? Things may get easier if we merge the Add...Packet and Process...Fec methods both here and in flexfec. It seems callers always call them together. > I don't think we ever send RED packets with two payloads, and if we want to > support receiving 2 payloads, why not support receiving N payloads? No idea, I'd guess there's some interop case behind the choice to support 2? > We just probably just try to get rid of Ulpfec at some point though. But that's going to take a few years, I guess.
I do not understand the purpose of RtpFecTestUlpfecOnly.FecRecoveryWithSeqNumGapOneFrameNoRecovery. https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... Last time I made changes to ForwardErrorCorrection, I simply left this test even though I didn't understand the value of them. I could be some intricacy of how sequence number wraps are handled by the bitmask in the FEC header. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_receiver.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_receiver.h:42: std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> AddReceivedPacket( On 2017/09/14 12:55:51, nisse-webrtc wrote: > On 2017/09/14 11:49:45, brandtr wrote: > > Maybe rename, since it only converts the packet now? > > Any naming suggestion? It seems the only member variables it accesses are ssrcs > and packet counters. Maybe 'ConvertPacket', but then it would make sense to move the |packet_counter_| updates to ProcessReceivedPacket. This is more of a nit though, so feel free to skip. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: // separate arguments. On 2017/09/14 12:55:51, nisse-webrtc wrote: > On 2017/09/14 11:49:45, brandtr wrote: > > +1 for this. > > For a followup, then, it's not quite trivial. Might make change to replace the > AddReceivedPacket to a method that just returns the is_fec flag depending on > config. Yep, no need to do now. https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right): https://codereview.webrtc.org/3012243002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // UlpfecReceiverImpl::AddReceivedRedPacket may add two packets to On 2017/09/14 12:55:51, nisse-webrtc wrote: > On 2017/09/14 11:49:45, brandtr wrote: > > Right, I forgot about that! I guess it can be replaced by two calls to > > DecodeFec? > > Things may get easier if we merge the Add...Packet and Process...Fec methods > both here and in flexfec. It seems callers always call them together. In Flexfec, outside users call FlexfecReceiver::OnRtpPacket, which always call the two private methods together. https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/incl... In Ulpfec, outside users could call the different public methods separately, but they don't. https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_strea... > > I don't think we ever send RED packets with two payloads, and if we want to > > support receiving 2 payloads, why not support receiving N payloads? > > No idea, I'd guess there's some interop case behind the choice to support 2? Yes, probably. But why not support the N payload case then? > > We just probably just try to get rid of Ulpfec at some point though. > > But that's going to take a few years, I guess. Yes :(
Updated the ulpfec test to expect success. https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... File modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/rtp_fec_unittest.cc:454: // the log message "The recovered packet had a length larger than a typical IP I've just checked on master, and I get the same log message there. This needs fixing, but perhaps not in this cl?
lgtm https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... File modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/rtp_fec_unittest.cc:454: // the log message "The recovered packet had a length larger than a typical IP On 2017/09/15 06:34:57, nisse-webrtc wrote: > I've just checked on master, and I get the same log message there. This needs > fixing, but perhaps not in this cl? Sgtm to do this later.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
nisse@webrtc.org changed reviewers: + holmer@chromium.org
Stefan, would you like to have a look too? This refactoring should make it easier to fix handling of seqno wraparound, which currently looks broken to me. Unlikely to land today, but I hope to get it in reasonably soon, after the dust has settled after the great move out of webrtc/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21682)
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
lgtm https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... File modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/forward_error_correction.h:80: // TODO(holmer): Refactor into a proper class. May want to just delete this TODO, it's been there for 6 years. https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... File modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/rtp_fec_unittest.cc:453: // TODO(nisse): In this test, recovery based on the first FEC packets fails with FEC packet https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... File modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right): https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // TODO(nisse): But it looks like Please update the existing TODO instead if it's no longer correct. https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // TODO(nisse): But it looks like Please update the existing TODO instead if it's no longer correct.
I'll run the fyi and internal bot, and land if things look green. https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... File modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/forward_error_correction.h:80: // TODO(holmer): Refactor into a proper class. On 2017/09/15 13:36:13, stefan-webrtc wrote: > May want to just delete this TODO, it's been there for 6 years. Done. https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... File modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/rtp_fec_unittest.cc:453: // TODO(nisse): In this test, recovery based on the first FEC packets fails with On 2017/09/15 13:36:13, stefan-webrtc wrote: > FEC packet Done. https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... File modules/rtp_rtcp/source/ulpfec_receiver_impl.h (right): https://codereview.webrtc.org/3012243002/diff/100001/modules/rtp_rtcp/source/... modules/rtp_rtcp/source/ulpfec_receiver_impl.h:48: // TODO(nisse): But it looks like On 2017/09/15 13:36:13, stefan-webrtc wrote: > Please update the existing TODO instead if it's no longer correct. Done.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/3012243002/#ps140001 (title: "Rebased.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1505746567439560,
"parent_rev": "dd3abbb53250072dc5baac0391ebea135be6fa53", "commit_rev":
"a5f043f9cd201fff209e96415bebcb8445ba0176"}
Message was sent while issue was closed.
Description was changed from ========== Change ForwardErrorCorrection class to accept one received packet at a time. BUG=None ========== to ========== Change ForwardErrorCorrection class to accept one received packet at a time. BUG=None Review-Url: https://codereview.webrtc.org/3012243002 Cr-Commit-Position: refs/heads/master@{#19893} Committed: https://webrtc.googlesource.com/src/+/a5f043f9cd201fff209e96415bebcb8445ba0176 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://webrtc.googlesource.com/src/+/a5f043f9cd201fff209e96415bebcb8445ba0176 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
