[DO NOT COMMIT] Initial MediaPlayerRenderer plumbing and basic features

[DO NOT COMMIT] Initial MediaPlayerRenderer plumbing and basic features

UPDATE: The relevant code is mostly checked in. Closing this issue. See the bugs tied to https://bugs.chromium.org/p/chromium/issues/detail?id=619729 in order to get the full picture of the code that landed.

This CL is a work in progress and not ready for commiting.

This change introduces the overall plumbing for the MediaPlayerRenderer.

Before this change can be commited, the depcheck violations need to be
fixed (MediaPlayerRenderer needs to be moved to a different location).

Here are some issues that are on my mind, but that might not be
addressed in this CL:
  - We request the SurfaceView in the Renderer process, which then
  forwards this request to the Browser process. This should be updated
  so we request the SurfaceView from the browser directly.
  - MediaPlayerRenderer currently forwards all requests directly to
  the MediaPlayerBridge, without really caring for it's state. This
  causes seek to fail often.
  - Demuxers are normally responsible for notifying the pipeline of a
  duration change. The URL demuxer does not have duration information.
  I have added the OnDurationChanged method to the RendererClient
  interface. I'd rather not muddy that interface, suggestions are very
  welcome.
  - The player only supports fullscreen, but does not immediately
  transition into fullscreen on playback start.
  - There are blue frames when transitioning in/out of fullscreen.
  - The start time is not maintained between entering/exiting
  fullscreen.
  - A lot of the classes have many TODOs. I expect to spend a lot of
  time addressing the ones in MediaPlayerRenderer after this CL, so
  do not worry too much about the functional part of that class, unless
  therer is a big red flag somewhere.

I am looking for all types of feedback, and all comments are very
appreciated.

In order to build this, setting 'target_os = "android"', 'is_debug =
true' and 'force_mojo_media_player_renderer = true' should be enough,
but I have mostly tested code when 'enable_mojo_media = true' was also
set.

BUG=619729
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[tguilbert, 2016-06-17 22:22:28]

Description was changed from

==========
[DO NOT COMMIT] Initial MediaPlayerRenderer plumbing and basic features

This CL is a work in progress and not ready for commiting.

This change introduces the overall plumbing for the MediaPlayerRenderer.

Before this change can be commited, the depcheck violations need to be
fixed (MediaPlayerRenderer needs to be moved to a different location).

Here are some issues that are on my mind, but that might not be
addressed in this CL:
  - We request the SurfaceView in the Renderer process, which then
  forwards this request to the Browser process. This should be updated
  so we request the SurfaceView from the browser directly.
  - MediaPlayerRenderer currently forwards all requests directly to
  the MediaPlayerBridge, without really caring for it's state. This
  causes seek to fail often.
  - Demuxers are normally responsible for notifying the pipeline of a
  duration change. The URL demuxer does not have duration information.
  I have added the OnDurationChanged method to the RendererClient
  interface. I'd rather not muddy that interface, suggestions are very
  welcome.
  - The player only supports fullscreen, but does not immediately
  transition into fullscreen on playback start.
  - There are blue frames when transitioning in/out of fullscreen.
  - The start time is not maintained between entering/exiting
  fullscreen.
  - A lot of the classes have many TODOs. I expect to spend a lot of
  time addressing the ones in MediaPlayerRenderer after this CL, so
  do not worry too much about the functional part of that class, unless
  therer is a big red flag somewhere.

I am looking for all types of feedback, and all comments are very
appreciated.

In order to build this, setting 'target_os = "android"', 'is_debug =
true' and 'force_mojo_media_player_renderer = true' should be enough,
but I have mostly tested code when 'enable_mojo_media = true' was also
set.

BUG=619729
==========

to

==========
[DO NOT COMMIT] Initial MediaPlayerRenderer plumbing and basic features

This CL is a work in progress and not ready for commiting.

This change introduces the overall plumbing for the MediaPlayerRenderer.

Before this change can be commited, the depcheck violations need to be
fixed (MediaPlayerRenderer needs to be moved to a different location).

Here are some issues that are on my mind, but that might not be
addressed in this CL:
  - We request the SurfaceView in the Renderer process, which then
  forwards this request to the Browser process. This should be updated
  so we request the SurfaceView from the browser directly.
  - MediaPlayerRenderer currently forwards all requests directly to
  the MediaPlayerBridge, without really caring for it's state. This
  causes seek to fail often.
  - Demuxers are normally responsible for notifying the pipeline of a
  duration change. The URL demuxer does not have duration information.
  I have added the OnDurationChanged method to the RendererClient
  interface. I'd rather not muddy that interface, suggestions are very
  welcome.
  - The player only supports fullscreen, but does not immediately
  transition into fullscreen on playback start.
  - There are blue frames when transitioning in/out of fullscreen.
  - The start time is not maintained between entering/exiting
  fullscreen.
  - A lot of the classes have many TODOs. I expect to spend a lot of
  time addressing the ones in MediaPlayerRenderer after this CL, so
  do not worry too much about the functional part of that class, unless
  therer is a big red flag somewhere.

