Register MediaPlayerRenderer service

Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is disabled by default, and enabled by a runtime flag.

To use the player, add the following flag to your command line.
"--enable-features=android-media-player-renderer"

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

Committed: https://crrev.com/25a4d110be98f53dd4f9608740a283deed34fb6b
Cr-Commit-Position: refs/heads/master@{#425176}

[tguilbert, 2016-10-06 01:46:43]

Description was changed from

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is protected by a compiler flag.

To use the player, add the following line to your GN args file:
enable_mojo_media_player_renderer = true

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

to

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is protected by a compiler flag.

To use the player, add the following line to your GN args file:
enable_mojo_media_player_renderer = true

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

[tguilbert, 2016-10-06 02:11:38]

tguilbert@chromium.org changed reviewers:
+ liberato@chromium.org, watk@chromium.org

[tguilbert, 2016-10-06 02:11:39]

PTAL! 

There are a few "For reviewers" scattered around.

I also know there is some code duplication in the
RenderFrameImpl::createMediaPlayer(), but it was *a lot* harder to read when
dealing with cascading #if #elif. I will attempt a cleanup of the function
whenever the MPR becomes the default.

Thanks!
Thomas

https://codereview.chromium.org/2387293007/diff/1/content/browser/media/andro...
File content/browser/media/android/media_player_renderer.cc (right):

https://codereview.chromium.org/2387293007/diff/1/content/browser/media/andro...
content/browser/media/android/media_player_renderer.cc:86: // enough data to
send a playback_rate > 0, and for HLS files to trigger a
I will re-word this comment.

https://codereview.chromium.org/2387293007/diff/1/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2387293007/diff/1/media/BUILD.gn#newcode21
media/BUILD.gn:21: "enable_mojo_media_player_renderer can only be true on
Android")
I will find a nicer wording.

[liberato (no reviews please), 2016-10-06 14:55:46]

haven't gone through in detail.  just had a few questions that jumped out at me.

thanks
-fl

https://codereview.chromium.org/2387293007/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2123: #if
BUILDFLAG(ENABLE_MOJO_MEDIA_PLAYER_RENDERER)
have you considered a runtime flag instead of (or in addition to) a compile-time
flag?  this will let us test with a finch experiment.  it will also let us get
results in the a/v lab for wmpa vs mediaplayerrenderer.

it's really easy with base::Feature.  see
https://cs.chromium.org/chromium/src/media/base/media_switches.h?rcl=0&l=89 .

https://codereview.chromium.org/2387293007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6210: WebMediaPlayer*
RenderFrameImpl::CreateMojoMediaPlayerRenderer(
lots of this is common to the normal wmpi case.  i think that it would be
clearer if one merges them, and just special-cases the renderer factory creation
plus one or two special calls (EnableFallback...).

actually, i think that one can probably make EnableFallback... not a special
case.

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1364: fallback_url_ = url;
what's the difference between this and the one used in DoLoad?

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.h:543: bool use_fallback_path_;
i don't think that this is initialized during construction anywhere.

[liberato (no reviews please), 2016-10-07 20:17:38]

more high-level stuff.  sorry that it's not all at one time.

-fl

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1230: if (use_fallback_path_)
if we aren't doing this on every natural size change normally, then i doubt what
we should do it here every time after the first.

in general, i'm worried that things like WatchTimeReporter::OnPlaying won't get
called in this case -- we don't get onNaturalSizeChanged from MP until after
play(), right?

WatchTimeReporter might need to learn how to change the size.  it looks like it
already keeps the appropriate state for "is playing", etc.  if so, then it might
be worth doing that as a separate CL, and just leave the failover path watch
time metrics broken (with comments) in this CL.

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1363: void
WebMediaPlayerImpl::EnableFallbackMediaPlayer(const GURL& url) {
this is very implementation-detail-like for an external caller.  can WMPI figure
this out internally, based on (e.g.) the url it's given?

[tguilbert, 2016-10-11 18:54:34]

Changed from a compile time flag to a switch.

FWIW, I never found how to "qualify" a switch and give it options. It seems like
it's just a binary value.

However, if you use the --enable-media-player-renderer flag and you disable the
unified pipeline, you will always end up using the new media player renderer.

https://codereview.chromium.org/2387293007/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2123: #if
BUILDFLAG(ENABLE_MOJO_MEDIA_PLAYER_RENDERER)
On 2016/10/06 14:55:46, liberato wrote:
> have you considered a runtime flag instead of (or in addition to) a
compile-time
> flag?  this will let us test with a finch experiment.  it will also let us get
> results in the a/v lab for wmpa vs mediaplayerrenderer.
> 
> it's really easy with base::Feature.  see
> https://cs.chromium.org/chromium/src/media/base/media_switches.h?rcl=0&l=89 .

Added.

https://codereview.chromium.org/2387293007/diff/1/content/browser/media/andro...
File content/browser/media/android/media_player_renderer.cc (right):

https://codereview.chromium.org/2387293007/diff/1/content/browser/media/andro...
content/browser/media/android/media_player_renderer.cc:86: // enough data to
send a playback_rate > 0, and for HLS files to trigger a
On 2016/10/06 02:11:39, ThomasGuilbert wrote:
> I will re-word this comment.

Done.

https://codereview.chromium.org/2387293007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6210: WebMediaPlayer*
RenderFrameImpl::CreateMojoMediaPlayerRenderer(
On 2016/10/06 14:55:46, liberato wrote:
> lots of this is common to the normal wmpi case.  i think that it would be
> clearer if one merges them, and just special-cases the renderer factory
creation
> plus one or two special calls (EnableFallback...).
> 
> actually, i think that one can probably make EnableFallback... not a special
> case.

I had found that, for the time being, it was clearer to duplicate everything
into a new function, until we remove WMPA. I'm fine re-merging everything, but
the initialization logic will be messy.

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1230: if (use_fallback_path_)
On 2016/10/07 20:17:38, liberato wrote:
> if we aren't doing this on every natural size change normally, then i doubt
what
> we should do it here every time after the first.
> 
> in general, i'm worried that things like WatchTimeReporter::OnPlaying won't
get
> called in this case -- we don't get onNaturalSizeChanged from MP until after
> play(), right?
> 
> WatchTimeReporter might need to learn how to change the size.  it looks like
it
> already keeps the appropriate state for "is playing", etc.  if so, then it
might
> be worth doing that as a separate CL, and just leave the failover path watch
> time metrics broken (with comments) in this CL.

Ok. I need to do an HLS related UMA cleanup anyways, I will also cover this when
doing so. See 650891

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1363: void
WebMediaPlayerImpl::EnableFallbackMediaPlayer(const GURL& url) {
On 2016/10/07 20:17:37, liberato wrote:
> this is very implementation-detail-like for an external caller.  can WMPI
figure
> this out internally, based on (e.g.) the url it's given?

Right now, as of this patch, |use_fallback_path_| is only used in StartPipeline.

We could accurately deduce it based off the the RendererFactory type, but that
is just a consequent of what UseWebMediaPlayerImpl() returns... I'm not sure how
I feel about adding a "prefered demuxer type" to the renderer factory interface.
We could also move the "UseWMPI/UseFallback" inside of WMPI; I don't know if
there any gotchas with the fact that the renderer factory is created outside of
WMPI, and the final redirected URL might change and not correspond to a type of
media that can be played with the renderer factory we were given.

WDYT?

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1364: fallback_url_ = url;
On 2016/10/06 14:55:46, liberato wrote:
> what's the difference between this and the one used in DoLoad?

None. Updated :)

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2387293007/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.h:543: bool use_fallback_path_;
On 2016/10/06 14:55:46, liberato wrote:
> i don't think that this is initialized during construction anywhere.

Done.

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

https://codereview.chromium.org/2387293007/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:52: #include "media/media_features.h"
Will remove.

[watk, 2016-10-11 23:49:35]

lgtm

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2194: if
(base::FeatureList::IsEnabled(media::kEnableMediaPlayerRenderer)) {
Feature should not have "enable" in the name for clarity.

https://codereview.chromium.org/2387293007/diff/20001/content/browser/media/a...
File content/browser/media/android/media_player_renderer.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/content/browser/media/a...
content/browser/media/android/media_player_renderer.cc:90: // NOTE:
OnBufferingUpdate is trigerred whenever the media has buffered or
triggered

https://codereview.chromium.org/2387293007/diff/20001/content/browser/media/a...
content/browser/media/android/media_player_renderer.cc:91: // played up to a %
value between 1-100, and it not a reliable indicator of
it's

[liberato (no reviews please), 2016-10-12 15:15:18]

lgtm.

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:111: // TODO(tguilbert):
For-reviewers -- Is there a convenient way to get rid of the
sorry, i don't.

don't forget to remove the comment :)

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

https://codereview.chromium.org/2387293007/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1500: // |is_streaming| and |is_static|?
false, false i think is fine.  i think that it matches what wmpa did.

[xhwang, 2016-10-12 20:14:15]

some drive-by comments/nits

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:231:
media::MojoRendererService::Create(nullptr, nullptr, nullptr,
add a comment about what those nullptrs are.

https://codereview.chromium.org/2387293007/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2724: } else {
Drive-by comment: Does it make sense to put this block under "#if
defined(OS_ANDROID)" so that we don't link in unnecessary classes on non-android
platforms? The compiler/linker might be smart enough to figure out
|use_fallback_path| is always false on non-android platforms, but I am not sure.

https://codereview.chromium.org/2387293007/diff/20001/media/base/media_switch...
File media/base/media_switches.h (right):

https://codereview.chromium.org/2387293007/diff/20001/media/base/media_switch...
media/base/media_switches.h:90: MEDIA_EXPORT extern const base::Feature
kEnableMediaPlayerRenderer;
android only?

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:830: done_cb.Run(PIPELINE_ERROR_COULD_NOT_RENDER);
Could you please add a DVLOG since it's a failure case?

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:836: // N.B: Empty GURL are not valid.
nit: N.B.?

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:839: return;
Could this happen in reality? Wondering whether we should dcheck.

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:839: return;
ditto for DVLOG

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:879: metadata.has_audio = true;
In https://chromiumcodereview.appspot.com/2389473002/, we also report
video/audio config here. And the assumption was that if has_audio is true, the
audio config is not null. Now for URL type this is not true, we should add
comments in PipelineMetadata about this.

https://codereview.chromium.org/2387293007/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_renderer_factory.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_renderer_factory.cc:33: if
(!get_gpu_factories_cb_.is_null()) {
Please add a comment when this could be null.

[tguilbert, 2016-10-12 21:46:23]

tguilbert@chromium.org changed reviewers:
+ nick@chromium.org

[tguilbert, 2016-10-12 21:46:25]

PTAL.

nick, can you OWNERS review content/*?

Thank you!
Thomas

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:111: // TODO(tguilbert):
For-reviewers -- Is there a convenient way to get rid of the
On 2016/10/12 15:15:18, liberato wrote:
> sorry, i don't.
> 
> don't forget to remove the comment :)

Done.

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:231:
media::MojoRendererService::Create(nullptr, nullptr, nullptr,
On 2016/10/12 20:14:14, xhwang wrote:
> add a comment about what those nullptrs are.

Done.

https://codereview.chromium.org/2387293007/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2194: if
(base::FeatureList::IsEnabled(media::kEnableMediaPlayerRenderer)) {
On 2016/10/11 23:49:35, watk wrote:
> Feature should not have "enable" in the name for clarity.

Done.

https://codereview.chromium.org/2387293007/diff/20001/content/browser/media/a...
File content/browser/media/android/media_player_renderer.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/content/browser/media/a...
content/browser/media/android/media_player_renderer.cc:90: // NOTE:
OnBufferingUpdate is trigerred whenever the media has buffered or
On 2016/10/11 23:49:35, watk wrote:
> triggered

Done.

https://codereview.chromium.org/2387293007/diff/20001/content/browser/media/a...
content/browser/media/android/media_player_renderer.cc:91: // played up to a %
value between 1-100, and it not a reliable indicator of
On 2016/10/11 23:49:35, watk wrote:
> it's

Done.

https://codereview.chromium.org/2387293007/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2724: } else {
On 2016/10/12 20:14:15, xhwang wrote:
> Drive-by comment: Does it make sense to put this block under "#if
> defined(OS_ANDROID)" so that we don't link in unnecessary classes on
non-android
> platforms? The compiler/linker might be smart enough to figure out
> |use_fallback_path| is always false on non-android platforms, but I am not
sure.

I think the compiler is smart enough, but I don't have the definitive data to
prove it. Adding the #if defined(OS_ANDROID) to make it extra clear (and avoid
problems if someone were to compile while explicitly disabling optimizations)

https://codereview.chromium.org/2387293007/diff/20001/media/base/media_switch...
File media/base/media_switches.h (right):

https://codereview.chromium.org/2387293007/diff/20001/media/base/media_switch...
media/base/media_switches.h:90: MEDIA_EXPORT extern const base::Feature
kEnableMediaPlayerRenderer;
On 2016/10/12 20:14:15, xhwang wrote:
> android only?

Done.

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:830: done_cb.Run(PIPELINE_ERROR_COULD_NOT_RENDER);
On 2016/10/12 20:14:15, xhwang wrote:
> Could you please add a DVLOG since it's a failure case?

Done.

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:836: // N.B: Empty GURL are not valid.
On 2016/10/12 20:14:15, xhwang wrote:
> nit: N.B.?

Woops...
N.B. is short for the latin "nota bene", which means "pay close attention". It's
common in french textbooks and legal documents. It seems like "NOTE:" is the way
to go in Chrome.

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:839: return;
On 2016/10/12 20:14:15, xhwang wrote:
> ditto for DVLOG

Done.

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:839: return;
On 2016/10/12 20:14:15, xhwang wrote:
> Could this happen in reality? Wondering whether we should dcheck.

I'm not sure TBH. Leaving it as is for the moment.

https://codereview.chromium.org/2387293007/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:879: metadata.has_audio = true;
On 2016/10/12 20:14:15, xhwang wrote:
> In https://chromiumcodereview.appspot.com/2389473002/, we also report
> video/audio config here. And the assumption was that if has_audio is true, the
> audio config is not null. Now for URL type this is not true, we should add
> comments in PipelineMetadata about this.

Done.

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

https://codereview.chromium.org/2387293007/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:52: #include "media/media_features.h"
On 2016/10/11 18:54:33, ThomasGuilbert wrote:
> Will remove.

Done.

https://codereview.chromium.org/2387293007/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1500: // |is_streaming| and |is_static|?
On 2016/10/12 15:15:18, liberato wrote:
> false, false i think is fine.  i think that it matches what wmpa did.

Done.

https://codereview.chromium.org/2387293007/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_renderer_factory.cc (right):

https://codereview.chromium.org/2387293007/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_renderer_factory.cc:33: if
(!get_gpu_factories_cb_.is_null()) {
On 2016/10/12 20:14:15, xhwang wrote:
> Please add a comment when this could be null.

Done.

[ncarter (slow), 2016-10-12 21:51:27]

The description references a 'compile flag', but it looks like you're using a
base::Feature, which is runtime-enabled. Could you update the description?

[tguilbert, 2016-10-12 22:57:31]

Description was changed from

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is protected by a compiler flag.

To use the player, add the following line to your GN args file:
enable_mojo_media_player_renderer = true

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

to

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is disabled by default, and enabled by a runtime flag.

To use the player, add the "--android-media-player-renderer"
flag to your command line parameters.

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

[tguilbert, 2016-10-12 22:58:01]

On 2016/10/12 21:51:27, ncarter wrote:
> The description references a 'compile flag', but it looks like you're using a
> base::Feature, which is runtime-enabled. Could you update the description?

Ah yes, thanks! Updated.

[tguilbert, 2016-10-12 23:29:32]

Description was changed from

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is disabled by default, and enabled by a runtime flag.

To use the player, add the "--android-media-player-renderer"
flag to your command line parameters.

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

to

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is disabled by default, and enabled by a runtime flag.

To use the player, add the following flag to your command line.
"--enable-features=android-media-player-renderer"

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

[tguilbert, 2016-10-13 01:30:00]

The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 01:30:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 01:33:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 01:33:37]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[tguilbert, 2016-10-13 01:37:23]

The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 01:37:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 02:43:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 02:43:27]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-10-13 17:01:18]

content lgtm

https://codereview.chromium.org/2387293007/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2387293007/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:118: #include
"media/mojo/services/mojo_renderer_service.h" //nogncheck
should be:

[space][space]//[space]nogncheck

[tguilbert, 2016-10-13 20:15:49]

The CQ bit was checked by tguilbert@chromium.org

[tguilbert, 2016-10-13 20:15:49]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
liberato@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2387293007/#ps100001
(title: "Typo.")

[commit-bot: I haz the power, 2016-10-13 20:16:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 21:57:17]

Description was changed from

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is disabled by default, and enabled by a runtime flag.

To use the player, add the following flag to your command line.
"--enable-features=android-media-player-renderer"

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

to

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is disabled by default, and enabled by a runtime flag.

To use the player, add the following flag to your command line.
"--enable-features=android-media-player-renderer"

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

[commit-bot: I haz the power, 2016-10-13 21:57:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-13 21:59:17]

Description was changed from

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is disabled by default, and enabled by a runtime flag.

To use the player, add the following flag to your command line.
"--enable-features=android-media-player-renderer"

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

to

==========
Register MediaPlayerRenderer service

This change adds the necessary code to allow the creation of a
MediaPlayerRenderer for HLS playback. The service's registration
is disabled by default, and enabled by a runtime flag.

To use the player, add the following flag to your command line.
"--enable-features=android-media-player-renderer"

The leftover issues before the MediaPlayerRenderer can be officially
shipped and before WMPA can be deleted are tracked by the issues
blocking crbug.com/619729.

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

Committed: https://crrev.com/25a4d110be98f53dd4f9608740a283deed34fb6b
Cr-Commit-Position: refs/heads/master@{#425176}
==========

[commit-bot: I haz the power, 2016-10-13 21:59:18]

Patchset 6 (id:??) landed as
https://crrev.com/25a4d110be98f53dd4f9608740a283deed34fb6b
Cr-Commit-Position: refs/heads/master@{#425176}