Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389473002:
Media Remoting: Add RemotingController. (Closed)
|
Created:
4 years, 2 months ago by xjz Modified:
4 years, 2 months ago Reviewers:
erickung1, sandersd (OOO until July 31), apacible, esprehn, jam, dcheng, miu, foolip, xhwang CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, chromoting-reviews_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, jam, kinuko+watch, mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: Add RemotingController.
Add a RemotingController which does the following:
1) Implements media::mojom::RemotingSource interface, and
sends/Receives messages from/to a media::mojom::Remoter;
2) Monitors player events as a MediaPlayerObserver;
3) May trigger the switch of the media renderer between local
playback and remoting.
BUG=643964
Committed: https://crrev.com/d3fe45a5f7f8395c26b5f75f0574487eb7618639
Cr-Commit-Position: refs/heads/master@{#424791}
Patch Set 1 #
Total comments: 58
Patch Set 2 : Addressed xhwang's comments. Rebased. #Patch Set 3 : Fix bug on Android. #
Total comments: 39
Patch Set 4 : Addressed xhwang's comments. #Patch Set 5 : Not use RemotingRendererFactory for Android. #Patch Set 6 : Add building flag. #
Total comments: 12
Patch Set 7 : Use BUILDFLAG. #
Total comments: 8
Patch Set 8 : Addressed comments. #Patch Set 9 : Rebased. #
Total comments: 11
Patch Set 10 : Remove changes in Blink from this CL. #Patch Set 11 : Rebased. #
Messages
Total messages: 87 (40 generated)
Description was changed from ========== Media Remoting: Add RemotingController. Add a RemotingController which does the following: 1) Implements media::mojom::RemotingSource interface, and sends/Receives messages from/to a media::mojom::Remoter; 2) Monitors player events as a MediaPlayerObserver; 3) May trigger the switch of the media renderer between local playback and remoting. BUG=643964 ========== to ========== Media Remoting: Add RemotingController. Add a RemotingController which does the following: 1) Implements media::mojom::RemotingSource interface, and sends/Receives messages from/to a media::mojom::Remoter; 2) Monitors player events as a MediaPlayerObserver; 3) May trigger the switch of the media renderer between local playback and remoting. BUG=643964 ==========
xjz@chromium.org changed reviewers: + miu@chromium.org, xhwang@chromium.org
xjz@chromium.org changed reviewers: + esprehn@chromium.org
xhwang: PTAL media/base/* and media/blink/*. esprehn: PTAL content/renderer/* and third_party/WebKit/*. miu: PTAL all :) Thanks all in advance!
What's the plan for moving the media code into blink? This is again making the API surface bigger, and we want to go the other direction and just speak mojo directly from blink.
On 2016/10/01 00:41:52, esprehn wrote: > What's the plan for moving the media code into blink? I believe xhwang@ would be in a better position to answer this question. xjz@ and I are not src/media/blink OWNERS. > This is again making the > API surface bigger, and we want to go the other direction and just speak mojo > directly from blink. IMHO, the changes xjz@ is making here aren't anything that would make a later migration of WebMediaPlayerImpl into Blink any more difficult than it already would be. FWIW, we've been working very closely with the Video Stack team throughout this process; and they've been very careful about making sure our code changes have minimum impact on the existing complexity of WMPI and related code. In fact, once we're done, there are huge chunks of code that will go away (such as WebMediaPlayerCast), which will actually make the migration of WMPI into Blink easier.
PS1 lgtm. Note: PS1 is identical to the final patchset of a "WIP" code review that went through several iterations and looks good to me.
This definitely looks better! Here are my first round of comments. https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... File media/base/mediaplayer_observer.h (right): https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:5: #ifndef MEDIA_BASE_MEDIAPLAYER_OBSERVER_H_ nit: media and player are two words. https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:16: class MEDIA_EXPORT MediaPlayerObserver { As commented before, we don't use "MediaPlayer" outside of media/blink. Also, the concept of fullscreen doesn't belong to media/base layer as well. For the former, how about just MediaObserver? We have a MediaObserver in content/public/browser, but that shouldn't matter. For the latter, I don't have any better idea at this moment :) https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:21: // Called when the media element or its ancestor is entered/exited fullscreen. What if the ancestor (e.g. a <div>) entered fullscreen, but the media element doesn't take 100% width/height of the <div>? https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:25: // Called when CDM is attached to the media element. Add a comment that the |cdm_context| is only guaranteed to be valid in this call. https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:28: // Set video/audio config after demuxer is initialized. nit: "Set" is ambiguous. This is really just notify the observer. How about "called when audio/video config is available after demuxer is initialized"? https://codereview.chromium.org/2389473002/diff/1/media/base/pipeline_metadata.h File media/base/pipeline_metadata.h (right): https://codereview.chromium.org/2389473002/diff/1/media/base/pipeline_metadat... media/base/pipeline_metadata.h:23: PipelineMetadata& operator=(const PipelineMetadata&); hmm, do you need these? https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:171: const WebMediaPlayerParams& params) Since the |observer| is optional, can you put it in WebMediaPlayerParams? I am not a fan of WebMediaPlayerParams, but for optional params it seems a good fit. https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1104: pipeline_metadata_.video_decoder_config); So if there's no video, then we don't call OnDecoderConfigChanged() even if we do have a AudioDecoderConfig... this is not consistent with the Media*Observer API. Maybe we should always call OnDecoderConfigChanged(), but the callee will decide what to do? https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:92: // |observer| outlives this class, and could be null. In your case, the |observer| is the same object as the |renderer_factory|, which is owned by |this| class. So you can't claim that the |observer| outlives this class, because in theory, this class can destroy the |renderer_factory| at any time since |this| owns it. Is it possible to pass in a weak_ptr of the |observer| so that we don't need to worry about the life time here? https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.cc:114: return false; You should be able to DCHECK in OnDecoderConfigChanged() that both audio/video configs are valid. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.cc:118: case VideoCodec::kCodecVP8: Just OCC, VP9 is not supported? https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.cc:131: return false; ditto https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:22: class MEDIA_EXPORT RemotingController final : public MediaPlayerObserver, You should NOT use MEDIA_EXPORT since this class doesn't belong to the "media" target. Actually since media/remoting is a source_set, I think you don't need *_EXPORT. Could you please check whether you really need this? If you do, media_remoting should have it's own EXPORT, e.g. MEDIA_REMOTING_EXPORT, similar to MEDIA_BLINK_EXPORT. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:26: // remain valid after the constructor returns. It's a bit odd that you pass in a factory and use it immediately in the ctor. In that case, why don't you call remoter_factory->Create() first, then construct this class, and pass in the RemotingSourceRequest and RemoterPtr? IMHO that feels more natural and you don't need this comment any more. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:36: void OnStopped(mojom::RemotingStopReason reson) override; s/reson/reason https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:43: const VideoDecoderConfig& video_config) override; nit: empty line here https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:55: bool isAudioConfigSupported(); nit: s/is/Is https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:59: void Update(); nit: this function name seems too generic and doesn't reflect what it does... https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:62: bool is_fullscreen_; nit: you can specify default value here, but different folder has different rule, so it's totally up to you. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:80: const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; Looks like this class single threaded? If so, add a comment. Also, it seems you only use |task_runner_| to make sure everything runs on the same thread. In this case, you can use ThreadChecker: https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?q=Thread... https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_factory.cc (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.cc:29: return std::unique_ptr<Renderer>(); If you check in this code today will this cause trouble? We assume CreateRenderer() never returns a nullptr: https://cs.chromium.org/chromium/src/media/filters/pipeline_controller.cc?rcl... https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=14752632... https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_factory.h (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.h:14: } You don't need this since they are from RendererFactory https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.h:19: class VideoRendererSink; ditto https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.h:24: class MEDIA_EXPORT RemotingRendererFactory : public RendererFactory { ditto about EXPORT https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.h:40: std::unique_ptr<RemotingController> const remoting_controller_; style nit: "putting the const first" https://google.github.io/styleguide/cppguide.html#Use_of_const https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebMediaPlayer.h:202: virtual void ancestorExitedFullscreen() {} See my previous comment on this. What if there are other elements sharing the same ancestor with the media element? You need someone who's more familiar with blink/elements/fullscreen to review this.
BTW, I only skimmed media/remoting/* since I am not familiar with the logic there.
On 2016/10/01 07:12:59, xhwang wrote: > BTW, I only skimmed media/remoting/* since I am not familiar with the logic > there. Also, do you plan to enable "media remoting" on all platforms? For example, will this be enabled on Chrome for Android? If not, we don't want increase the binary size for features that we don't use.
xjz@chromium.org changed reviewers: + erickung@chromium.org
On 2016/10/01 02:26:07, miu wrote: > On 2016/10/01 00:41:52, esprehn wrote: > > What's the plan for moving the media code into blink? > > I believe xhwang@ would be in a better position to answer this question. xjz@ > and I are not src/media/blink OWNERS. esprehn: We are migrating a lot of media out-of-process components from IPC to mojo. But currently I am not aware of any plan to move media/blink to use mojo (if I understand your question correctly). Do you have more context to share regarding that idea? Is there a larger plan to move all blink implementations (e.g. media/blink) to use mojo?
Addressed xhwang's comments. PTAL https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... File media/base/mediaplayer_observer.h (right): https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:5: #ifndef MEDIA_BASE_MEDIAPLAYER_OBSERVER_H_ On 2016/10/01 07:12:14, xhwang wrote: > nit: media and player are two words. Renamed to MediaObserver. https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:16: class MEDIA_EXPORT MediaPlayerObserver { On 2016/10/01 07:12:14, xhwang wrote: > As commented before, we don't use "MediaPlayer" outside of media/blink. Also, > the concept of fullscreen doesn't belong to media/base layer as well. For the > former, how about just MediaObserver? We have a MediaObserver in > content/public/browser, but that shouldn't matter. For the latter, I don't have > any better idea at this moment :) Renamed as "MediaObserver". https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:21: // Called when the media element or its ancestor is entered/exited fullscreen. On 2016/10/01 07:12:14, xhwang wrote: > What if the ancestor (e.g. a <div>) entered fullscreen, but the media element > doesn't take 100% width/height of the <div>? We currently treat this scenario same as the media element itself entering fullscreen. It seems reasonable for normal video websites (such as vimeo, youtube, etc.) . But We may add more criteria later if we find the logic doesn't work well. Added some comments in RemotingController.h for this. https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:25: // Called when CDM is attached to the media element. On 2016/10/01 07:12:14, xhwang wrote: > Add a comment that the |cdm_context| is only guaranteed to be valid in this > call. Done. https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_obse... media/base/mediaplayer_observer.h:28: // Set video/audio config after demuxer is initialized. On 2016/10/01 07:12:14, xhwang wrote: > nit: "Set" is ambiguous. This is really just notify the observer. How about > "called when audio/video config is available after demuxer is initialized"? Changed to pass PipelineMetadata instead to know if we have video/audio. https://codereview.chromium.org/2389473002/diff/1/media/base/pipeline_metadata.h File media/base/pipeline_metadata.h (right): https://codereview.chromium.org/2389473002/diff/1/media/base/pipeline_metadat... media/base/pipeline_metadata.h:23: PipelineMetadata& operator=(const PipelineMetadata&); On 2016/10/01 07:12:15, xhwang wrote: > hmm, do you need these? These are required by compiler. https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:171: const WebMediaPlayerParams& params) On 2016/10/01 07:12:15, xhwang wrote: > Since the |observer| is optional, can you put it in WebMediaPlayerParams? I am > not a fan of WebMediaPlayerParams, but for optional params it seems a good fit. Done. https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1104: pipeline_metadata_.video_decoder_config); On 2016/10/01 07:12:15, xhwang wrote: > So if there's no video, then we don't call OnDecoderConfigChanged() even if we > do have a AudioDecoderConfig... this is not consistent with the Media*Observer > API. Maybe we should always call OnDecoderConfigChanged(), but the callee will > decide what to do? Changed to pass the PipelineMetadata. And will be called when either audio or video is available. https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.h:92: // |observer| outlives this class, and could be null. On 2016/10/01 07:12:15, xhwang wrote: > In your case, the |observer| is the same object as the |renderer_factory|, which > is owned by |this| class. So you can't claim that the |observer| outlives this > class, because in theory, this class can destroy the |renderer_factory| at any > time since |this| owns it. > > Is it possible to pass in a weak_ptr of the |observer| so that we don't need to > worry about the life time here? Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.cc:114: return false; On 2016/10/01 07:12:15, xhwang wrote: > You should be able to DCHECK in OnDecoderConfigChanged() that both audio/video > configs are valid. Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.cc:118: case VideoCodec::kCodecVP8: On 2016/10/01 07:12:15, xhwang wrote: > Just OCC, VP9 is not supported? For now, assume receiver only supports H264 and VP8. Later we may add the query for the receiver's true capability. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.cc:131: return false; On 2016/10/01 07:12:15, xhwang wrote: > ditto Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:22: class MEDIA_EXPORT RemotingController final : public MediaPlayerObserver, On 2016/10/01 07:12:15, xhwang wrote: > You should NOT use MEDIA_EXPORT since this class doesn't belong to the "media" > target. > > Actually since media/remoting is a source_set, I think you don't need *_EXPORT. > Could you please check whether you really need this? If you do, media_remoting > should have it's own EXPORT, e.g. MEDIA_REMOTING_EXPORT, similar to > MEDIA_BLINK_EXPORT. Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:26: // remain valid after the constructor returns. On 2016/10/01 07:12:15, xhwang wrote: > It's a bit odd that you pass in a factory and use it immediately in the ctor. In > that case, why don't you call remoter_factory->Create() first, then construct > this class, and pass in the RemotingSourceRequest and RemoterPtr? IMHO that > feels more natural and you don't need this comment any more. Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:36: void OnStopped(mojom::RemotingStopReason reson) override; On 2016/10/01 07:12:15, xhwang wrote: > s/reson/reason Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:43: const VideoDecoderConfig& video_config) override; On 2016/10/01 07:12:15, xhwang wrote: > nit: empty line here Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:55: bool isAudioConfigSupported(); On 2016/10/01 07:12:15, xhwang wrote: > nit: s/is/Is Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:59: void Update(); On 2016/10/01 07:12:15, xhwang wrote: > nit: this function name seems too generic and doesn't reflect what it does... Renamed as UpdateAndMaybeSwitch. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:62: bool is_fullscreen_; On 2016/10/01 07:12:15, xhwang wrote: > nit: you can specify default value here, but different folder has different > rule, so it's totally up to you. Leave it with the constructor. :) https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:80: const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2016/10/01 07:12:15, xhwang wrote: > Looks like this class single threaded? If so, add a comment. > > Also, it seems you only use |task_runner_| to make sure everything runs on the > same thread. In this case, you can use ThreadChecker: > > https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?q=Thread... In the up-coming changes, we may need to add media thread task runner to handle the received RPC messages. So leave this as it is for now. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_factory.cc (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.cc:29: return std::unique_ptr<Renderer>(); On 2016/10/01 07:12:15, xhwang wrote: > If you check in this code today will this cause trouble? We assume > CreateRenderer() never returns a nullptr: > > https://cs.chromium.org/chromium/src/media/filters/pipeline_controller.cc?rcl... > https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=14752632... This will be never executed before we integrated all the changes. And this will not return a nullptr after the integration. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... File media/remoting/remoting_renderer_factory.h (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.h:14: } On 2016/10/01 07:12:15, xhwang wrote: > You don't need this since they are from RendererFactory Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.h:19: class VideoRendererSink; On 2016/10/01 07:12:15, xhwang wrote: > ditto Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.h:24: class MEDIA_EXPORT RemotingRendererFactory : public RendererFactory { On 2016/10/01 07:12:15, xhwang wrote: > ditto about EXPORT Done. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_ren... media/remoting/remoting_renderer_factory.h:40: std::unique_ptr<RemotingController> const remoting_controller_; On 2016/10/01 07:12:15, xhwang wrote: > style nit: "putting the const first" > > https://google.github.io/styleguide/cppguide.html#Use_of_const Done. https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebMediaPlayer.h:202: virtual void ancestorExitedFullscreen() {} On 2016/10/01 07:12:15, xhwang wrote: > See my previous comment on this. What if there are other elements sharing the > same ancestor with the media element? > > You need someone who's more familiar with blink/elements/fullscreen to review > this. Only one video element can start remoting successfully at one time. The current logic is the first come first serve. We may improve this logic in future change.
The CQ bit was checked by xjz@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by xjz@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: This issue passed the CQ dry run.
Looking pretty good! I just have some more comments (mostly nits). There are some comments in PS1. Also, I have a question about the rollout plan for you and miu@. https://codereview.chromium.org/2389473002/diff/1/media/base/pipeline_metadata.h File media/base/pipeline_metadata.h (right): https://codereview.chromium.org/2389473002/diff/1/media/base/pipeline_metadat... media/base/pipeline_metadata.h:23: PipelineMetadata& operator=(const PipelineMetadata&); On 2016/10/03 22:31:08, xjz wrote: > On 2016/10/01 07:12:15, xhwang wrote: > > hmm, do you need these? > > These are required by compiler. OOC, won't the compiler auto-generate these if you don't have them here explicitly? I might be missing something. So it seems we need more comments here... https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:80: const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2016/10/03 22:31:09, xjz wrote: > On 2016/10/01 07:12:15, xhwang wrote: > > Looks like this class single threaded? If so, add a comment. > > > > Also, it seems you only use |task_runner_| to make sure everything runs on the > > same thread. In this case, you can use ThreadChecker: > > > > > https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?q=Thread... > In the up-coming changes, we may need to add media thread task runner to handle > the received RPC messages. So leave this as it is for now. Please still add a comment about the threading model. You can update it in a later CL. https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebMediaPlayer.h:202: virtual void ancestorExitedFullscreen() {} On 2016/10/03 22:31:09, xjz wrote: > On 2016/10/01 07:12:15, xhwang wrote: > > See my previous comment on this. What if there are other elements sharing the > > same ancestor with the media element? > > > > You need someone who's more familiar with blink/elements/fullscreen to review > > this. > > Only one video element can start remoting successfully at one time. The current > logic is the first come first serve. We may improve this logic in future change. > Could you put this comment somewhere in the code? Just in case people may ask again. https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2676: #endif // defined(OS_ANDROID) || defined(ENABLE_MOJO_RENDERER) nit: Is it possible to move this whole block to a helper function? createMediaPlayer() is already too large to read... https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2714: std::move(remoting_controller))); Should we not use media::RemotingRendererFactory on Android? https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2714: std::move(remoting_controller))); +miu I am not sure whether the remoting sink has been hooked up yet (is_sink_available_ to be true). If you land this CL now, RemotingRendererFactory will be created and used. We need to make sure it never returns a null Renderer. What's the roll out plan? Do you want to enable this feature behind a flag first? Do you want to run experiments to compare mirroring and remoting? https://codereview.chromium.org/2389473002/diff/40001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2389473002/diff/40001/media/base/media_observ... media/base/media_observer.h:8: #include "base/callback.h" nit: not needed? https://codereview.chromium.org/2389473002/diff/40001/media/base/pipeline_met... File media/base/pipeline_metadata.h (right): https://codereview.chromium.org/2389473002/diff/40001/media/base/pipeline_met... media/base/pipeline_metadata.h:23: PipelineMetadata& operator=(const PipelineMetadata&); See my reply to my previous comment. https://codereview.chromium.org/2389473002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2389473002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1103: observer_->OnMetadata(metadata); The MediaObserver interface doesn't say that OnMetadata() should only be called when we have audio or video. I feel we should always call OnMetadata() and let the MediaObserver implementation decide what to do with the metadata. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:9: #include "media/base/bind_to_current_loop.h" nit: not needed? https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:22: weak_factory_(this) {} |has_audio| and |has_video| are not initialized. This could cause tricky issue that's hard to debug. That's why I like the "Non-Static Class Member Initializers" :) https://chromium-cpp.appspot.com/ https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:27: DCHECK(task_runner_->BelongsToCurrentThread()); Add include for base::SingleThreadTaskRunner https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:123: return true; API-wise, it's really odd that IsVideoConfigSupported() returns true when there's no video... It'll probably be cleaner for the caller to check if (has_video_ && IsVideoConfigSupported()) Then you can DCHECK(has_video) here. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:140: return true; ditto https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:179: IsAudioConfigSupported() && !video_decoder_config_.is_encrypted() && What if the audio is encrypted? https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:180: !switch_renderer_cb_.is_null(); In what case can |switch_renderer_cb_| be null? Can we assume it'll always be non-null and DCHECK? https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:180: !switch_renderer_cb_.is_null(); This line is a bit hard to reason about. Could you please add some comments summarizing the condition that we could enter remoting mode? Also, you could wrap this into a function like bool ShouldBeRemoting() The nice thing about a helper function is that you can now return early, for example: bool ShouldBeRemoting() { // Only do remoting when the tab is being casted blabla if (!is_sink_available_) return false; // Add comments if (!is_fullscreen_) return false; ...... } Then you can put comments for each condition and the code will be much easier to read. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:187: remoter_->Start(); Please add a comment that switch_renderer_cb_.Run() will be called after Start() finished (OnStarted()). https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.h:26: mojom::RemoterPtr remoter); no need for "explicit" now https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.h:37: // MediaObserver implementation. nit: You use "implementations" above but use "implementation" here. Be consistent :) https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.h:48: void SetSwitchRenderCallback(const SwitchRendererCallback& cb); nit: It should be SetSwitchRendererCallback https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.h:89: const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; forward declare base::SingleThreadTaskRunner?
Addressed xhwang's comments. PTAL. https://codereview.chromium.org/2389473002/diff/1/media/base/pipeline_metadata.h File media/base/pipeline_metadata.h (right): https://codereview.chromium.org/2389473002/diff/1/media/base/pipeline_metadat... media/base/pipeline_metadata.h:23: PipelineMetadata& operator=(const PipelineMetadata&); On 2016/10/04 06:30:29, xhwang wrote: > On 2016/10/03 22:31:08, xjz wrote: > > On 2016/10/01 07:12:15, xhwang wrote: > > > hmm, do you need these? > > > > These are required by compiler. > > OOC, won't the compiler auto-generate these if you don't have them here > explicitly? I might be missing something. So it seems we need more comments > here... Removed the assignment constructor, but copy constructor is required by chromium style. Added comment. https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_con... media/remoting/remoting_controller.h:80: const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2016/10/04 06:30:29, xhwang wrote: > On 2016/10/03 22:31:09, xjz wrote: > > On 2016/10/01 07:12:15, xhwang wrote: > > > Looks like this class single threaded? If so, add a comment. > > > > > > Also, it seems you only use |task_runner_| to make sure everything runs on > the > > > same thread. In this case, you can use ThreadChecker: > > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?q=Thread... > > In the up-coming changes, we may need to add media thread task runner to > handle > > the received RPC messages. So leave this as it is for now. > > Please still add a comment about the threading model. You can update it in a > later CL. Added a TODO comment. https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/WebMediaPlayer.h:202: virtual void ancestorExitedFullscreen() {} On 2016/10/04 06:30:29, xhwang wrote: > On 2016/10/03 22:31:09, xjz wrote: > > On 2016/10/01 07:12:15, xhwang wrote: > > > See my previous comment on this. What if there are other elements sharing > the > > > same ancestor with the media element? > > > > > > You need someone who's more familiar with blink/elements/fullscreen to > review > > > this. > > > > Only one video element can start remoting successfully at one time. The > current > > logic is the first come first serve. We may improve this logic in future > change. > > > > Could you put this comment somewhere in the code? Just in case people may ask > again. Done. https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2676: #endif // defined(OS_ANDROID) || defined(ENABLE_MOJO_RENDERER) On 2016/10/04 06:30:29, xhwang wrote: > nit: Is it possible to move this whole block to a helper function? > createMediaPlayer() is already too large to read... Added a helper to create the RemotingController. https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2714: std::move(remoting_controller))); On 2016/10/04 06:30:29, xhwang wrote: > +miu > > I am not sure whether the remoting sink has been hooked up yet > (is_sink_available_ to be true). If you land this CL now, > RemotingRendererFactory will be created and used. We need to make sure it never > returns a null Renderer. > > What's the roll out plan? Do you want to enable this feature behind a flag > first? Do you want to run experiments to compare mirroring and remoting? |is_sink_available_| will not be true until the change to MediaRouter is landed, which will happen in the final step. https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2714: std::move(remoting_controller))); On 2016/10/04 06:30:29, xhwang wrote: > Should we not use media::RemotingRendererFactory on Android? |remoting_controller| will be nullptr on Android. So RemotingRendererFactory is the same as the |default_renderer_factory|. https://codereview.chromium.org/2389473002/diff/40001/media/base/media_observ... File media/base/media_observer.h (right): https://codereview.chromium.org/2389473002/diff/40001/media/base/media_observ... media/base/media_observer.h:8: #include "base/callback.h" On 2016/10/04 06:30:29, xhwang wrote: > nit: not needed? Done. https://codereview.chromium.org/2389473002/diff/40001/media/base/pipeline_met... File media/base/pipeline_metadata.h (right): https://codereview.chromium.org/2389473002/diff/40001/media/base/pipeline_met... media/base/pipeline_metadata.h:23: PipelineMetadata& operator=(const PipelineMetadata&); On 2016/10/04 06:30:29, xhwang wrote: > See my reply to my previous comment. Replied. https://codereview.chromium.org/2389473002/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2389473002/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1103: observer_->OnMetadata(metadata); On 2016/10/04 06:30:29, xhwang wrote: > The MediaObserver interface doesn't say that OnMetadata() should only be called > when we have audio or video. I feel we should always call OnMetadata() and let > the MediaObserver implementation decide what to do with the metadata. Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:9: #include "media/base/bind_to_current_loop.h" On 2016/10/04 06:30:29, xhwang wrote: > nit: not needed? Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:22: weak_factory_(this) {} On 2016/10/04 06:30:29, xhwang wrote: > |has_audio| and |has_video| are not initialized. This could cause tricky issue > that's hard to debug. That's why I like the "Non-Static Class Member > Initializers" :) > > https://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:27: DCHECK(task_runner_->BelongsToCurrentThread()); On 2016/10/04 06:30:29, xhwang wrote: > Add include for base::SingleThreadTaskRunner Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:123: return true; On 2016/10/04 06:30:29, xhwang wrote: > API-wise, it's really odd that IsVideoConfigSupported() returns true when > there's no video... > > It'll probably be cleaner for the caller to check > > if (has_video_ && IsVideoConfigSupported()) > > Then you can DCHECK(has_video) here. Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:140: return true; On 2016/10/04 06:30:30, xhwang wrote: > ditto Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:179: IsAudioConfigSupported() && !video_decoder_config_.is_encrypted() && On 2016/10/04 06:30:30, xhwang wrote: > What if the audio is encrypted? Add check for encrypted audio too. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:180: !switch_renderer_cb_.is_null(); On 2016/10/04 06:30:29, xhwang wrote: > In what case can |switch_renderer_cb_| be null? Can we assume it'll always be > non-null and DCHECK? Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:180: !switch_renderer_cb_.is_null(); On 2016/10/04 06:30:30, xhwang wrote: > This line is a bit hard to reason about. Could you please add some comments > summarizing the condition that we could enter remoting mode? > > Also, you could wrap this into a function like > > bool ShouldBeRemoting() > > The nice thing about a helper function is that you can now return early, for > example: > > bool ShouldBeRemoting() { > // Only do remoting when the tab is being casted blabla > if (!is_sink_available_) > return false; > > // Add comments > if (!is_fullscreen_) > return false; > > ...... > } > > Then you can put comments for each condition and the code will be much easier to > read. Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.cc:187: remoter_->Start(); On 2016/10/04 06:30:29, xhwang wrote: > Please add a comment that switch_renderer_cb_.Run() will be called after Start() > finished (OnStarted()). Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.h:26: mojom::RemoterPtr remoter); On 2016/10/04 06:30:30, xhwang wrote: > no need for "explicit" now Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.h:37: // MediaObserver implementation. On 2016/10/04 06:30:30, xhwang wrote: > nit: You use "implementations" above but use "implementation" here. Be > consistent :) Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.h:48: void SetSwitchRenderCallback(const SwitchRendererCallback& cb); On 2016/10/04 06:30:30, xhwang wrote: > nit: It should be SetSwitchRendererCallback Done. https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting... media/remoting/remoting_controller.h:89: const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2016/10/04 06:30:30, xhwang wrote: > forward declare base::SingleThreadTaskRunner? Done.
Addressed xhwang's comments. PTAL https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2714: std::move(remoting_controller))); On 2016/10/04 19:21:29, xjz wrote: > On 2016/10/04 06:30:29, xhwang wrote: > > Should we not use media::RemotingRendererFactory on Android? > > |remoting_controller| will be nullptr on Android. So RemotingRendererFactory is > the same as the |default_renderer_factory|. As discussed offline, now used the DefaultRendererFactory for Android.
Add building flag to enable media remoting in PS#6. PTAL
xhwang@chromium.org changed reviewers: + sandersd@chromium.org
Looking good. I only have a few more comments/nits. sandersd: Could you please have a quick high level look as well, mostly on the interaction around WMPI/SetSwitchRendererCallback/ScheduleRestart. https://chromiumcodereview.appspot.com/2389473002/diff/100001/content/rendere... File content/renderer/BUILD.gn (right): https://chromiumcodereview.appspot.com/2389473002/diff/100001/content/rendere... content/renderer/BUILD.gn:515: defines += [ "ENABLE_MEDIA_REMOTING" ] This is the old way. For new code, please use BUILDFLAG: https://cs.chromium.org/chromium/src/build/buildflag_header.gni?sq=package:ch... Also, is it possible that you'll need to check this build flag in the browser side as well (e.g. content/browser)? Since enable_media_remoting is declared in media/, usually we define the buildflag in media/ as well. See this example: https://cs.chromium.org/chromium/src/media/BUILD.gn?rcl=0&l=16 https://chromiumcodereview.appspot.com/2389473002/diff/100001/content/rendere... File content/renderer/render_frame_impl.h (right): https://chromiumcodereview.appspot.com/2389473002/diff/100001/content/rendere... content/renderer/render_frame_impl.h:1053: // Create the RemotingController to control whether to switch to/from media nit: Create_s_ https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/media_opt... File media/media_options.gni (right): https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/media_opt... media/media_options.gni:124: enable_media_remoting = !is_chromecast && !is_ios && !is_android Please add documentation about what this is. https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/remoting/... File media/remoting/remoting_controller.cc (right): https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/remoting/... media/remoting/remoting_controller.cc:165: if (video_decoder_config_.is_encrypted()) How about make this check part of IsVideoConfigSupported()? https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/remoting/... File media/remoting/remoting_controller.h (right): https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/remoting/... media/remoting/remoting_controller.h:38: // logic is the first come first serve. This logic might be changed in future. Usually for overrides, we don't have extra comments. If we need comment for the interface, it should be in mojom::RemotingSource. If it's an implementation detail comment, it should be in the cc file.
Addressed xhwang's comments. PTAL. sandersd@: PTAL content::RenderFrameImpl::createMediaPlayer(), where WMPI::ScheduleRestart() is set as a callback to RemotingController to switch renderer. https://codereview.chromium.org/2389473002/diff/100001/content/renderer/BUILD.gn File content/renderer/BUILD.gn (right): https://codereview.chromium.org/2389473002/diff/100001/content/renderer/BUILD... content/renderer/BUILD.gn:515: defines += [ "ENABLE_MEDIA_REMOTING" ] On 2016/10/05 05:27:53, xhwang wrote: > This is the old way. For new code, please use BUILDFLAG: > > https://cs.chromium.org/chromium/src/build/buildflag_header.gni?sq=package:ch... > > Also, is it possible that you'll need to check this build flag in the browser > side as well (e.g. content/browser)? Since enable_media_remoting is declared in > media/, usually we define the buildflag in media/ as well. See this example: > > https://cs.chromium.org/chromium/src/media/BUILD.gn?rcl=0&l=16 Done. https://codereview.chromium.org/2389473002/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/2389473002/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:1053: // Create the RemotingController to control whether to switch to/from media On 2016/10/05 05:27:53, xhwang wrote: > nit: Create_s_ Done. https://codereview.chromium.org/2389473002/diff/100001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/2389473002/diff/100001/media/media_options.gn... media/media_options.gni:124: enable_media_remoting = !is_chromecast && !is_ios && !is_android On 2016/10/05 05:27:53, xhwang wrote: > Please add documentation about what this is. Done. https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remotin... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remotin... media/remoting/remoting_controller.cc:165: if (video_decoder_config_.is_encrypted()) On 2016/10/05 05:27:53, xhwang wrote: > How about make this check part of IsVideoConfigSupported()? I would leave this check here for now. It will change in the later CL when the control for EME is added. https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remotin... File media/remoting/remoting_controller.h (right): https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remotin... media/remoting/remoting_controller.h:38: // logic is the first come first serve. This logic might be changed in future. On 2016/10/05 05:27:53, xhwang wrote: > Usually for overrides, we don't have extra comments. If we need comment for the > interface, it should be in mojom::RemotingSource. If it's an implementation > detail comment, it should be in the cc file. Removed this comment here. This is controled by browser, and the comment is already there: chrome/browser/media/cast_remoting_connector.h
Nice work! LGTM from my side with two last nits. Please wait for sandersd@'s response for full media/ OWNERS approval. https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remotin... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remotin... media/remoting/remoting_controller.cc:165: if (video_decoder_config_.is_encrypted()) On 2016/10/05 17:24:23, xjz wrote: > On 2016/10/05 05:27:53, xhwang wrote: > > How about make this check part of IsVideoConfigSupported()? > > I would leave this check here for now. It will change in the later CL when the > control for EME is added. Then please at least check |has_video_| before checking video_decoder_config_.is_encrypted(), otherwise you are relying on the default value of VideoDecoderConfig, which could be risky. https://codereview.chromium.org/2389473002/diff/120001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/2389473002/diff/120001/media/media_options.gn... media/media_options.gni:124: # Enable to switch media renderer between remoting and local playback. Could you please explain more about what "remoting" is? Also, you are not "switching" one media renderer between remoting and local playback, you are switching between local renderer and remoting renderer...
ping esprehn@: Does the changes to third_party/WebKit/* LGTY? The purpose of the changes there is to notify WebMediaPlayerImpl that the media element or its ancestor enters/exits full screen.
PS7 lgtm % nits: https://codereview.chromium.org/2389473002/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2389473002/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:2725: std::unique_ptr<media::RendererFactory> media_renderer_factory( nit: This is duplicate code. Could we simplify a little by: #if defined(ENABLE_MOJO_RENDERER) ... #else std::unique_ptr<media::RendererFactory> media_renderer_factory(new DefaultRendererFactory...) #endif #if BUILDFLAG(ENABLE_MEDIA_REMOTING) media::RemotingController* remoting_controller_ptr = remoting_controller.get(); media_renderer_factory = base::MakeUnique<media::RemotingRendererFactory>( std::move(media_renderer_factory), std::move(remoting_controller)); #endif This way, it's very clear that the RemotingRendererFactory wraps the RendererFactory. https://codereview.chromium.org/2389473002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2389473002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1103: observer_->OnMetadata(metadata); naming nit: OnMetadataChanged() https://codereview.chromium.org/2389473002/diff/120001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/2389473002/diff/120001/media/media_options.gn... media/media_options.gni:124: # Enable to switch media renderer between remoting and local playback. Also, it would be more accurate to say that this switch defines whether the Media Remoting implementation will be built.
> sandersd@: PTAL content::RenderFrameImpl::createMediaPlayer(), where > WMPI::ScheduleRestart() is set as a callback to RemotingController to switch > renderer. This pattern is a little strange, since it's exposing internal details, but I don't see anything wrong with it as long as there is a 1:1 mapping with WMPI instances (which appears to be the case). I would prefer something more like the delegate, where WMPI explicitly registers itself early in the lifecycle. Ideally it could unregister as well; it's a little strange that the remoting controller assumes that the callback will function when in is in fact possible that the callback goes nowhere. I'll take this code 10x over the current |is_remote_| code, so lgtm overall.
On 2016/10/05 21:28:29, sandersd wrote: > I would prefer something more like the delegate, where WMPI explicitly registers > itself early in the lifecycle. Ideally it could unregister as well; it's a > little strange that the remoting controller assumes that the callback will > function when in is in fact possible that the callback goes nowhere. The "weak pointer" in the callback won't actually ever be invalidated before the callback is run. The ownership semantics are weird, but there really isn't obviously a better way: 1. RemotingRendererFactory owns RemotingController. 2a. WMPI takes ownership of RemotingRendererFactory. 2b. We also provide WMPI a raw pointer to the MediaObserver (which is impl by the same RemotingController). 3. If WMPI is destroyed, it'll destroy RemotingRendererFactory, which will in turn destroy RemotingController. In other words, WMPI becomes the root of the object ownership graph, and RemotingController would be a leaf. It's worth pointing out that a lot of all this weirdness stems from eliminating any dependencies between //media/blink and //media/remoting. ;-)
On 2016/10/05 21:40:53, miu wrote: > On 2016/10/05 21:28:29, sandersd wrote: > > I would prefer something more like the delegate, where WMPI explicitly > registers > > itself early in the lifecycle. Ideally it could unregister as well; it's a > > little strange that the remoting controller assumes that the callback will > > function when in is in fact possible that the callback goes nowhere. > > The "weak pointer" in the callback won't actually ever be invalidated before the > callback is run. The ownership semantics are weird, but there really isn't > obviously a better way: > > 1. RemotingRendererFactory owns RemotingController. > 2a. WMPI takes ownership of RemotingRendererFactory. > 2b. We also provide WMPI a raw pointer to the MediaObserver (which is impl by > the same RemotingController). > 3. If WMPI is destroyed, it'll destroy RemotingRendererFactory, which will in > turn destroy RemotingController. > > In other words, WMPI becomes the root of the object ownership graph, and > RemotingController would be a leaf. Thanks for the clarifications! > It's worth pointing out that a lot of all this weirdness stems from eliminating > any dependencies between //media/blink and //media/remoting. ;-) I always manage to forget about this.
Thanks miu@, xhwang@, and sandersd@! Addressed comments in PS8. https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remotin... File media/remoting/remoting_controller.cc (right): https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remotin... media/remoting/remoting_controller.cc:165: if (video_decoder_config_.is_encrypted()) On 2016/10/05 17:36:41, xhwang wrote: > On 2016/10/05 17:24:23, xjz wrote: > > On 2016/10/05 05:27:53, xhwang wrote: > > > How about make this check part of IsVideoConfigSupported()? > > > > I would leave this check here for now. It will change in the later CL when the > > control for EME is added. > > Then please at least check |has_video_| before checking > video_decoder_config_.is_encrypted(), otherwise you are relying on the default > value of VideoDecoderConfig, which could be risky. Done. https://codereview.chromium.org/2389473002/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2389473002/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:2725: std::unique_ptr<media::RendererFactory> media_renderer_factory( On 2016/10/05 19:13:57, miu wrote: > nit: This is duplicate code. Could we simplify a little by: > > #if defined(ENABLE_MOJO_RENDERER) > ... > #else > std::unique_ptr<media::RendererFactory> media_renderer_factory(new > DefaultRendererFactory...) > #endif > > #if BUILDFLAG(ENABLE_MEDIA_REMOTING) > media::RemotingController* remoting_controller_ptr = > remoting_controller.get(); > media_renderer_factory = base::MakeUnique<media::RemotingRendererFactory>( > std::move(media_renderer_factory), std::move(remoting_controller)); > #endif > > This way, it's very clear that the RemotingRendererFactory wraps the > RendererFactory. Done. https://codereview.chromium.org/2389473002/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2389473002/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1103: observer_->OnMetadata(metadata); On 2016/10/05 19:13:57, miu wrote: > naming nit: OnMetadataChanged() Done. https://codereview.chromium.org/2389473002/diff/120001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/2389473002/diff/120001/media/media_options.gn... media/media_options.gni:124: # Enable to switch media renderer between remoting and local playback. On 2016/10/05 19:13:57, miu wrote: > Also, it would be more accurate to say that this switch defines whether the > Media Remoting implementation will be built. Done. https://codereview.chromium.org/2389473002/diff/120001/media/media_options.gn... media/media_options.gni:124: # Enable to switch media renderer between remoting and local playback. On 2016/10/05 17:36:41, xhwang wrote: > Could you please explain more about what "remoting" is? Also, you are not > "switching" one media renderer between remoting and local playback, you are > switching between local renderer and remoting renderer... Done.
The CQ bit was checked by xjz@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 xjz@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
esprehn@chromium.org changed reviewers: + foolip@chromium.org
foolip@ Can you help them with the full screen code? This patch doesn't feel right, you're crawling the entire document every time we go full screen looking for videos, for some pages that might be thousands of elements. https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:795: if (fullscreenElement->contains(&htmlMediaElement)) This doesn't understand shadow dom, also what if the full screen element is in another frame? You'd need isShadowIncludingInclusiveAncestorOf to handle Shadow DOM correctly. https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FullscreenController.cpp:92: Traversal<HTMLVideoElement>::descendantsOf(*element)) { This is traversing every element of the element which just went full screen, I don't think we want to do that, also this doesn't handle Shadow DOM, what are you trying to do here? https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FullscreenController.cpp:132: Traversal<HTMLVideoElement>::descendantsOf(*element)) { ditto, walking the entire document seems unfortunate, why do we need this callback?
Thanks esprehn@. What we want to do here is to notify WebMediaPlayer when user clicks full screen button in the media controller, so that for tab content mirroring we can directly send the media stream to remote devices and render the media there. This is to improve the mirroring quality by eliminating all the local rendering, capturing, and re-encoding processes. As you can see, I am not familiar with full screen / DOM related codes. Any questions and suggestions are more than welcome. BTW, Is there any event for full screen that we can listen to? On 2016/10/08 03:33:04, esprehn wrote: > foolip@ Can you help them with the full screen code? This patch doesn't feel > right, you're crawling the entire document every time we go full screen looking > for videos, for some pages that might be thousands of elements. > > https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): > > https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:795: if > (fullscreenElement->contains(&htmlMediaElement)) > This doesn't understand shadow dom, also what if the full screen element is in > another frame? > > You'd need isShadowIncludingInclusiveAncestorOf to handle Shadow DOM correctly. > > https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/FullscreenController.cpp (right): > > https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/FullscreenController.cpp:92: > Traversal<HTMLVideoElement>::descendantsOf(*element)) { > This is traversing every element of the element which just went full screen, I > don't think we want to do that, also this doesn't handle Shadow DOM, what are > you trying to do here? > > https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/FullscreenController.cpp:132: > Traversal<HTMLVideoElement>::descendantsOf(*element)) { > ditto, walking the entire document seems unfortunate, why do we need this > callback?
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Fullscreen... may be a good starting point for this. FullscreenController::didEnterFullscreen() knows exactly what element is going fullscreen (and already has some video element specific logic, it looks like)
Thanks dcheng@! If the video element itself goes to full screen, WebMediaPlayer can get notified through FullscreenController::didEnterFullscreen(). But for the web videos (such as Vimeo, YouTube videos), when user clicks full screen button, it is actually the DIV element that becomes the full screen element. And the video element doesn't know this. What I did in this CL is just trying to find the video element and notify it in this case. On 2016/10/08 05:02:32, dcheng wrote: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Fullscreen... > may be a good starting point for this. > > FullscreenController::didEnterFullscreen() knows exactly what element is going > fullscreen (and already has some video element specific logic, it looks like)
On 2016/10/08 05:13:00, xjz wrote: > Thanks dcheng@! > > If the video element itself goes to full screen, WebMediaPlayer can get notified > through FullscreenController::didEnterFullscreen(). > But for the web videos (such as Vimeo, YouTube videos), when user clicks full > screen button, > it is actually the DIV element that becomes the full screen element. And the > video element doesn't know this. > What I did in this CL is just trying to find the video element and notify it in > this case. > > > On 2016/10/08 05:02:32, dcheng wrote: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Fullscreen... > > may be a good starting point for this. > > > > FullscreenController::didEnterFullscreen() knows exactly what element is going > > fullscreen (and already has some video element specific logic, it looks like) Ah, I'm sorry. I was only answering the question about listening for the fullscreen event; I should have looked at the diffs as well. If we want to find video descendants, I don't really see a better way other than traversing through children of the fullscreen element or keeping a registry of video elements so we can check just the video elements. esprehn@, are you concerned about the traversal because documents are often fullscreened? xjz@, what happens if there's more than one video element that's playing?
https://codereview.chromium.org/2389473002/diff/160001/media/base/media_obser... File media/base/media_observer.h (right): https://codereview.chromium.org/2389473002/diff/160001/media/base/media_obser... media/base/media_observer.h:19: // Called when the media element or its ancestor is entered/exited fullscreen. Stray "is" https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FullscreenController.cpp:92: Traversal<HTMLVideoElement>::descendantsOf(*element)) { On 2016/10/08 03:33:04, esprehn wrote: > This is traversing every element of the element which just went full screen, I > don't think we want to do that, also this doesn't handle Shadow DOM, what are > you trying to do here? I believe the idea is to detect a case where the video is the only thing visible and switch to the other mode, but I think CL will do it even if there are multiple videos in fullscreen, a single video as part of a presentation, and perhaps other cases. I was in an email thread about this, but I don't think I understand all of the constraints. It seems that it needs to integrate with some or all of layout, paint and compositing, perhaps something similar to the code using HTMLVideoElement::usesOverlayFullscreenVideo. Detecting this as part of layout should mean that the case where the video element is the fullscreen element or one of the descendants could be treated the same, all that matters is that there's nothing obscuring it.
https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FullscreenController.cpp:92: Traversal<HTMLVideoElement>::descendantsOf(*element)) { On 2016/10/10 08:35:22, foolip wrote: > On 2016/10/08 03:33:04, esprehn wrote: > > This is traversing every element of the element which just went full screen, I > > don't think we want to do that, also this doesn't handle Shadow DOM, what are > > you trying to do here? > > I believe the idea is to detect a case where the video is the only thing visible > and switch to the other mode, but I think CL will do it even if there are > multiple videos in fullscreen, a single video as part of a presentation, and > perhaps other cases. > > I was in an email thread about this, but I don't think I understand all of the > constraints. It seems that it needs to integrate with some or all of layout, > paint and compositing, perhaps something similar to the code using > HTMLVideoElement::usesOverlayFullscreenVideo. > > Detecting this as part of layout should mean that the case where the video > element is the fullscreen element or one of the descendants could be treated the > same, all that matters is that there's nothing obscuring it. Please make sure to write tests that show the behavior both for when the logic should kick in and some of the cases where it shouldn't, which I hope would include the case where there's a lot of stuff covering the video and where the video doesn't cover most of the screen.
On 2016/10/10 08:35:22, foolip wrote: > https://codereview.chromium.org/2389473002/diff/160001/media/base/media_obser... > File media/base/media_observer.h (right): > > https://codereview.chromium.org/2389473002/diff/160001/media/base/media_obser... > media/base/media_observer.h:19: // Called when the media element or its ancestor > is entered/exited fullscreen. > Stray "is" > > https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/FullscreenController.cpp (right): > > https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/FullscreenController.cpp:92: > Traversal<HTMLVideoElement>::descendantsOf(*element)) { > On 2016/10/08 03:33:04, esprehn wrote: > > This is traversing every element of the element which just went full screen, I > > don't think we want to do that, also this doesn't handle Shadow DOM, what are > > you trying to do here? > > I believe the idea is to detect a case where the video is the only thing visible > and switch to the other mode, but I think CL will do it even if there are > multiple videos in fullscreen, a single video as part of a presentation, and > perhaps other cases. > > I was in an email thread about this, but I don't think I understand all of the > constraints. It seems that it needs to integrate with some or all of layout, > paint and compositing, perhaps something similar to the code using > HTMLVideoElement::usesOverlayFullscreenVideo. > > Detecting this as part of layout should mean that the case where the video > element is the fullscreen element or one of the descendants could be treated the > same, all that matters is that there's nothing obscuring it. Thanks foolip@ and dcheng@! As in media remoting, only media stream can be remoted to remote devices, I was originally trying to find a way to detect the video element that covers the most of the screen when full screen button is clicked, but finally didn't find a proper way for that. So now in this CL if the current fullscreen element is not HTMLMediaElement, it just simply notifies every video descendant that its ancestor entered full screen. This logic seems work except the following scenarios: 1. There exist multiple video elements in the tab. All video elements may try to start media remoting, but at most only one will succeed. The current policy for this is "first come, first served".https://cs.chromium.org/chromium/src/chrome/browser/media/cast_remoting_connector.h?rcl=0&l=48 The remaining video elements will still be rendered on local screen in this case. 2. There are other contents in the tab. Only the video element will be remotely played on TV, all other elements will stay on local screen. Currently we don't have data showing how often these cases happen in mirroring. But it seems not a problem for common websites (such as Vimeo, YouTube, Twitch.tv, etc.). So we decided to go with this simple logic for now. Foolip@: Do you know if there is a way to detect if the video covers most of the screen (e.g., >80%) in WebMediaPlayerImpl? It is the best if we can detect this, and only remote the video that covers most of the screen. Thanks!
The CQ bit was checked by xjz@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks all for reviewing. It seems I may need re-think how to get notification when non-HTMLMediaElement enters/exits full screen. Since this CL blocks several other Remoting CLs for landing, I currently removes the related changes and will address this issue in a separate CL. esprehn@: I still need your approval for changes in content/renderer/*. PTAL. Thanks! https://codereview.chromium.org/2389473002/diff/160001/media/base/media_obser... File media/base/media_observer.h (right): https://codereview.chromium.org/2389473002/diff/160001/media/base/media_obser... media/base/media_observer.h:19: // Called when the media element or its ancestor is entered/exited fullscreen. On 2016/10/10 08:35:22, foolip wrote: > Stray "is" Done. https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:795: if (fullscreenElement->contains(&htmlMediaElement)) On 2016/10/08 03:33:04, esprehn wrote: > This doesn't understand shadow dom, also what if the full screen element is in > another frame? > > You'd need isShadowIncludingInclusiveAncestorOf to handle Shadow DOM correctly. Removed this from this CL. As commented, will open a new CL for all full screen related changes, and will address this there. https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FullscreenController.cpp:92: Traversal<HTMLVideoElement>::descendantsOf(*element)) { On 2016/10/10 08:39:08, foolip wrote: > On 2016/10/10 08:35:22, foolip wrote: > > On 2016/10/08 03:33:04, esprehn wrote: > > > This is traversing every element of the element which just went full screen, > I > > > don't think we want to do that, also this doesn't handle Shadow DOM, what > are > > > you trying to do here? > > > > I believe the idea is to detect a case where the video is the only thing > visible > > and switch to the other mode, but I think CL will do it even if there are > > multiple videos in fullscreen, a single video as part of a presentation, and > > perhaps other cases. > > > > I was in an email thread about this, but I don't think I understand all of the > > constraints. It seems that it needs to integrate with some or all of layout, > > paint and compositing, perhaps something similar to the code using > > HTMLVideoElement::usesOverlayFullscreenVideo. > > > > Detecting this as part of layout should mean that the case where the video > > element is the fullscreen element or one of the descendants could be treated > the > > same, all that matters is that there's nothing obscuring it. > > Please make sure to write tests that show the behavior both for when the logic > should kick in and some of the cases where it shouldn't, which I hope would > include the case where there's a lot of stuff covering the video and where the > video doesn't cover most of the screen. Removed this from this CL. As commented, will open a new CL for all full screen related changes. https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FullscreenController.cpp:132: Traversal<HTMLVideoElement>::descendantsOf(*element)) { On 2016/10/08 03:33:04, esprehn wrote: > ditto, walking the entire document seems unfortunate, why do we need this > callback? Removed this from this CL. As commented, will open a new CL for all full screen related changes. https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:232: virtual void ancestorExitedFullscreen() {} Removed this from this CL. As commented, will open a new CL for all full screen related changes, and will address this there.
The CQ bit was checked by xjz@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...
A few thoughts on the ancestroEnteredFullscreen().
What if the video occupies the full window/tab (e.g. the default mode when you
start watching Netflix/AIV), but it's not fullscreen? In this case shall we
enable remoting?
In either case, it seems to me the ultimate question is "whether the video
occupies the full window (or the full screen)?" I wonder whether we can answer
this by measuring the sizes directly, e.g.:
// offsetWidth() is the real width of the <video> on screen.
// document().body()->clientWidth() is the window width.
bool HTMLMediaElement::isFullWindow() {
return offsetWidth() == document().body()->clientWidth();
}
// offsetWidth() is the real width of the <video> on screen.
// document().executingWindow()->screen()->width() is the screen width.
bool HTMLMediaElement::isDefactoFullScreen() {
return offsetWidth() == document().executingWindow()->screen()->width();
}
I tried on Netflix and AIV (which uses <div> fullscreen) and these two functions
both work. I am not familiar with all the sizes in blink so this is just an
example to show the idea. Any thoughts?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks xhwang@. Replied in line.
On 2016/10/10 20:12:02, xhwang wrote:
> A few thoughts on the ancestroEnteredFullscreen().
>
> What if the video occupies the full window/tab (e.g. the default mode when you
> start watching Netflix/AIV), but it's not fullscreen? In this case shall we
> enable remoting?
We currently wait for user's gesture (clicking full screen button) to activate
checking
if the video covers most of the tab window.
>
> In either case, it seems to me the ultimate question is "whether the video
> occupies the full window (or the full screen)?" I wonder whether we can answer
> this by measuring the sizes directly, e.g.:
>
> // offsetWidth() is the real width of the <video> on screen.
> // document().body()->clientWidth() is the window width.
> bool HTMLMediaElement::isFullWindow() {
> return offsetWidth() == document().body()->clientWidth();
> }
>
> // offsetWidth() is the real width of the <video> on screen.
> // document().executingWindow()->screen()->width() is the screen width.
> bool HTMLMediaElement::isDefactoFullScreen() {
> return offsetWidth() == document().executingWindow()->screen()->width();
> }
I am not really sure which size I should check. I did test some similar logic
before by comparing
HTMLMediaElement.clientWidth/height v.s.
document().domWindow()->innerWidth()/innerHeigh().
That logic seems work for most web sites, but not for YouTube.
>
> I tried on Netflix and AIV (which uses <div> fullscreen) and these two
functions
> both work. I am not familiar with all the sizes in blink so this is just an
> example to show the idea. Any thoughts?
Now I separated this full screen related changes to another CL:
https://codereview.chromium.org/2411553003/
Please continue discuss in that CL. :) Thanks!
esprehn@: need owner's approval on changes in content/*. PTAL Thanks!
On 2016/10/10 17:48:48, xjz wrote: > Foolip@: Do you know if there is a way to detect if the video covers most of the > screen (e.g., >80%) in WebMediaPlayerImpl? It is the best if we can detect this, > and only remote the video that covers most of the screen. Thanks! Let us continue this in https://codereview.chromium.org/2411553003/
This lgtm to me, but the plumbing between blink isn't clear to me, how does that work?
On 2016/10/11 at 21:42:23, esprehn wrote: > This lgtm to me, but the plumbing between blink isn't clear to me, how does that work? Also it's not clear to me that the web platform side of this will work, and that we'll be able to land that other change. So while we can go forward with this change, I think the feature itself needs an API review, and I don't think we can land the other blink change yet.
On 2016/10/11 21:42:23, esprehn wrote: > This lgtm to me, but the plumbing between blink isn't clear to me, how does that > work? Thanks esprehn@! The blink changes in another CL is independent with this one. This Cl will only enable to remote videos that are active fullscreen element, which doesn't require any change in blink since the notification was already plumbed in. The blink changes in the other CL are trying to enable the remoting for videos that are not fullscreen element. For example, when DIV element goes to full screen, but the video actually occupies the whole/most of the window. For this, we can continue the discussion in that CL.
On 2016/10/11 22:03:41, esprehn wrote: > On 2016/10/11 at 21:42:23, esprehn wrote: > > This lgtm to me, but the plumbing between blink isn't clear to me, how does > that work? > > Also it's not clear to me that the web platform side of this will work, and that > we'll be able to land that other change. So while we can go forward with this > change, I think the feature itself needs an API review, and I don't think we can > land the other blink change yet. OK. This CL has no API change at all. I'll land this first to unblock the others. And will continue the discussion in the other CL. Thanks!
On 2016/10/11 at 22:06:35, xjz wrote: > On 2016/10/11 21:42:23, esprehn wrote: > > This lgtm to me, but the plumbing between blink isn't clear to me, how does that > > work? > > Thanks esprehn@! > > The blink changes in another CL is independent with this one. > This Cl will only enable to remote videos that are active fullscreen element, > which doesn't require any change in blink since the notification was already plumbed in. > > The blink changes in the other CL are trying to enable the remoting for videos that are > not fullscreen element. For example, when DIV element goes to full screen, but the video > actually occupies the whole/most of the window. For this, we can continue the discussion > in that CL. Okay, lgtm then.
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, miu@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2389473002/#ps200001 (title: "Rebased.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
xjz@chromium.org changed reviewers: + jam@chromium.org
jam@: need approval for media/remoting/DEPS. Thanks!
lgtm
The CQ bit was checked by xjz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Add RemotingController. Add a RemotingController which does the following: 1) Implements media::mojom::RemotingSource interface, and sends/Receives messages from/to a media::mojom::Remoter; 2) Monitors player events as a MediaPlayerObserver; 3) May trigger the switch of the media renderer between local playback and remoting. BUG=643964 ========== to ========== Media Remoting: Add RemotingController. Add a RemotingController which does the following: 1) Implements media::mojom::RemotingSource interface, and sends/Receives messages from/to a media::mojom::Remoter; 2) Monitors player events as a MediaPlayerObserver; 3) May trigger the switch of the media renderer between local playback and remoting. BUG=643964 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Add RemotingController. Add a RemotingController which does the following: 1) Implements media::mojom::RemotingSource interface, and sends/Receives messages from/to a media::mojom::Remoter; 2) Monitors player events as a MediaPlayerObserver; 3) May trigger the switch of the media renderer between local playback and remoting. BUG=643964 ========== to ========== Media Remoting: Add RemotingController. Add a RemotingController which does the following: 1) Implements media::mojom::RemotingSource interface, and sends/Receives messages from/to a media::mojom::Remoter; 2) Monitors player events as a MediaPlayerObserver; 3) May trigger the switch of the media renderer between local playback and remoting. BUG=643964 Committed: https://crrev.com/d3fe45a5f7f8395c26b5f75f0574487eb7618639 Cr-Commit-Position: refs/heads/master@{#424791} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d3fe45a5f7f8395c26b5f75f0574487eb7618639 Cr-Commit-Position: refs/heads/master@{#424791}
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Add RemotingController. Add a RemotingController which does the following: 1) Implements media::mojom::RemotingSource interface, and sends/Receives messages from/to a media::mojom::Remoter; 2) Monitors player events as a MediaPlayerObserver; 3) May trigger the switch of the media renderer between local playback and remoting. BUG=643964 Committed: https://crrev.com/d3fe45a5f7f8395c26b5f75f0574487eb7618639 Cr-Commit-Position: refs/heads/master@{#424791} ========== to ========== Media Remoting: Add RemotingController. Add a RemotingController which does the following: 1) Implements media::mojom::RemotingSource interface, and sends/Receives messages from/to a media::mojom::Remoter; 2) Monitors player events as a MediaPlayerObserver; 3) May trigger the switch of the media renderer between local playback and remoting. BUG=643964 Committed: https://crrev.com/d3fe45a5f7f8395c26b5f75f0574487eb7618639 Cr-Commit-Position: refs/heads/master@{#424791} ==========
Message was sent while issue was closed.
apacible@chromium.org changed reviewers: + apacible@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
