Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 3006063002: Reland of 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, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Reland of Use RtxReceiveStream. (patchset #1 id:1 of https://codereview.webrtc.org/3010983002/ ) Reason for revert: Intend to fix perf failures and reland. Original issue's description: > Revert of Use RtxReceiveStream. (patchset #5 id:80001 of https://codereview.webrtc.org/3008773002/ ) > > Reason for revert: > A few perf tests broken, including > > RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx > RampUpTest.UpDownUpTransportSequenceNumberRtx > RampUpTest.UpDownUpTransportSequenceNumberPacketLoss > > > Original issue's description: > > 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/+/5c0f6c62ea3b1d2c43f8fc152961af27033475f7 > > TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=webrtc:7135 > > Review-Url: https://codereview.webrtc.org/3010983002 > Cr-Commit-Position: refs/heads/master@{#19653} > Committed: https://chromium.googlesource.com/external/webrtc/+/3c39c0137afa274d1d524b150b50304b38a2847b TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/3006063002 Cr-Commit-Position: refs/heads/master@{#19715} Committed: https://chromium.googlesource.com/external/webrtc/+/35713eaf565c0fef07c8afc158d7b8fdf7ec3d78

Patch Set 1 #

Patch Set 2 : Rebase. #

Patch Set 3 : Fix RTCP feedback for RTX packets. #

Patch Set 4 : Merge remote-tracking branch 'origin/master' into use-RtxStreamReceiver-reland #

Total comments: 4

Patch Set 5 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -68 lines) Patch
M webrtc/call/rampup_tests.cc View 1 1 chunk +3 lines, -2 lines 0 comments Download
M webrtc/call/rtx_receive_stream.h View 1 2 3 4 2 chunks +9 lines, -1 line 0 comments Download
M webrtc/call/rtx_receive_stream.cc View 1 2 3 4 2 chunks +14 lines, -4 lines 0 comments Download
M webrtc/call/video_receive_stream.h View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine.cc View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine_unittest.cc View 1 5 chunks +41 lines, -13 lines 0 comments Download
M webrtc/video/BUILD.gn View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/video/end_to_end_tests.cc View 1 2 3 1 chunk +3 lines, -1 line 0 comments Download
M webrtc/video/rtp_video_stream_receiver.h View 1 2 3 2 chunks +2 lines, -3 lines 0 comments Download
M webrtc/video/rtp_video_stream_receiver.cc View 1 2 3 5 chunks +5 lines, -38 lines 0 comments Download
M webrtc/video/rtp_video_stream_receiver_unittest.cc View 1 2 2 chunks +7 lines, -3 lines 0 comments Download
M webrtc/video/video_receive_stream.h View 1 2 3 chunks +6 lines, -0 lines 0 comments Download
M webrtc/video/video_receive_stream.cc View 1 2 3 6 chunks +10 lines, -3 lines 0 comments Download

Messages

Total messages: 31 (12 generated)
nisse-webrtc
Created Reland of Use RtxReceiveStream.
3 years, 3 months ago (2017-09-04 12:26:32 UTC) #1
nisse-webrtc
I've done some debugging, with help from Björn. It seems that we don't get any ...
3 years, 3 months ago (2017-09-05 14:44:15 UTC) #2
danilchap
On 2017/09/05 14:44:15, nisse-webrtc wrote: > I've done some debugging, with help from Björn. It ...
3 years, 3 months ago (2017-09-05 15:04:59 UTC) #3
brandtr
On 2017/09/05 15:04:59, danilchap wrote: > On 2017/09/05 14:44:15, nisse-webrtc wrote: > > I've done ...
3 years, 3 months ago (2017-09-05 15:37:03 UTC) #4
nisse-webrtc
On 2017/09/05 15:37:03, brandtr wrote: > On 2017/09/05 15:04:59, danilchap wrote: > > On 2017/09/05 ...
3 years, 3 months ago (2017-09-06 07:32:59 UTC) #5
danilchap
On 2017/09/06 07:32:59, nisse-webrtc wrote: > On 2017/09/05 15:37:03, brandtr wrote: > > On 2017/09/05 ...
3 years, 3 months ago (2017-09-06 07:41:20 UTC) #6
brandtr
On 2017/09/06 07:41:20, danilchap wrote: > On 2017/09/06 07:32:59, nisse-webrtc wrote: > > On 2017/09/05 ...
3 years, 3 months ago (2017-09-06 07:55:23 UTC) #7
nisse-webrtc
PTAL. webrtc_perf_tests --gtest_filter='RampUpTest.*' now work when I run it locally.
3 years, 3 months ago (2017-09-06 08:50:27 UTC) #8
danilchap
https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_stream.cc File webrtc/call/rtx_receive_stream.cc (right): https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_stream.cc#newcode42 webrtc/call/rtx_receive_stream.cc:42: const bool kNotRetransmitted = false; to avoid confusion about ...
3 years, 3 months ago (2017-09-06 10:29:52 UTC) #9
nisse-webrtc
https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_stream.cc File webrtc/call/rtx_receive_stream.cc (right): https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_stream.cc#newcode42 webrtc/call/rtx_receive_stream.cc:42: const bool kNotRetransmitted = false; On 2017/09/06 10:29:52, danilchap ...
3 years, 3 months ago (2017-09-06 11:25:10 UTC) #10
nisse-webrtc
On 2017/09/06 11:25:10, nisse-webrtc wrote: > https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_stream.cc > File webrtc/call/rtx_receive_stream.cc (right): > > https://codereview.webrtc.org/3006063002/diff/300001/webrtc/call/rtx_receive_stream.cc#newcode42 > ...
3 years, 3 months ago (2017-09-06 11:32:47 UTC) #11
danilchap
lgtm
3 years, 3 months ago (2017-09-06 11:51:45 UTC) #19
stefan-webrtc
On 2017/09/06 07:41:20, danilchap wrote: > On 2017/09/06 07:32:59, nisse-webrtc wrote: > > On 2017/09/05 ...
3 years, 3 months ago (2017-09-06 12:20:37 UTC) #22
stefan-webrtc
lgtm
3 years, 3 months ago (2017-09-06 12:23:53 UTC) #23
brandtr
On 2017/09/06 12:20:37, stefan-webrtc wrote: > On 2017/09/06 07:41:20, danilchap wrote: > > On 2017/09/06 ...
3 years, 3 months ago (2017-09-06 13:34:10 UTC) #24
brandtr
lgtm
3 years, 3 months ago (2017-09-06 13:35:50 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3006063002/320001
3 years, 3 months ago (2017-09-06 14:03:11 UTC) #27
commit-bot: I haz the power
Committed patchset #5 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/35713eaf565c0fef07c8afc158d7b8fdf7ec3d78
3 years, 3 months ago (2017-09-06 14:03:24 UTC) #30
nisse-webrtc
3 years, 3 months ago (2017-09-08 12:24:36 UTC) #31
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:320001) has been created in
https://codereview.webrtc.org/3007303002/ by nisse@webrtc.org.

The reason for reverting is: This change appears to break ulpfec, with severe
regressions, e.g., for webrtc_perf_test FullStackTest.ForemanCifPlr5Ulpfec.

Powered by Google App Engine
This is Rietveld 408576698