Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3008773002:
Use RtxReceiveStream. (Closed)
|
Created:
3 years, 3 months ago by nisse-webrtc Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse RtxReceiveStream.
This also has the beneficial side-effect that when a media stream
which is protected by FlexFEC receives an RTX retransmission, the
retransmitted media packet is passed into the FlexFEC machinery,
which should improve its ability to recover packets via FEC.
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/3008773002
Cr-Commit-Position: refs/heads/master@{#19649}
Committed: https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152961af27033475f7
Patch Set 1 #
Total comments: 17
Patch Set 2 : Set recovered flag on packets recovered via rtx. #Patch Set 3 : Address feedback on webrtcvideoengine_unittest.cc. #Patch Set 4 : Rebase on top of RtxReceiveStream bugfix. #Patch Set 5 : Improve TODO comments. #
Total comments: 2
Messages
Total messages: 47 (24 generated)
nisse@webrtc.org changed reviewers: + brandtr@webrtc.org, danilchap@webrtc.org
Cl moved here, from https://chromium-review.googlesource.com/c/external/webrtc/+/543475
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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/21877)
Prefer keep CL concentrated on single task. https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... File webrtc/call/rtx_receive_stream.cc (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:25: RTC_DCHECK_GT(associated_payload_types_.size(), 0); may be log a warning instead? while empty payload_types_ isn't very useful, it is not error - class still would work with, just discarding every packet. (likely in a different CL since it doesn't match the CL description) https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_stream.h File webrtc/call/rtx_receive_stream.h (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.h:20: // TODO(nisse): Consider renaming, RtxReceiveStream looks similar to I doubt these kind of TODOs belong to code and even less to CL with current description: "Use TODO comments for code that is temporary, a short-term solution, or good-enough but not perfect." but this TODO is about name, not code. It might be better to have here (non-TODO) comment that describe the class. https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.h:28: std::map<int, int> associated_payload_types, are these renames needed to use the RtxReceiveStream? https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... File webrtc/call/rtx_receive_stream_unittest.cc (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream_unittest.cc:90: RtxReceiveStream rtx_sink(&media_sink, PayloadTypeMapping(kOtherPayloadType), suggestion: {{kUnknownPayloadType, kMediaPayloadType}} instead of PayloadTypeMapping(kOtherPayloadType) https://codereview.webrtc.org/3008773002/diff/1/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/config.h#newcode52 webrtc/config.h:52: // TODO(nisse): Unclear to me if the next two values belong here? why this comment belong to the CL? why this description is a TODO? May be instead of the question leave description how flags are used/supposed to be used. https://codereview.webrtc.org/3008773002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine_unittest.cc (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine_unittest.cc:95: bool VerifyRtxReceiveAssociation( may be Has instead of Verify https://codereview.webrtc.org/3008773002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine_unittest.cc:105: for (auto& decoder : config.decoders) { may be const auto& to stress it is not modified https://codereview.webrtc.org/3008773002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine_unittest.cc:1340: << "RTX should be mapped also for the RED payload type"; may be remove 'also' (this expectation might fail independently from the previous one)
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/...
I intend to split out the changes to RtxStreamReceiver and its tests to a separate cl, to be landed first. https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... File webrtc/call/rtx_receive_stream.cc (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:25: RTC_DCHECK_GT(associated_payload_types_.size(), 0); On 2017/08/29 13:33:06, danilchap wrote: > may be log a warning instead? > while empty payload_types_ isn't very useful, it is not error - class still > would work with, just discarding every packet. > (likely in a different CL since it doesn't match the CL description) I can change to a warning. We also noticed a bug in this class, it doesn't set the recovered flag like it should. I'm considering landing that fix in a separate cl, I can fold the other changes to this class there too. https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_stream.h File webrtc/call/rtx_receive_stream.h (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.h:20: // TODO(nisse): Consider renaming, RtxReceiveStream looks similar to On 2017/08/29 13:33:06, danilchap wrote: > I doubt these kind of TODOs belong to code and even less to CL with current > description: > "Use TODO comments for code that is temporary, a short-term solution, or > good-enough but not perfect." > but this TODO is about name, not code. > > It might be better to have here (non-TODO) comment that describe the class. Rasmus wanted a TODO... https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.h:28: std::map<int, int> associated_payload_types, On 2017/08/29 13:33:06, danilchap wrote: > are these renames needed to use the RtxReceiveStream? Not strictly necessary, they're for consistency with the variable used to set this value. https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... File webrtc/call/rtx_receive_stream_unittest.cc (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream_unittest.cc:90: RtxReceiveStream rtx_sink(&media_sink, PayloadTypeMapping(kOtherPayloadType), On 2017/08/29 13:33:06, danilchap wrote: > suggestion: > {{kUnknownPayloadType, kMediaPayloadType}} > instead of > PayloadTypeMapping(kOtherPayloadType) If I change to a map initializer here, should I do that in PayloadTypeMapping too, for consistency? https://codereview.webrtc.org/3008773002/diff/1/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/config.h#newcode52 webrtc/config.h:52: // TODO(nisse): Unclear to me if the next two values belong here? On 2017/08/29 13:33:06, danilchap wrote: > why this comment belong to the CL? > why this description is a TODO? May be instead of the question leave description > how flags are used/supposed to be used. > I can try to rephrase or move. Comment is here because it makes the flags unused on one important code path. Current plan was outlined in a comment on gerrit: Leave as is here, but in VideoReceiveStreamConfig, replace structUlpfecConfig with just an int ulpfec_payload_type. I can write that as a TODO at that place instead. https://codereview.webrtc.org/3008773002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine_unittest.cc (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine_unittest.cc:95: bool VerifyRtxReceiveAssociation( On 2017/08/29 13:33:06, danilchap wrote: > may be Has instead of Verify Done. https://codereview.webrtc.org/3008773002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine_unittest.cc:105: for (auto& decoder : config.decoders) { On 2017/08/29 13:33:06, danilchap wrote: > may be const auto& to stress it is not modified Done. https://codereview.webrtc.org/3008773002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine_unittest.cc:1340: << "RTX should be mapped also for the RED payload type"; On 2017/08/29 13:33:06, danilchap wrote: > may be remove 'also' (this expectation might fail independently from the > previous one) Done, here and below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use RtxReceiveStream. BUG=webrtc:7135 ========== to ========== Use RtxReceiveStream. This also has the beneficial side-effect that when a media stream which is protected by FlexFEC receives an RTX retransmission, the retransmitted media packet is passed into the FlexFEC machinery, which should improve its ability to recover packets via FEC. BUG=webrtc:7135 ==========
nisse@webrtc.org changed reviewers: + magjed@webrtc.org, stefan@webrtc.org
Rebased on top of the bugfix in cl https://codereview.webrtc.org/3005793002/, updated description, and adding more OWNERs: stefan for call/ and video/, magjed for media/. https://codereview.webrtc.org/3008773002/diff/1/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/3008773002/diff/1/webrtc/config.h#newcode52 webrtc/config.h:52: // TODO(nisse): Unclear to me if the next two values belong here? On 2017/08/29 13:51:36, nisse-webrtc wrote: > On 2017/08/29 13:33:06, danilchap wrote: > > why this comment belong to the CL? > > why this description is a TODO? May be instead of the question leave > description > > how flags are used/supposed to be used. > > > > I can try to rephrase or move. Comment is here because it makes the flags unused > on one important code path. > > Current plan was outlined in a comment on gerrit: Leave as is here, but in > VideoReceiveStreamConfig, replace structUlpfecConfig with just an int > ulpfec_payload_type. I can write that as a TODO at that place instead. Done.
lgtm
lgtm
webrtc/media lgtm
lgtm https://codereview.webrtc.org/3008773002/diff/80001/webrtc/video/BUILD.gn File webrtc/video/BUILD.gn (right): https://codereview.webrtc.org/3008773002/diff/80001/webrtc/video/BUILD.gn#new... webrtc/video/BUILD.gn:65: "../call:rtp_receiver", It's a bit unclear to me what goes where. Why is RtxReceiveStream not in rtp_interfaces?
https://codereview.webrtc.org/3008773002/diff/80001/webrtc/video/BUILD.gn File webrtc/video/BUILD.gn (right): https://codereview.webrtc.org/3008773002/diff/80001/webrtc/video/BUILD.gn#new... webrtc/video/BUILD.gn:65: "../call:rtp_receiver", On 2017/09/01 08:38:17, stefan-webrtc wrote: > It's a bit unclear to me what goes where. Why is RtxReceiveStream not in > rtp_interfaces? The rtp_interfaces target includes interfaces only, including RtpPacketSinkInterface and RtpStreamReceiverControllerInterface. But here we need to instantiate RtxReceiveStream, which is a particular implementation of the RtpPacketSinkInterface. The rtp_receiver target includes the controller-related classes like the demuxer, and also RtxReceiveStream. It might make sense to split into two targets, but I don't think that's needed at the moment.
The CQ bit was checked by nisse@webrtc.org
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: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by nisse@webrtc.org
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_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by nisse@webrtc.org
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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/27366)
The CQ bit was checked by nisse@webrtc.org
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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/27372)
The CQ bit was checked by nisse@webrtc.org
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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/27457)
On 2017/09/04 07:54:06, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_rel on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/27457) The failures on Android trybots were fixed: http://crbug.com/761720 You can try the commit queue again.
The CQ bit was checked by nisse@webrtc.org
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": 80001, "attempt_start_ts": 1504516659794160,
"parent_rev": "ffbe1cd07ef1b20a59415a0d440e8684505a2902", "commit_rev":
"5c0f6c62ea3b1d2c43f8fc152961af27033475f7"}
Message was sent while issue was closed.
Description was changed from ========== Use RtxReceiveStream. This also has the beneficial side-effect that when a media stream which is protected by FlexFEC receives an RTX retransmission, the retransmitted media packet is passed into the FlexFEC machinery, which should improve its ability to recover packets via FEC. BUG=webrtc:7135 ========== to ========== Use RtxReceiveStream. This also has the beneficial side-effect that when a media stream which is protected by FlexFEC receives an RTX retransmission, the retransmitted media packet is passed into the FlexFEC machinery, which should improve its ability to recover packets via FEC. BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/3008773002 Cr-Commit-Position: refs/heads/master@{#19649} Committed: https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.webrtc.org/3010983002/ by nisse@webrtc.org. The reason for reverting is: A few perf tests broken, including RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx RampUpTest.UpDownUpTransportSequenceNumberRtx RampUpTest.UpDownUpTransportSequenceNumberPacketLoss . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