I am looking for all types of feedback, and all comments are very
appreciated.

In order to build this, setting 'target_os = "android"', 'is_debug =
true' and 'force_mojo_media_player_renderer = true' should be enough,
but I have mostly tested code when 'enable_mojo_media = true' was also
set.

BUG=619729
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[tguilbert, 2016-06-17 22:41:32]

tguilbert@chromium.org changed reviewers:
+ dalecurtis@chromium.org, liberato@chromium.org, sandersd@chromium.org,
xhwang@chromium.org

[tguilbert, 2016-06-17 22:41:33]

PTAL :)

I will gladly answer heftier questions in person or via hangouts.

Thanks!
Thomas

[DaleCurtis, 2016-06-17 22:43:26]

Haven't looked over it yet. This is fine for getting the gist of the final
product, but when it comes time to landing this, you'll need to split it up into
logical pieces that can be landed in well unittested chunks. So keep that in
mind while you iterate on this CL. Can't wait to see the final product :)

[xhwang, 2016-06-21 17:21:34]

Sorry for the delay. I'll take a highlevel look at this later today.

[xhwang, 2016-06-22 17:39:03]

Overall this looks pretty good. I didn't review details in the new
MediaPlayerRenderer though. Here are my high-level comments.

https://chromiumcodereview.appspot.com/2075303002/diff/40001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:88: #include
"media/media_features.h"
this is fine. content/ already depends on media/

https://chromiumcodereview.appspot.com/2075303002/diff/40001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:2066:
&media::MojoMediaPlayerRendererHelper::CreateMediaPlayerRenderer,
You don't really need a class to do this. You can probably just define a
CreateMediaPlayerRenderer() function, in this file, or in a new media/ file.

https://chromiumcodereview.appspot.com/2075303002/diff/40001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/content/renderer...
content/renderer/render_frame_impl.cc:2620: new
media::MediaPlayerRendererFactory(
ServiceRegistryImpl() is actually a shell::mojom::InterfaceProvider, the same
type as GetMediaInterfaceProvider(). So I feel you can just reuse
media::MojoRendererFactory here, but use a different
shell::mojom::InterfaceProvider (i.e. GetRemoteInterfaces().get()).

https://chromiumcodereview.appspot.com/2075303002/diff/40001/content/renderer...
content/renderer/render_frame_impl.cc:2632: GURL(url));
Do you need |url| in both MojoRendererFactory and MediaPlayerRendererFactory?

