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

Issue 1231613002: Fixing scenario where track is rejected and later un-rejected. (Closed)

Created:
5 years, 5 months ago by Taylor Brandstetter
Modified:
5 years, 5 months ago
Reviewers:
juberti1, pthatcher1
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Fixing scenario where track is rejected and later un-rejected. Added `RestartLocalTracks` and `RestartRemoteTracks` methods to `MediaStreamHandlerContainer` which will redo the track handlers' initial setup; most importantly, this will re-connect the renderer/capturer/etc. to a channel which was destroyed and then re-created. Also added `AcceptRemoteTracks` method to MediaStreamSignaling, which does the inverse of `RejectRemoteTracks`. Effectively this will notify sinks that the track is live again, after previously being set to `kEnded` when it was rejected. BUG=webrtc:2136 Committed: https://crrev.com/be37888b6d5d269dbd5385569dba15c0d70594f2 Cr-Commit-Position: refs/heads/master@{#9600}

Patch Set 1 #

Patch Set 2 : Removing debug log messages #

Total comments: 6

Patch Set 3 : Combining "Accept" and "Reject" methods to avoid duplicate code #

Patch Set 4 : Changing method naming as suggested #

Total comments: 2

Patch Set 5 : Removing obsolete method #

Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -19 lines) Patch
M talk/app/webrtc/mediastreamhandler.h View 1 2 3 10 chunks +17 lines, -1 line 0 comments Download
M talk/app/webrtc/mediastreamhandler.cc View 1 2 3 10 chunks +51 lines, -9 lines 0 comments Download
M talk/app/webrtc/mediastreamsignaling.h View 1 2 3 4 1 chunk +3 lines, -2 lines 0 comments Download
M talk/app/webrtc/mediastreamsignaling.cc View 1 2 6 chunks +23 lines, -7 lines 0 comments Download
M talk/app/webrtc/peerconnection.cc View 1 2 3 2 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (4 generated)
Taylor Brandstetter
The solution we agreed on ended up working, like I expected. But I noticed that ...
5 years, 5 months ago (2015-07-08 20:34:22 UTC) #2
pthatcher1
Looks really good. Just a few improvements to make. https://codereview.webrtc.org/1231613002/diff/20001/talk/app/webrtc/mediastreamhandler.h File talk/app/webrtc/mediastreamhandler.h (right): https://codereview.webrtc.org/1231613002/diff/20001/talk/app/webrtc/mediastreamhandler.h#newcode54 talk/app/webrtc/mediastreamhandler.h:54: ...
5 years, 5 months ago (2015-07-08 20:45:32 UTC) #3
Taylor Brandstetter
https://codereview.webrtc.org/1231613002/diff/20001/talk/app/webrtc/mediastreamhandler.h File talk/app/webrtc/mediastreamhandler.h (right): https://codereview.webrtc.org/1231613002/diff/20001/talk/app/webrtc/mediastreamhandler.h#newcode54 talk/app/webrtc/mediastreamhandler.h:54: // Associate |track_| with |_ssrc|. Can be called multiple ...
5 years, 5 months ago (2015-07-08 23:04:03 UTC) #4
pthatcher1
lgtm https://codereview.webrtc.org/1231613002/diff/60001/talk/app/webrtc/mediastreamsignaling.h File talk/app/webrtc/mediastreamsignaling.h (right): https://codereview.webrtc.org/1231613002/diff/60001/talk/app/webrtc/mediastreamsignaling.h#newcode333 talk/app/webrtc/mediastreamsignaling.h:333: void AcceptRemoteTracks(cricket::MediaType media_type); Is this still used?
5 years, 5 months ago (2015-07-16 22:56:03 UTC) #5
Taylor Brandstetter
https://codereview.webrtc.org/1231613002/diff/60001/talk/app/webrtc/mediastreamsignaling.h File talk/app/webrtc/mediastreamsignaling.h (right): https://codereview.webrtc.org/1231613002/diff/60001/talk/app/webrtc/mediastreamsignaling.h#newcode333 talk/app/webrtc/mediastreamsignaling.h:333: void AcceptRemoteTracks(cricket::MediaType media_type); On 2015/07/16 22:56:02, pthatcher1 wrote: > ...
5 years, 5 months ago (2015-07-17 16:28:26 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1231613002/80001
5 years, 5 months ago (2015-07-17 16:29:05 UTC) #9
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 5 months ago (2015-07-17 17:30:49 UTC) #10
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/be37888b6d5d269dbd5385569dba15c0d70594f2 Cr-Commit-Position: refs/heads/master@{#9600}
5 years, 5 months ago (2015-07-17 17:31:03 UTC) #11
juberti1
Nice. Is there a unit test for the bug 2136 behavior?
5 years, 5 months ago (2015-07-17 17:35:33 UTC) #13
Taylor Brandstetter
On 2015/07/17 17:35:33, juberti1 wrote: > Nice. Is there a unit test for the bug ...
5 years, 5 months ago (2015-07-17 17:44:09 UTC) #14
juberti1
On 2015/07/17 17:44:09, Taylor Brandstetter wrote: > On 2015/07/17 17:35:33, juberti1 wrote: > > Nice. ...
5 years, 5 months ago (2015-07-17 17:49:20 UTC) #15
magjed_webrtc
5 years, 5 months ago (2015-07-23 13:02:18 UTC) #16
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/1247443005/ by magjed@webrtc.org.

The reason for reverting is: I think this causes
WebRtcBrowserTest.CallAndModifyStream to fail on Android. See
https://code.google.com/p/webrtc/issues/detail?id=4857 for more info..

Powered by Google App Engine
This is Rietveld 408576698