Media Remoting: Add RemotingController. (issue 2389473002 by xjz@chromium.org)

[x...@chromium.org]

Reviewers: miu, xhwang, esprehn
CL: https://codereview.chromium.org/2389473002/

Message:
xhwang: PTAL media/base/* and media/blink/*.
esprehn: PTAL content/renderer/* and third_party/WebKit/*.
miu: PTAL all :)

Thanks all in advance!

Description:
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

Affected files (+692, -20 lines):
M content/renderer/BUILD.gn
M content/renderer/render_frame_impl.cc
M media/base/BUILD.gn
A media/base/mediaplayer_observer.h
A + media/base/mediaplayer_observer.cc
M media/base/pipeline_impl.cc
M media/base/pipeline_metadata.h
A media/base/pipeline_metadata.cc
M media/blink/webmediaplayer_impl.h
M media/blink/webmediaplayer_impl.cc
M media/blink/webmediaplayer_impl_unittest.cc
M media/remoting/BUILD.gn
A + media/remoting/DEPS
A media/remoting/remoting_controller.h
A media/remoting/remoting_controller.cc
A media/remoting/remoting_controller_unittest.cc
A media/remoting/remoting_renderer_factory.h
A media/remoting/remoting_renderer_factory.cc
M third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp
M third_party/WebKit/Source/web/FullscreenController.cpp
M third_party/WebKit/public/platform/WebMediaPlayer.h

[esp...@chromium.org]

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.

https://codereview.chromium.org/2389473002/

[m...@chromium.org]

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.


https://codereview.chromium.org/2389473002/

[m...@chromium.org]

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.

https://codereview.chromium.org/2389473002/

[xhw...@chromium.org]

This definitely looks better!

Here are my first round of comments.


https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_observer.h
File media/base/mediaplayer_observer.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_observer.h#newcode5
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_observer.h#newcode16
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_observer.h#newcode21
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_observer.h#newcode25
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_observer.h#newcode28
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_metadata.h#newcode23
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_impl.cc
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_impl.cc#newcode171
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_impl.cc#newcode1104
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_impl.h
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_impl.h#newcode92
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_controller.cc
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_controller.cc#newcode114
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_controller.cc#newcode118
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_controller.cc#newcode131
media/remoting/remoting_controller.cc:131: return false;
ditto

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_controller.h
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_controller.h#newcode22
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_controller.h#newcode26
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_controller.h#newcode36
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_controller.h#newcode43
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_controller.h#newcode55
media/remoting/remoting_controller.h:55: bool isAudioConfigSupported();
nit: s/is/Is

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_controller.h#newcode59
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_controller.h#newcode62
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_controller.h#newcode80
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=ThreadChecker&sq=package:chromium&dr=CSs

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_renderer_factory.cc
File media/remoting/remoting_renderer_factory.cc (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_renderer_factory.cc#newcode29
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=1475263265&l=53
https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=1475263265&l=897

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_renderer_factory.h
File media/remoting/remoting_renderer_factory.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_renderer_factory.h#newcode14
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_renderer_factory.h#newcode19
media/remoting/remoting_renderer_factory.h:19: class VideoRendererSink;
ditto

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_renderer_factory.h#newcode24
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_renderer_factory.h#newcode40
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/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode202
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.

https://codereview.chromium.org/2389473002/

[xhw...@chromium.org]

BTW, I only skimmed media/remoting/* since I am not familiar with the logic
there.

https://codereview.chromium.org/2389473002/

[xhw...@chromium.org]

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.

https://chromiumcodereview.appspot.com/2389473002/

[xhw...@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?

https://chromiumcodereview.appspot.com/2389473002/

[x...@chromium.org]

Addressed xhwang's comments. PTAL

https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_observer.h
File media/base/mediaplayer_observer.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/base/mediaplayer_observer.h#newcode5
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_observer.h#newcode16
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_observer.h#newcode21
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_observer.h#newcode25
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_observer.h#newcode28
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_metadata.h#newcode23
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_impl.cc
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_impl.cc#newcode171
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_impl.cc#newcode1104
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_impl.h
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/blink/webmediaplayer_impl.h#newcode92
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_controller.cc
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_controller.cc#newcode114
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_controller.cc#newcode118
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_controller.cc#newcode131
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_controller.h
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_controller.h#newcode22
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_controller.h#newcode26
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_controller.h#newcode36
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_controller.h#newcode43
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_controller.h#newcode55
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_controller.h#newcode59
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_controller.h#newcode62
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_controller.h#newcode80
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=ThreadChecker&sq=package:chromium&dr=CSs

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_renderer_factory.cc
File media/remoting/remoting_renderer_factory.cc (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_renderer_factory.cc#newcode29
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=1475263265&l=53
>
https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=1475263265&l=897

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_renderer_factory.h
File media/remoting/remoting_renderer_factory.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_renderer_factory.h#newcode14
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_renderer_factory.h#newcode19
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_renderer_factory.h#newcode24
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_renderer_factory.h#newcode40
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/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode202
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.

https://codereview.chromium.org/2389473002/

[xhw...@chromium.org]

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_metadata.h#newcode23
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_controller.h
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_controller.h#newcode80
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=ThreadChecker&sq=package:chromium&dr=CSs
> 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/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode202
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_frame_impl.cc
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render_frame_impl.cc#newcode2676
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_frame_impl.cc#newcode2714
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_frame_impl.cc#newcode2714
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_observer.h
File media/base/media_observer.h (right):

https://codereview.chromium.org/2389473002/diff/40001/media/base/media_observer.h#newcode8
media/base/media_observer.h:8: #include "base/callback.h"
nit: not needed?

https://codereview.chromium.org/2389473002/diff/40001/media/base/pipeline_metadata.h
File media/base/pipeline_metadata.h (right):

https://codereview.chromium.org/2389473002/diff/40001/media/base/pipeline_metadata.h#newcode23

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/webmediaplayer_impl.cc
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/40001/media/blink/webmediaplayer_impl.cc#newcode1103
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_controller.cc
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting_controller.cc#newcode9
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_controller.cc#newcode22
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_controller.cc#newcode27
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_controller.cc#newcode123
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_controller.cc#newcode140
media/remoting/remoting_controller.cc:140: return true;
ditto

https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting_controller.cc#newcode179
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_controller.cc#newcode180
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_controller.cc#newcode180
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_controller.cc#newcode187
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_controller.h
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting_controller.h#newcode26
media/remoting/remoting_controller.h:26: mojom::RemoterPtr remoter);
no need for "explicit" now

https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting_controller.h#newcode37
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_controller.h#newcode48
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_controller.h#newcode89
media/remoting/remoting_controller.h:89: const
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
forward declare base::SingleThreadTaskRunner?

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

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_metadata.h#newcode23
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_controller.h
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2389473002/diff/1/media/remoting/remoting_controller.h#newcode80
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=ThreadChecker&sq=package:chromium&dr=CSs
> > 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/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2389473002/diff/1/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode202
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_frame_impl.cc
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render_frame_impl.cc#newcode2676
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_frame_impl.cc#newcode2714
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_frame_impl.cc#newcode2714
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_observer.h
File media/base/media_observer.h (right):

https://codereview.chromium.org/2389473002/diff/40001/media/base/media_observer.h#newcode8
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_metadata.h
File media/base/pipeline_metadata.h (right):

https://codereview.chromium.org/2389473002/diff/40001/media/base/pipeline_metadata.h#newcode23
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/webmediaplayer_impl.cc
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/40001/media/blink/webmediaplayer_impl.cc#newcode1103
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_controller.cc
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting_controller.cc#newcode9
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_controller.cc#newcode22
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_controller.cc#newcode27
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_controller.cc#newcode123
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_controller.cc#newcode140
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_controller.cc#newcode179
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_controller.cc#newcode180
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_controller.cc#newcode180
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_controller.cc#newcode187
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_controller.h
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2389473002/diff/40001/media/remoting/remoting_controller.h#newcode26
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_controller.h#newcode37
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_controller.h#newcode48
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_controller.h#newcode89
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.

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

Addressed xhwang's comments. PTAL

https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render_frame_impl.cc
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/40001/content/renderer/render_frame_impl.cc#newcode2714
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.

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

Add building flag to enable media remoting in PS#6. PTAL

https://codereview.chromium.org/2389473002/

[xhw...@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/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://chromiumcodereview.appspot.com/2389473002/diff/100001/content/renderer/BUILD.gn#newcode515
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:chromium&dr=CSs

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/renderer/render_frame_impl.h
File content/renderer/render_frame_impl.h (right):

https://chromiumcodereview.appspot.com/2389473002/diff/100001/content/renderer/render_frame_impl.h#newcode1053
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_options.gni
File media/media_options.gni (right):

https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/media_options.gni#newcode124
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/remoting_controller.cc
File media/remoting/remoting_controller.cc (right):

https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/remoting/remoting_controller.cc#newcode165
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/remoting_controller.h
File media/remoting/remoting_controller.h (right):

https://chromiumcodereview.appspot.com/2389473002/diff/100001/media/remoting/remoting_controller.h#newcode38
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.

https://chromiumcodereview.appspot.com/2389473002/

[x...@chromium.org]

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.gn#newcode515

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:chromium&dr=CSs
>
> 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/render_frame_impl.h
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2389473002/diff/100001/content/renderer/render_frame_impl.h#newcode1053

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.gni#newcode124

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/remoting_controller.cc
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remoting_controller.cc#newcode165
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/remoting_controller.h
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remoting_controller.h#newcode38

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

https://codereview.chromium.org/2389473002/

[xhw...@chromium.org]

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/remoting_controller.cc
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remoting_controller.cc#newcode165
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.gni#newcode124
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...

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

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.

https://codereview.chromium.org/2389473002/

[m...@chromium.org]

PS7 lgtm % nits:


https://codereview.chromium.org/2389473002/diff/120001/content/renderer/render_frame_impl.cc
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/120001/content/renderer/render_frame_impl.cc#newcode2725
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/webmediaplayer_impl.cc
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/120001/media/blink/webmediaplayer_impl.cc#newcode1103
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.gni#newcode124
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.

https://codereview.chromium.org/2389473002/

[sand...@chromium.org]

> 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.

https://codereview.chromium.org/2389473002/

[m...@chromium.org]

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. ;-)

https://codereview.chromium.org/2389473002/

[sand...@chromium.org]

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.



https://codereview.chromium.org/2389473002/

[x...@chromium.org]

Thanks miu@, xhwang@, and sandersd@! Addressed comments in PS8.

https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remoting_controller.cc
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2389473002/diff/100001/media/remoting/remoting_controller.cc#newcode165
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/render_frame_impl.cc
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/120001/content/renderer/render_frame_impl.cc#newcode2725
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/webmediaplayer_impl.cc
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2389473002/diff/120001/media/blink/webmediaplayer_impl.cc#newcode1103
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.gni#newcode124
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.gni#newcode124
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.

https://codereview.chromium.org/2389473002/

[esp...@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/Source/web/FrameLoaderClientImpl.cpp
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp#newcode795
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/Source/web/FullscreenController.cpp
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Source/web/FullscreenController.cpp#newcode92
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/Source/web/FullscreenController.cpp#newcode132
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?

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

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?

[dch...@chromium.org]

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/FullscreenController.cpp?rcl=1475882812&l=65
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)

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

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.

[dch...@chromium.org]

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/

[foo...@chromium.org]

https://codereview.chromium.org/2389473002/diff/160001/media/base/media_observer.h
File media/base/media_observer.h (right):

https://codereview.chromium.org/2389473002/diff/160001/media/base/media_observer.h#newcode19
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/Source/web/FullscreenController.cpp
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Source/web/FullscreenController.cpp#newcode92
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/

[foo...@chromium.org]

https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Source/web/FullscreenController.cpp
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Source/web/FullscreenController.cpp#newcode92
third_party/WebKit/Source/web/FullscreenController.cpp:92:
Traversal<HTMLVideoElement>::descendantsOf(*element)) {

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.

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

On 2016/10/10 08:35:22, foolip wrote:
>

https://codereview.chromium.org/2389473002/diff/160001/media/base/media_observer.h
> File media/base/media_observer.h (right):
>
>
https://codereview.chromium.org/2389473002/diff/160001/media/base/media_observer.h#newcode19
> 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/Source/web/FullscreenController.cpp
> File third_party/WebKit/Source/web/FullscreenController.cpp (right):
>
>
https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Source/web/FullscreenController.cpp#newcode92
> 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!


https://codereview.chromium.org/2389473002/

[x...@chromium.org]

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_observer.h
File media/base/media_observer.h (right):

https://codereview.chromium.org/2389473002/diff/160001/media/base/media_observer.h#newcode19
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/Source/web/FrameLoaderClientImpl.cpp
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp#newcode795
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/Source/web/FullscreenController.cpp
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/Source/web/FullscreenController.cpp#newcode92
third_party/WebKit/Source/web/FullscreenController.cpp:92:
Traversal<HTMLVideoElement>::descendantsOf(*element)) {

> 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/Source/web/FullscreenController.cpp#newcode132
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/public/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2389473002/diff/160001/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode232
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.

https://codereview.chromium.org/2389473002/

[xhw...@chromium.org]

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?

https://chromiumcodereview.appspot.com/2389473002/

[x...@chromium.org]

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!

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

esprehn@: need owner's approval on changes in content/*. PTAL Thanks!

https://codereview.chromium.org/2389473002/

[foo...@chromium.org]

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/

https://codereview.chromium.org/2389473002/

[esp...@chromium.org]

This lgtm to me, but the plumbing between blink isn't clear to me, how does that
work?

https://codereview.chromium.org/2389473002/

[esp...@chromium.org]

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.

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

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.


https://codereview.chromium.org/2389473002/

[x...@chromium.org]

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!



https://codereview.chromium.org/2389473002/

[esp...@chromium.org]

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.

https://codereview.chromium.org/2389473002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2389473002/200001


https://codereview.chromium.org/2389473002/

[commit-bot@chromium.org via codereview.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_presubmit/builds/278680)

https://codereview.chromium.org/2389473002/

[x...@chromium.org]

jam@: need approval for media/remoting/DEPS. Thanks!

https://codereview.chromium.org/2389473002/

[j...@chromium.org]

lgtm



https://codereview.chromium.org/2389473002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #11 (id:200001)

https://codereview.chromium.org/2389473002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 11 (id:??) landed as
https://crrev.com/d3fe45a5f7f8395c26b5f75f0574487eb7618639
Cr-Commit-Position: refs/heads/master@{#424791}

https://codereview.chromium.org/2389473002/