Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1964473002:
Potential fix for rtx/red issue where red is removed only from the remote description. (Closed)
|
Created:
4 years, 7 months ago by stefan-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, danilchap, yujie_mao (webrtc), zhuangzesen_agora.io, zhengzhonghou_agora.io, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPotential fix for rtx/red issue where red is removed only from the remote description.
BUG=5675
R=pbos@webrtc.org
Committed: https://crrev.com/2b1f651d1535f4c4029e29678d052ad7f1d91683
Cr-Commit-Position: refs/heads/master@{#12776}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Comments addressed, pt 1. #Patch Set 3 : Comments addressed, test added. #Patch Set 4 : Cleanup #Patch Set 5 : Tests added and passing, clean ups done. #Patch Set 6 : improved. #Patch Set 7 : Format #Patch Set 8 : Fix memory issue. #Patch Set 9 : Renamed disable method. #
Total comments: 2
Patch Set 10 : Fix mistake. #
Messages
Total messages: 36 (12 generated)
stefan@webrtc.org changed reviewers: + pbos@webrtc.org
Peter, can you take a look at this? I'm not entirely sure that this is how we should be fixing this, and I left a few comments in the code about things I have doubts about. This is an attempt to implement #3 in: https://bugs.chromium.org/p/webrtc/issues/detail?id=5675#c16 https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:2518: } Will be removed
Description was changed from ========== Potential fix for rtx/red issue where red is removed only from the remote description. BUG=5675 ========== to ========== Potential fix for rtx/red issue where red is removed only from the remote description. BUG=5675 ==========
pbos@webrtc.org changed reviewers: + deadbeef@webrtc.org
On 2016/05/09 13:32:44, stefan-webrtc (holmer) wrote: > Peter, can you take a look at this? I'm not entirely sure that this is how we > should be fixing this, and I left a few comments in the code about things I have > doubts about. > > This is an attempt to implement #3 in: > https://bugs.chromium.org/p/webrtc/issues/detail?id=5675#c16 > > https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... > webrtc/media/engine/webrtcvideoengine2.cc:2518: } > Will be removed deadbeef@: Can you help take a look as well?
I think this needs some unit tests, but it looks good. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1245: LOG(LS_WARNING) << "Remote peer has red " << this; May want to make this log message look nicer (and the one below). https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1249: MergeFecConfig(recv_codecs_[i].fec, &config->rtp.fec); If this is actually completely redundant, you might as well just remove it and see if any tests break. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1253: config->rtp.fec.ulpfec_payload_type = -1; Dumb question, but it possible to use ulpfec without red? https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1500: return true; Remember to remove this before submitting https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.h:439: void DisableFec(); May want to explain why this exists in a comment, and add a TODO to remove it (if applicable).
https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1166: LOG(LS_WARNING) << "AddRecvStream" INFO https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1245: LOG(LS_WARNING) << "Remote peer has red " << this; INFO https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1251: LOG(LS_WARNING) << "Remote peer doesn't have red " << this; INFO https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1498: static int i = 0; don't commit this ;p https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:2516: void WebRtcVideoChannel2::WebRtcVideoReceiveStream::DisableFec() { Can you re-enable FEC after this change by negotiation, or how does that work? https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:2517: stream_->DisableFec(); I think this method should instead recreate the stream without FEC enabled instead of adding APIs below here for something temporary.
Comments addressed, pt 1.
Comments addressed, test added.
Cleanup
Tests added and passing, clean ups done.
improved.
Format
Redid this a lot and added tests. PTAL https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1166: LOG(LS_WARNING) << "AddRecvStream" On 2016/05/09 19:57:01, pbos-webrtc wrote: > INFO Done. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1245: LOG(LS_WARNING) << "Remote peer has red " << this; On 2016/05/09 19:57:01, pbos-webrtc wrote: > INFO Done. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1249: MergeFecConfig(recv_codecs_[i].fec, &config->rtp.fec); On 2016/05/09 18:40:36, Taylor Brandstetter wrote: > If this is actually completely redundant, you might as well just remove it and > see if any tests break. Done. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1251: LOG(LS_WARNING) << "Remote peer doesn't have red " << this; On 2016/05/09 19:57:00, pbos-webrtc wrote: > INFO Done. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1253: config->rtp.fec.ulpfec_payload_type = -1; On 2016/05/09 18:40:36, Taylor Brandstetter wrote: > Dumb question, but it possible to use ulpfec without red? In theory, maybe, but we don't support it. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1498: static int i = 0; On 2016/05/09 19:57:00, pbos-webrtc wrote: > don't commit this ;p Acknowledged. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1500: return true; On 2016/05/09 18:40:36, Taylor Brandstetter wrote: > Remember to remove this before submitting Acknowledged. https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/1964473002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.h:439: void DisableFec(); On 2016/05/09 18:40:36, Taylor Brandstetter wrote: > May want to explain why this exists in a comment, and add a TODO to remove it > (if applicable). Done.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964473002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/14723)
Fix memory issue.
Renamed disable method.
lgtm (eew :)) https://codereview.webrtc.org/1964473002/diff/160001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1964473002/diff/160001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:847: if (red_was_disabled != red_disabled_by_remote_side_) { check inside streams instead (one red_disabled.. is enough)
Taylor, I will land this, but I'd be happy to follow up if you have any additional comments. https://codereview.webrtc.org/1964473002/diff/160001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1964473002/diff/160001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:847: if (red_was_disabled != red_disabled_by_remote_side_) { On 2016/05/16 16:48:26, pbos-webrtc wrote: > check inside streams instead (one red_disabled.. is enough) I need to track this to be able to pass it in when creating new streams.
The CQ bit was checked by stefan@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964473002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964473002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7518) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/9854) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11403)
Fix mistake.
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1964473002/#ps180001 (title: "Fix mistake.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964473002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964473002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Potential fix for rtx/red issue where red is removed only from the remote description. BUG=5675 ========== to ========== Potential fix for rtx/red issue where red is removed only from the remote description. BUG=5675 R=pbos@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/2b1f651d1535f4c4029e29678... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) manually as 2b1f651d1535f4c4029e29678d052ad7f1d91683 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Potential fix for rtx/red issue where red is removed only from the remote description. BUG=5675 R=pbos@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/2b1f651d1535f4c4029e29678... ========== to ========== Potential fix for rtx/red issue where red is removed only from the remote description. BUG=5675 R=pbos@webrtc.org Committed: https://crrev.com/2b1f651d1535f4c4029e29678d052ad7f1d91683 Cr-Commit-Position: refs/heads/master@{#12776} ==========
Message was sent while issue was closed.
Belated lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
