Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2969093002:
Make rendering of MediaStreams reflect changes to its set of tracks. (Closed)
|
Created:
3 years, 5 months ago by Guido Urdaneta Modified:
3 years, 4 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, jam, jochen+watch_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo, posciak+watch_chromium.org, nessy, Srirama Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake rendering of MediaStreams reflect changes to its set of tracks.
Before this CL, MediaStreams assigned to a media element required
reassignment of the stream to the element in order to make changes
to the set of tracks visible.
This CL fixes this problem by making WebMediaPlayerMS subscribe to
changes in the set of tracks of a MediaStream, and correspondingly
update audio and video renderers.
BUG=720258
Review-Url: https://codereview.chromium.org/2969093002
Cr-Commit-Position: refs/heads/master@{#490906}
Committed: https://chromium.googlesource.com/chromium/src/+/c9612ba8bf66e48a32301e35425dd3384f48a3d8
Patch Set 1 #
Total comments: 12
Patch Set 2 : rebase and address comments by foolip@ #
Total comments: 10
Patch Set 3 : address some of emircan@ comments #Patch Set 4 : rebase #Patch Set 5 : Fix unit tests #
Total comments: 3
Patch Set 6 : rebase #
Messages
Total messages: 55 (31 generated)
Description was changed from ========== Make rendering of MediaStreams reflect changes to its set of tracks. Before this CL, MediaStreams assigned to a media element required reassignment of the stream to the element in order to make changes to the set of tracks visible. This CL fixes this problem by making WebMediaPlayerMS subscribe to changes in the set of tracks of a MediaStream, and correspondingly updating audio and video renderers when necessary. BUG=720258 ========== to ========== Make rendering of MediaStreams reflect changes to its set of tracks. Before this CL, MediaStreams assigned to a media element required reassignment of the stream to the element in order to make changes to the set of tracks visible. This CL fixes this problem by making WebMediaPlayerMS subscribe to changes in the set of tracks of a MediaStream, and correspondingly updating audio and video renderers. BUG=720258 ==========
guidou@chromium.org changed reviewers: + emircan@chromium.org, foolip@chromium.org
guidou@chromium.org changed reviewers: + mkwst@chromium.org
The CQ bit was checked by guidou@chromium.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.chromium.or...
Description was changed from ========== Make rendering of MediaStreams reflect changes to its set of tracks. Before this CL, MediaStreams assigned to a media element required reassignment of the stream to the element in order to make changes to the set of tracks visible. This CL fixes this problem by making WebMediaPlayerMS subscribe to changes in the set of tracks of a MediaStream, and correspondingly updating audio and video renderers. BUG=720258 ========== to ========== Make rendering of MediaStreams reflect changes to its set of tracks. Before this CL, MediaStreams assigned to a media element required reassignment of the stream to the element in order to make changes to the set of tracks visible. This CL fixes this problem by making WebMediaPlayerMS subscribe to changes in the set of tracks of a MediaStream, and correspondingly update audio and video renderers. BUG=720258 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
//content/shell LGTM.
emircan@: Please review content/renderer/media foolip@: Please review third_party/WebKit, content/test/data/media and content/browser/webrtc
https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:198: native_stream->RemoveObserver(this); Can a DCHECK be added here or in the management of web_stream_ that makes sure that if we reach this line, we have also called AddObserver for the same object? https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:293: } Blank lines between functions seems to be the norm in this file. https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:324: web_stream_.AudioTracks(audio_tracks); Can you add a comment explaining that this is a getter? https://codereview.chromium.org/2969093002/diff/1/content/test/data/media/get... File content/test/data/media/getusermedia.html (right): https://codereview.chromium.org/2969093002/diff/1/content/test/data/media/get... content/test/data/media/getusermedia.html:618: original_stream = stream; original_stream===stream at every point in time after this, so I think just collapsing into stream would make it more clear what's going on. https://codereview.chromium.org/2969093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2969093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:783: if (src_object_ == src_object) https://html.spec.whatwg.org/multipage/media.html#dom-media-srcobject says "On setting, it must set the element's assigned media provider object to the new value, and then invoke the element's media element load algorithm." with no short-circuiting for when the value is the same. Is this a necessary part of the fix?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:198: native_stream->RemoveObserver(this); On 2017/07/05 08:49:44, foolip wrote: > Can a DCHECK be added here or in the management of web_stream_ that makes sure > that if we reach this line, we have also called AddObserver for the same object? RemoveObserver DCHECKs that its observer argument has been previously added with AddObserver. See https://cs.chromium.org/chromium/src/content/renderer/media/media_stream.cc?t... I guess that's enough. https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:293: } On 2017/07/05 08:49:44, foolip wrote: > Blank lines between functions seems to be the norm in this file. Done. https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:324: web_stream_.AudioTracks(audio_tracks); On 2017/07/05 08:49:44, foolip wrote: > Can you add a comment explaining that this is a getter? Done. Also for VideoTracks above. https://codereview.chromium.org/2969093002/diff/1/content/test/data/media/get... File content/test/data/media/getusermedia.html (right): https://codereview.chromium.org/2969093002/diff/1/content/test/data/media/get... content/test/data/media/getusermedia.html:618: original_stream = stream; On 2017/07/05 08:49:44, foolip wrote: > original_stream===stream at every point in time after this, so I think just > collapsing into stream would make it more clear what's going on. Done. Also for |full_stream| in the other test. https://codereview.chromium.org/2969093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2969093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:783: if (src_object_ == src_object) On 2017/07/05 08:49:44, foolip wrote: > https://html.spec.whatwg.org/multipage/media.html#dom-media-srcobject says "On > setting, it must set the element's assigned media provider object to the new > value, and then invoke the element's media element load algorithm." with no > short-circuiting for when the value is the same. Is this a necessary part of the > fix? Reverted the change and added a test to make sure the load algorithm runs when reassigning the same object.
lgtm https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/1/content/renderer/media/webm... content/renderer/media/webmediaplayer_ms.cc:198: native_stream->RemoveObserver(this); On 2017/07/05 13:05:02, Guido Urdaneta wrote: > On 2017/07/05 08:49:44, foolip wrote: > > Can a DCHECK be added here or in the management of web_stream_ that makes sure > > that if we reach this line, we have also called AddObserver for the same > object? > > RemoveObserver DCHECKs that its observer argument has been previously added with > AddObserver. See > https://cs.chromium.org/chromium/src/content/renderer/media/media_stream.cc?t... > I guess that's enough. Yep. https://codereview.chromium.org/2969093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2969093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:783: if (src_object_ == src_object) On 2017/07/05 13:05:02, Guido Urdaneta wrote: > On 2017/07/05 08:49:44, foolip wrote: > > https://html.spec.whatwg.org/multipage/media.html#dom-media-srcobject says "On > > setting, it must set the element's assigned media provider object to the new > > value, and then invoke the element's media element load algorithm." with no > > short-circuiting for when the value is the same. Is this a necessary part of > the > > fix? > > Reverted the change and added a test to make sure the load algorithm runs when > reassigning the same object. Great! https://codereview.chromium.org/2969093002/diff/40001/content/test/data/media... File content/test/data/media/getusermedia.html (right): https://codereview.chromium.org/2969093002/diff/40001/content/test/data/media... content/test/data/media/getusermedia.html:622: function srcObjectReassignSameObject() { This would be possible to test in web-platform-tests, but there are no existing tests for the HTML bits of it. So, this is OK.
https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:305: bool has_video_track = !video_tracks.IsEmpty() && const https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:308: if (has_video_track && !video_frame_provider_) { I have a question regarding multiple video track case implementation in blink. I am not sure how many video tracks are allowed, but consider this. There are 2 tracks: A-B. We start playing and add sink to only the first track A. Suppose A is removed. Then, we don't do anything based on this if else loop. Is that expected? https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:317: video_frame_provider_->Start(); You should also cover the uses of WebMediaPlayerMS::HasVideo(), since we can now start playing with no video and add it later on. i.e., RendererWebMediaPlayerDelegate::DidPlay() wouldn't reflect these changes as it is.
https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:305: bool has_video_track = !video_tracks.IsEmpty() && const https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:308: if (has_video_track && !video_frame_provider_) { I have a question regarding multiple video track case implementation in blink. I am not sure how many video tracks are allowed, but consider this. There are 2 tracks: A-B. We start playing and add sink to only the first track A. Suppose A is removed. Then, we don't do anything based on this if else loop. Is that expected? https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:317: video_frame_provider_->Start(); You should also cover the uses of WebMediaPlayerMS::HasVideo(), since we can now start playing with no video and add it later on. i.e., RendererWebMediaPlayerDelegate::DidPlay() wouldn't reflect these changes as it is.
https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:308: if (has_video_track && !video_frame_provider_) { On 2017/07/05 18:04:09, emircan wrote: > I have a question regarding multiple video track case implementation in blink. I > am not sure how many video tracks are allowed, but consider this. There are 2 > tracks: A-B. We start playing and add sink to only the first track A. Suppose A > is removed. Then, we don't do anything based on this if else loop. Is that > expected? Good question. I'll check what other browsers do ang get back to you. https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:317: video_frame_provider_->Start(); On 2017/07/05 18:04:09, emircan wrote: > You should also cover the uses of WebMediaPlayerMS::HasVideo(), since we can now > start playing with no video and add it later on. i.e., > RendererWebMediaPlayerDelegate::DidPlay() wouldn't reflect these changes as it > is. Can you elaborate a bit since I don't have the full context?
https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:317: video_frame_provider_->Start(); On 2017/07/05 19:01:08, Guido Urdaneta wrote: > On 2017/07/05 18:04:09, emircan wrote: > > You should also cover the uses of WebMediaPlayerMS::HasVideo(), since we can > now > > start playing with no video and add it later on. i.e., > > RendererWebMediaPlayerDelegate::DidPlay() wouldn't reflect these changes as it > > is. > > Can you elaborate a bit since I don't have the full context? > RendererWebMediaPlayerDelegate::DidPlay() [0] keeps a count of active videos. Suppose we start playing <video> where there are no video tracks initially and later on add a video track. AFACT, only Reload() will be called. RendererWebMediaPlayerDelegate would not be notified of this update. [0] https://cs.chromium.org/chromium/src/content/renderer/media/renderer_webmedia...
The CQ bit was checked by guidou@chromium.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.chromium.or...
https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:305: bool has_video_track = !video_tracks.IsEmpty() && On 2017/07/05 18:04:09, emircan wrote: > const Removed. https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:308: if (has_video_track && !video_frame_provider_) { On 2017/07/05 18:04:09, emircan wrote: > I have a question regarding multiple video track case implementation in blink. I > am not sure how many video tracks are allowed, but consider this. There are 2 > tracks: A-B. We start playing and add sink to only the first track A. Suppose A > is removed. Then, we don't do anything based on this if else loop. Is that > expected? Edge only plays a mediaStream if it has a single track. If it has 0, 2 or more tracks, nothing is rendered on the video element. Firefox, like Chrome, plays the first track. If the first track is removed, it stops playing, but if the same stream is reassigned to the element it plays the new first track, so it's probably a bug that it stops playing when the first track is removed. This new patchset makes Chrome always play the first track. If the first track is removed, keep playing the new first track. https://codereview.chromium.org/2969093002/diff/40001/content/renderer/media/... content/renderer/media/webmediaplayer_ms.cc:317: video_frame_provider_->Start(); On 2017/07/05 19:45:57, emircan wrote: > On 2017/07/05 19:01:08, Guido Urdaneta wrote: > > On 2017/07/05 18:04:09, emircan wrote: > > > You should also cover the uses of WebMediaPlayerMS::HasVideo(), since we can > > now > > > start playing with no video and add it later on. i.e., > > > RendererWebMediaPlayerDelegate::DidPlay() wouldn't reflect these changes as > it > > > is. > > > > Can you elaborate a bit since I don't have the full context? > > > > RendererWebMediaPlayerDelegate::DidPlay() [0] keeps a count of active videos. > Suppose we start playing <video> where there are no video tracks initially and > later on add a video track. AFACT, only Reload() will be called. > RendererWebMediaPlayerDelegate would not be notified of this update. > > [0] > https://cs.chromium.org/chromium/src/content/renderer/media/renderer_webmedia... As discussed offline, calling DidPlay twice causes a browser CHECK. Adding dalecurtis@ for his thoughts on this.
guidou@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@: Can you take a look? It is not clear what to do about the DidPlay notification. The browser CHECKs if DidPlay is sent twice. But if the media player starts with only audio, and then video is added, and DidPlay is not called, the browser-side observer will believe that the player is still audio only. Do you have any advice wrt this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by guidou@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by guidou@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
ping
Sorry don't look at Rietveld anymore. Does this still need review?
On 2017/07/28 23:56:31, DaleCurtis wrote: > Sorry don't look at Rietveld anymore. Does this still need review? Yes, it still needs review. No problem with the delay. I've been OOO for the last three weeks anyway. Thanks!
https://codereview.chromium.org/2969093002/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:242: web_stream_ = GetWebMediaStreamFromWebMediaPlayerSource(source); How can this be null now? I don't see a change for that.
https://codereview.chromium.org/2969093002/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:242: web_stream_ = GetWebMediaStreamFromWebMediaPlayerSource(source); On 2017/07/31 16:22:40, DaleCurtis wrote: > How can this be null now? I don't see a change for that. Null check in line 235 from the original file suggests that it can be null.
lgtm https://codereview.chromium.org/2969093002/diff/100001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2969093002/diff/100001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:242: web_stream_ = GetWebMediaStreamFromWebMediaPlayerSource(source); On 2017/07/31 at 16:30:23, Guido Urdaneta wrote: > On 2017/07/31 16:22:40, DaleCurtis wrote: > > How can this be null now? I don't see a change for that. > > Null check in line 235 from the original file suggests that it can be null. Ah missed that, thanks!
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2969093002/#ps100001 (title: "Fix unit tests")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, dalecurtis@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2969093002/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by guidou@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1501569888255880,
"parent_rev": "64abc8e04cf992bb93d014efcb43828dcab11298", "commit_rev":
"c9612ba8bf66e48a32301e35425dd3384f48a3d8"}
Message was sent while issue was closed.
Description was changed from ========== Make rendering of MediaStreams reflect changes to its set of tracks. Before this CL, MediaStreams assigned to a media element required reassignment of the stream to the element in order to make changes to the set of tracks visible. This CL fixes this problem by making WebMediaPlayerMS subscribe to changes in the set of tracks of a MediaStream, and correspondingly update audio and video renderers. BUG=720258 ========== to ========== Make rendering of MediaStreams reflect changes to its set of tracks. Before this CL, MediaStreams assigned to a media element required reassignment of the stream to the element in order to make changes to the set of tracks visible. This CL fixes this problem by making WebMediaPlayerMS subscribe to changes in the set of tracks of a MediaStream, and correspondingly update audio and video renderers. BUG=720258 Review-Url: https://codereview.chromium.org/2969093002 Cr-Commit-Position: refs/heads/master@{#490906} Committed: https://chromium.googlesource.com/chromium/src/+/c9612ba8bf66e48a32301e35425d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/c9612ba8bf66e48a32301e35425d...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:120001) has been created in https://codereview.chromium.org/2985393002/ by jkrcal@chromium.org. The reason for reverting is: Breaking build https://uberchromegw.corp.google.com/i/chromium.linux/builders/Cast%20Audio%2.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
