Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886813002:
Delete RtpData::OnRecoveredPacket, use RecoveredPacketReceiver instead. (Closed)
|
Created:
3 years, 7 months ago by nisse-webrtc Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhuangzesen_agora.io, Andrew MacDonald, zhengzhonghou_agora.io, henrika_webrtc, stefan-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc, the sun, AleBzk, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete RtpData::OnRecoveredPacket, use RecoveredPacketReceiver instead.
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2886813002
Cr-Commit-Position: refs/heads/master@{#18305}
Committed: https://chromium.googlesource.com/external/webrtc/+/30e8931ea7700e831cb2e4e373c5368604503e46
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comment update. #
Total comments: 4
Patch Set 3 : Address comments. #Patch Set 4 : Rebased. #
Total comments: 2
Patch Set 5 : Mark voe::Channel::OnRecoveredPacket as private. #
Total comments: 2
Patch Set 6 : Comment nit. #Messages
Total messages: 42 (21 generated)
nisse@webrtc.org changed reviewers: + danilchap@webrtc.org
PTAL. This separation makes sense to me, there seems to be no "source" which uses both of the OnRecoveredPacket and OnReceivedPayloadData. The OnRecoveredPacket methods involved in this cl seems to be used by ulpfec and rtx. It's not quite clear to me what to do about them, they need to parse the packet and identify extensions, violating the model that all extension identification should be done by the parser owned by RtpTransportController.
When rtx receiver would implement RtpPacketSinkInterface, i.e. accept parsed packet, then it wouldn't need to reparse the extension - it can just copy them over. Similar for ulpfec - by reusing one of the received packets it can keep all extension mappings. https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:200: // Callback interface for packets recovered by FlexFEC. The implementation since it is not used just by flexfec, update the comment https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right): https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:216: packet->length); if recovered packet is invalid now function will continue where before it was terminated. Can that be a problem? Do we have any tests for this scenario to be sure?
On 2017/05/16 18:06:43, danilchap wrote: > When rtx receiver would implement RtpPacketSinkInterface, i.e. accept parsed > packet, then it wouldn't need to reparse the extension - it can just copy them > over. Hmm. So it's the method RTPPayloadRegistry::RestoreOriginalPacket which needs to be replaced by something producing an RtpPacketReceived? https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... > Similar for ulpfec - by reusing one of the received packets it can keep all > extension mappings. I'd expect this case to be a bit trickier, but let's do RTX first.
On 2017/05/17 07:54:09, nisse-webrtc wrote: > On 2017/05/16 18:06:43, danilchap wrote: > > When rtx receiver would implement RtpPacketSinkInterface, i.e. accept parsed > > packet, then it wouldn't need to reparse the extension - it can just copy them > > over. > > Hmm. So it's the method RTPPayloadRegistry::RestoreOriginalPacket which needs to > be replaced by something producing an RtpPacketReceived? > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... Somewhen later, yes. But that probably not subject of this CL - this one only move a function from one interface to another, dedicated interface. > > > Similar for ulpfec - by reusing one of the received packets it can keep all > > extension mappings. > > I'd expect this case to be a bit trickier, but let's do RTX first. Fully agree: I also expect it a bit trickier than rtx and trickier than I currently imagine.
nisse@webrtc.org changed reviewers: + brandtr@webrtc.org
Rasmus, please take a look, in particular on the changes to the ulpfec code. https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right): https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:216: packet->length); On 2017/05/16 18:06:43, danilchap wrote: > if recovered packet is invalid now function will continue where before it was > terminated. > Can that be a problem? Do we have any tests for this scenario to be sure? I don't know. Rasmus, do you know this code?
https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right): https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:216: packet->length); On 2017/05/22 12:02:06, nisse-webrtc wrote: > On 2017/05/16 18:06:43, danilchap wrote: > > if recovered packet is invalid now function will continue where before it was > > terminated. > > Can that be a problem? Do we have any tests for this scenario to be sure? > > I don't know. Rasmus, do you know this code? The ForwardErrorCorrection class basically trusts the input it gets from the UlpfecReceiver through the ::DecodeFec call. If the media packet is not parseable, then it's probably a bad idea to put it into the FEC buffer, as any recovered packets relying on the corrupt media packet then also will be corrupt. After this change, I believe that could happen. There are some tests that inject garbage packets into the UlpfecReceiver, but it seems that they still pass. https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/mock... As an aside, the UlpfecReceiver is a bit weird. It receives all RED packets (both media and FEC) and immediately sends the media packets back through the callback. The FEC packets are then fed to the erasure code, and eventually the recovered packets are sent back through the callback too. I guess the point of this loop is to let the UlpfecReceiver strip all packets of their RED headers, so the RtpStreamReceiver doesn't have to do that. The FlexfecReceiver is simpler: it receives parsed packets and simply put them inside the ForwardErrorCorrection class. Only recovered packets are sent through the callback. https://codereview.webrtc.org/2886813002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2886813002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:200: // Callback interface for packets recovered by FlexFEC or Ulpfec. In nit: ULPFEC. https://codereview.webrtc.org/2886813002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2886813002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc:40: class MockRecoveredPacketReceiver : public RecoveredPacketReceiver { You can reuse this class: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/mock...
https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right): https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:216: packet->length); On 2017/05/22 12:54:36, brandtr wrote: > The ForwardErrorCorrection class basically trusts the input it gets from the > UlpfecReceiver through the ::DecodeFec call. If the media packet is not > parseable, then it's probably a bad idea to put it into the FEC buffer, as any > recovered packets relying on the corrupt media packet then also will be corrupt. > After this change, I believe that could happen. Not sure we need to care about that case, it seems a bit unlikely. And the converse could happen too: some of the media packets generated by the remote end are broken (according to our parser), but the fec packets are computed based on that broken media packet, and hence it is still relevant for reconstructing lost packets. And I hope the crypto we use below RTP includes some message authentication? https://codereview.webrtc.org/2886813002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2886813002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:200: // Callback interface for packets recovered by FlexFEC or Ulpfec. In On 2017/05/22 12:54:36, brandtr wrote: > nit: ULPFEC. Done. https://codereview.webrtc.org/2886813002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2886813002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc:40: class MockRecoveredPacketReceiver : public RecoveredPacketReceiver { On 2017/05/22 12:54:36, brandtr wrote: > You can reuse this class: > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/mock... Done.
https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc (right): https://codereview.webrtc.org/2886813002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc:216: packet->length); On 2017/05/22 13:08:05, nisse-webrtc wrote: > On 2017/05/22 12:54:36, brandtr wrote: > > The ForwardErrorCorrection class basically trusts the input it gets from the > > UlpfecReceiver through the ::DecodeFec call. If the media packet is not > > parseable, then it's probably a bad idea to put it into the FEC buffer, as any > > recovered packets relying on the corrupt media packet then also will be > corrupt. > > After this change, I believe that could happen. > > Not sure we need to care about that case, it seems a bit unlikely. > And the converse could happen too: some of the media packets generated by the > remote end are broken (according to our parser), but the fec packets are > computed based on that broken media packet, and hence it is still relevant for > reconstructing lost packets. > > And I hope the crypto we use below RTP includes some message authentication? It might be safer if CL would do one kind of thing, not two: This CL can only split interface into two parts making no functional changes. (and match it's description) It can be done by changing RecoveredPacketReceiver::OnRecoveredPacket to return bool. Simplifying interface by dropping return value can be moved into dedicated followup CL.
On 2017/05/22 13:34:32, danilchap wrote: > It can be done by changing RecoveredPacketReceiver::OnRecoveredPacket to return > bool. > > Simplifying interface by dropping return value can be moved into dedicated > followup CL. Note that RecoveredPacketReceiver is not a new interface, this cl just moves it to a different header file. I deleted the return value just the other week, in https://codereview.webrtc.org/2693123002. So I'd prefer to not have to add it back...
On 2017/05/22 13:47:49, nisse-webrtc wrote: > On 2017/05/22 13:34:32, danilchap wrote: > > > It can be done by changing RecoveredPacketReceiver::OnRecoveredPacket to > return > > bool. > > > > Simplifying interface by dropping return value can be moved into dedicated > > followup CL. > > Note that RecoveredPacketReceiver is not a new interface, this cl just moves it > to a different header file. I deleted the return value just the other week, in > https://codereview.webrtc.org/2693123002. So I'd prefer to not have to add it > back... Acknowledged. lgtm
lgtm
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: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24750)
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, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2886813002/#ps60001 (title: "Rebased.")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17474)
nisse@webrtc.org changed reviewers: + solenberg@webrtc.org, sprang@webrtc.org
PTAL. Erik: in particular on the video/ changes. Fredrik: Trivial change in voice_engine/.
https://codereview.webrtc.org/2886813002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2886813002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:320: bool OnRecoveredPacket(const uint8_t* packet, size_t packet_length); IIUC this can be made private?
https://codereview.webrtc.org/2886813002/diff/60001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2886813002/diff/60001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:320: bool OnRecoveredPacket(const uint8_t* packet, size_t packet_length); On 2017/05/29 14:00:31, the sun wrote: > IIUC this can be made private? You're right. Done.
lgtm for video/ https://codereview.webrtc.org/2886813002/diff/80001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.h (right): https://codereview.webrtc.org/2886813002/diff/80001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:106: // Implements RecoveredPacketReceiver nit: End comment with period.
https://codereview.webrtc.org/2886813002/diff/80001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.h (right): https://codereview.webrtc.org/2886813002/diff/80001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:106: // Implements RecoveredPacketReceiver On 2017/05/29 14:30:33, sprang_webrtc wrote: > nit: End comment with period. Done.
lgtm for voice_engine/
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 nisse@webrtc.org
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, danilchap@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2886813002/#ps100001 (title: "Comment nit.")
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": 100001, "attempt_start_ts": 1496068777455750,
"parent_rev": "69bf1b3f3a1832724ebe6c254f263543dd3b56a7", "commit_rev":
"30e8931ea7700e831cb2e4e373c5368604503e46"}
Message was sent while issue was closed.
Description was changed from ========== Delete RtpData::OnRecoveredPacket, use RecoveredPacketReceiver instead. BUG=webrtc:7135 ========== to ========== Delete RtpData::OnRecoveredPacket, use RecoveredPacketReceiver instead. BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/2886813002 Cr-Commit-Position: refs/heads/master@{#18305} Committed: https://chromium.googlesource.com/external/webrtc/+/30e8931ea7700e831cb2e4e37... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/30e8931ea7700e831cb2e4e37... |