Actually why do we need the URL in any of them? I though we are getting the URL
from the new UrlDemuxerStreamProvider?

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/base/andro...
File media/base/android/media_player_renderer.h (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/base/andro...
media/base/android/media_player_renderer.h:55: const PipelineStatusCB& init_cb)
override;
nit: drop "virtual" in overrides

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/base/andro...
File media/base/android/url_demuxer_stream.h (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/base/andro...
media/base/android/url_demuxer_stream.h:18: class UrlDemuxerStream : public
DemuxerStream {
I don't feel we should have a UrlDemuxerStream. The way we use it is totally
different from a DemuxerStream, so it doesn't pass the "UrlDemuxerStream is a
DemuxerStream" test. The "NOTREACHED" in the .cc file demonstrated this as well.

Conceptually, the UrlDemuxerStreamProvider doesn't even need to be a Demuxer
either. For example, Seek() on UrlDemuxerStreamProvider doesn't make sense. But
I see that currently WMPI and Pipeline* is deeply integrated with Demuxer.
Changing that would require a much larger refactoring. So I think
UrlDemuxerStreamProvider is fine for now. In the future we can iterate on this.

With that, I wonder whether we can:

1, In DemuxerStreamProvider, in addition to GetStream(), add another method like
GetUrl(). The contract will be something like a DemuxerStreamProvider should
return non-null for GetStream() (audio and/or video), or non-empty URL for
GetUrl(), but not both.

2. Since UrlDemuxerStreamProvider is a Demuxer which is a DemuxerStreamProvider,
this should be sufficient for the Renderer to retrieve the URL.

3. With this, I feel DemuxerStreamProvider should be renamed. It's really a
"MediaSource" now, but unfortunately that name has been taken. Maybe
MediaProvider? Or anything you propose :)

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/base/rende...
File media/base/renderer_client.h (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/base/rende...
media/base/renderer_client.h:43: virtual void OnDurationChange(base::TimeDelta
duration) = 0;
As discussed offline, this change sgtm :) Now we have two sources of duration
(from Renderer and from Demuxer). Somewhere we need to document about what the
expectation is. For example, we shouldn't get duration change from both sources.

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/clien...
File media/mojo/clients/mojo_renderer_factory.cc (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/clien...
media/mojo/clients/mojo_renderer_factory.cc:47: renderer->SetUrl(url_);
Now I see why you have |url| in MojoRendererFactory's ctor :) But this seems
unnecessary. In MojoRendererImpl::Initialize() you can get the URL from the
|demuxer_stream_provider_|.

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/inter...
File media/mojo/interfaces/renderer.mojom (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/inter...
media/mojo/interfaces/renderer.mojom:17: string? url,
In the future we may want to wrap audio, video and url into a mojo
MediaProvider. Can you add a TODO here?

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/inter...
media/mojo/interfaces/renderer.mojom:18: int64 surface_id) => (bool success);
In C++, currently we pass the |request_surface_cb| at construction time (in
RendererFactory::CreateRenderer). When using MojoRenderer in
MojoMediaApplication, we can add construction time parameters in
ServiceFactory::CreateRenderer()

https://chromiumcodereview.appspot.com/2016053003/

In your case, you are not using ServiceFactory, and GetInterface() won't allow
you to pass any additional parameters :( I think sandersd@ hit this problem as
well when working on mojo VideoDeocder. The solution there is to add a
Construct() call:

https://cs.chromium.org/chromium/src/media/mojo/interfaces/video_decoder.mojo...

I don't have a good solution here. You current change is okay, but please add a
TODO so we can revisit this later.

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/servi...
File media/mojo/services/mojo_renderer_service.cc (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/servi...
media/mojo/services/mojo_renderer_service.cc:54: // TODO(tguilbert): For review:
are c-style casts acceptable?
no :) use static_cast instead

[xhwang, 2016-06-22 17:43:09]

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/inter...
File media/mojo/interfaces/renderer.mojom (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/inter...
media/mojo/interfaces/renderer.mojom:18: int64 surface_id) => (bool success);
On 2016/06/22 17:39:02, xhwang wrote:
> In C++, currently we pass the |request_surface_cb| at construction time (in
> RendererFactory::CreateRenderer). When using MojoRenderer in
> MojoMediaApplication, we can add construction time parameters in
> ServiceFactory::CreateRenderer()
> 
> https://chromiumcodereview.appspot.com/2016053003/
> 
> In your case, you are not using ServiceFactory, and GetInterface() won't allow
> you to pass any additional parameters :( I think sandersd@ hit this problem as
> well when working on mojo VideoDeocder. The solution there is to add a
> Construct() call:
> 
>
https://cs.chromium.org/chromium/src/media/mojo/interfaces/video_decoder.mojo...
> 
> I don't have a good solution here. You current change is okay, but please add
a
> TODO so we can revisit this later.

For more background, we try to keep the mojo part a pure transport layer, so
mojom::Renderer::Initialize() matches Renderer::Initialize(). But as discussed
above this causes some headache. So maybe we should move all construction time
and initialization time parameters to mojom::Renderer::Initialize(), to make
things simple. On platforms that don't care about some of the parameters they
can just ignore them.

+sandersd: WDYT?

[sandersd (OOO until July 31), 2016-06-23 18:43:35]

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/inter...
File media/mojo/interfaces/renderer.mojom (right):

https://chromiumcodereview.appspot.com/2075303002/diff/40001/media/mojo/inter...
media/mojo/interfaces/renderer.mojom:18: int64 surface_id) => (bool success);
The last time we talked, we decided against parameters for CreateVideoDecoder()
this because we anticipated a future where we only had GetInterface()-style
access. There was no actual technical restriction preventing us from doing so at
the time.

In general, there should not be any classes for which Initialize() is called
more than once. I don't see any problem with passing "constructor" parameters
along with "initialize" parameters to one method named Initialize(). If there
are many constructor parameters, they should go into a struct.

I agree that the rest of the parameters should match media:: interfaces as
closely as possible, and that it's fine to ignore some of those parameters.

[liberato (no reviews please), 2016-06-23 21:31:39]

https://codereview.chromium.org/2075303002/diff/40001/media/base/android/medi...
File media/base/android/media_player_renderer.cc (right):

https://codereview.chromium.org/2075303002/diff/40001/media/base/android/medi...
media/base/android/media_player_renderer.cc:51:
media_player_->SetVideoSurface(content::GetViewSurface(surface_id_));
i think that using SV initially during development was a very good idea. 
unfortunately, switching between SV and ST might be somewhat harder.

if you intend to include ST support in v1, then you might want to consider
transitioning this CL to use SurfaceTexture exclusively.  it could avoid a lot
of edge cases.  such ideas arrive thanks to a morning conversation with sandersd
and watk :)

we can add SV support later, if usage stats are high enough.

https://codereview.chromium.org/2075303002/diff/40001/media/base/android/medi...
File media/base/android/media_player_renderer.h (right):

https://codereview.chromium.org/2075303002/diff/40001/media/base/android/medi...
media/base/android/media_player_renderer.h:100: int surface_id_;
i don't see any calls to SetSurfaceId.  is your plan that WMPI will call it?

also, |surface_id_| is left uninitialized after construction.

[Julien Isorce Samsung, 2016-08-08 13:01:58]

https://codereview.chromium.org/2075303002/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2075303002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1303: return;
Hi, not sure what CL is the most up to date with
https://codereview.chromium.org/2161083004/diff/1/media/blink/webmediaplayer_...,
just in case I am duplicating the question here:

Hi, sorry for the short introduction, I was thinking of creating a new
MojoPipelineService but then I saw your solution that re-use MojoRenderer + this
pseudo demuxer that just passes the url. You said that creating a proper service
would have required too much unnecessary refactoring "Removing the Demuxer from
the pipeline when using a MediaPlayer would require a non-trivial amount of
refactoring, that might not be worth it." So I was wondering if after all this
work you are still thinking that there is no need for a proper
MojoPipelineService ? Nice solution btw !

[tguilbert, 2016-09-26 23:31:31]

Description was changed from

==========
[DO NOT COMMIT] Initial MediaPlayerRenderer plumbing and basic features

This CL is a work in progress and not ready for commiting.

This change introduces the overall plumbing for the MediaPlayerRenderer.

Before this change can be commited, the depcheck violations need to be
fixed (MediaPlayerRenderer needs to be moved to a different location).

Here are some issues that are on my mind, but that might not be
addressed in this CL:
  - We request the SurfaceView in the Renderer process, which then
  forwards this request to the Browser process. This should be updated
  so we request the SurfaceView from the browser directly.
  - MediaPlayerRenderer currently forwards all requests directly to
  the MediaPlayerBridge, without really caring for it's state. This
  causes seek to fail often.
  - Demuxers are normally responsible for notifying the pipeline of a
  duration change. The URL demuxer does not have duration information.
  I have added the OnDurationChanged method to the RendererClient
  interface. I'd rather not muddy that interface, suggestions are very
  welcome.
  - The player only supports fullscreen, but does not immediately
  transition into fullscreen on playback start.
  - There are blue frames when transitioning in/out of fullscreen.
  - The start time is not maintained between entering/exiting
  fullscreen.
  - A lot of the classes have many TODOs. I expect to spend a lot of
  time addressing the ones in MediaPlayerRenderer after this CL, so
  do not worry too much about the functional part of that class, unless
  therer is a big red flag somewhere.

I am looking for all types of feedback, and all comments are very
appreciated.

In order to build this, setting 'target_os = "android"', 'is_debug =
true' and 'force_mojo_media_player_renderer = true' should be enough,
but I have mostly tested code when 'enable_mojo_media = true' was also
set.

BUG=619729
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[DO NOT COMMIT] Initial MediaPlayerRenderer plumbing and basic features

UPDATE: The relevant code is mostly checked in. Closing this issue. See the bugs
tied to https://bugs.chromium.org/p/chromium/issues/detail?id=619729 in order to
get the full picture of the code that landed.

This CL is a work in progress and not ready for commiting.

This change introduces the overall plumbing for the MediaPlayerRenderer.

Before this change can be commited, the depcheck violations need to be
fixed (MediaPlayerRenderer needs to be moved to a different location).

Here are some issues that are on my mind, but that might not be
addressed in this CL:
  - We request the SurfaceView in the Renderer process, which then
  forwards this request to the Browser process. This should be updated
  so we request the SurfaceView from the browser directly.
  - MediaPlayerRenderer currently forwards all requests directly to
  the MediaPlayerBridge, without really caring for it's state. This
  causes seek to fail often.
  - Demuxers are normally responsible for notifying the pipeline of a
  duration change. The URL demuxer does not have duration information.
  I have added the OnDurationChanged method to the RendererClient
  interface. I'd rather not muddy that interface, suggestions are very
  welcome.
  - The player only supports fullscreen, but does not immediately
  transition into fullscreen on playback start.
  - There are blue frames when transitioning in/out of fullscreen.
  - The start time is not maintained between entering/exiting
  fullscreen.
  - A lot of the classes have many TODOs. I expect to spend a lot of
  time addressing the ones in MediaPlayerRenderer after this CL, so
  do not worry too much about the functional part of that class, unless
  therer is a big red flag somewhere.

I am looking for all types of feedback, and all comments are very
appreciated.

In order to build this, setting 'target_os = "android"', 'is_debug =
true' and 'force_mojo_media_player_renderer = true' should be enough,
but I have mostly tested code when 'enable_mojo_media = true' was also
set.

BUG=619729
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========