Add MediaPlayerRenderer/MediaPlayerRendererClient

content/browser/media/android/media_player_renderer.cc

Index: content/browser/media/android/media_player_renderer.cc
diff --git a/content/browser/media/android/media_player_renderer.cc b/content/browser/media/android/media_player_renderer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b396192758fe5d268267745e83d7b876773cc8a1
--- /dev/null
+++ b/content/browser/media/android/media_player_renderer.cc
@@ -0,0 +1,223 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/media/android/media_player_renderer.h"
+
+#include <memory>
+
+#include "content/browser/media/android/media_resource_getter_impl.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/storage_partition.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/content_client.h"
+
+// TODO(tguilbert): Remove this ID once MediaPlayerManager has been deleted
+// and MediaPlayerBridge updated. See comment in header file.
+const int kUnusedAndIrrelevantPlayerId = 0;

[DaleCurtis, 2016/08/10 21:14:05]

constexpr int..

[tguilbert, 2016/08/11 00:28:59]

Done.

+
+namespace content {
+
+MediaPlayerRenderer::MediaPlayerRenderer(RenderFrameHost* render_frame_host)
+    : render_frame_host_(render_frame_host),
+      duration_(base::TimeDelta::FromMicroseconds(0)),

[DaleCurtis, 2016/08/10 21:14:04]

Unnecessary, default is zero.

[tguilbert, 2016/08/11 00:28:59]

Done.

+      has_error_(false),
+      weak_factory_(this) {}
+
+MediaPlayerRenderer::~MediaPlayerRenderer() {}
+
+void MediaPlayerRenderer::Initialize(
+    media::DemuxerStreamProvider* demuxer_stream_provider,
+    media::RendererClient* client,
+    const media::PipelineStatusCB& init_cb) {
+  DVLOG(1) << __func__;
+  if (demuxer_stream_provider->GetType() !=
+      media::DemuxerStreamProvider::Type::URL) {
+    LOG(ERROR) << "DemuxerStreamProvider is not of Type URL";

[DaleCurtis, 2016/08/10 21:14:04]

Prefer DLOG?

[tguilbert, 2016/08/11 00:28:59]

Done.

+    init_cb.Run(media::PIPELINE_ERROR_INITIALIZATION_FAILED);
+    return;
+  }
+
+  GURL url = demuxer_stream_provider->GetUrl();
+  renderer_client_ = client;
+
+  const std::string user_agent = GetContentClient()->GetUserAgent();
+
+  // TODO(tguilbert): FOR REVIEWERS: I am not sure what each parameter below
+  // is used for. Are there any ones that I should be concerned with ATM? I
+  // have added the parameter name for convenience next to the ones I don't
+  // know what to do with.
+  media_player_.reset(new media::MediaPlayerBridge(
+      kUnusedAndIrrelevantPlayerId, url,
+      GURL(),  // first_party_for_cookies

[DaleCurtis, 2016/08/10 21:14:05]

I think this will be important to fill. How is WMPA filling this? +qinmin to
look over this. If you're not careful here, WMPA won't be able to load videos
which require some cookies / authentication.

[tguilbert, 2016/08/11 00:28:59]

WMPA gets it from the WebFrame.document().firstPartyForCookies(), with the
WebFrame being passed in by the contructor.

If I need to to pass that information from WMPI to the MediaPlayerRenderer, it
might be worth creating a new initialize method for mojom::Renderer, because, at
some point, I will need to pass
-- The URL that MediaPlayer will use to play.
-- The first party cookies URL.
-- Some token to retrieve the registered SurfaceTexture.

I opened 636604 to track this issue.

+      user_agent,
+      false,  // hide_url_log
+      this, base::Bind(&MediaPlayerRenderer::OnDecoderResourcesReleased,
+                       weak_factory_.GetWeakPtr()),
+      GURL(),  // frame_url
+      false,   // allow_crendentials
+      0));     // media_session_id
+
+  // TODO(tguilbert): Register and Send the proper surface ID. See
+  // crbug.com/627658
+
+  media_player_->Initialize();
+  init_cb.Run(media::PIPELINE_OK);
+}
+
+void MediaPlayerRenderer::SetCdm(media::CdmContext* cdm_context,
+                                 const media::CdmAttachedCB& cdm_attached_cb) {
+  NOTREACHED();
+}
+
+void MediaPlayerRenderer::Flush(const base::Closure& flush_cb) {
+  DVLOG(3) << __func__;
+  flush_cb.Run();
+}
+
+void MediaPlayerRenderer::StartPlayingFrom(base::TimeDelta time) {
+  // MediaPlayerBridge's Start() is idempotent, except when it has encountered
+  // an error (in which case, calling Start() again is logged as a new error).
+  if (has_error_)
+    return;
+
+  media_player_->Start();
+  media_player_->SeekTo(time);

[DaleCurtis, 2016/08/10 21:14:05]

Should SeekTo happen first?

[tguilbert, 2016/08/11 00:28:59]

Either one works. Calling Start() first causes the MediaPlayer to start to
"prepare" first, and it will automatically seek to the last given |time| when it
is done preparing.

+}
+
+void MediaPlayerRenderer::SetPlaybackRate(double playback_rate) {

[DaleCurtis, 2016/08/10 21:14:04]

Needs a comment about what's happening here. You're not using the rate for
anything other than "Stopped" or "started"

[tguilbert, 2016/08/11 00:28:59]

Good point. I added a comment in the header file.

MediaPlayerBridge does not expose MediaPlayer's SetPlaybackParams() method, but
we could add support for variable playback rate in the future.

+  if (has_error_)
+    return;
+
+  if (playback_rate == 0) {
+    media_player_->Pause(true);
+  } else {
+    // MediaPlayerBridge's Start() is idempotent.
+    media_player_->Start();
+  }
+}
+
+void MediaPlayerRenderer::SetVolume(float volume) {
+  media_player_->SetVolume(volume);
+}
+
+base::TimeDelta MediaPlayerRenderer::GetMediaTime() {
+  return media_player_->GetCurrentTime();
+}
+
+bool MediaPlayerRenderer::HasAudio() {
+  return media_player_->HasAudio();
+}
+
+bool MediaPlayerRenderer::HasVideo() {
+  return media_player_->HasVideo();
+}
+
+media::MediaResourceGetter* MediaPlayerRenderer::GetMediaResourceGetter() {
+  if (!media_resource_getter_.get()) {
+    WebContents* web_contents =

[DaleCurtis, 2016/08/10 21:14:04]

This can only be done on the UI thread IIRC. Do you want to add some
DCHECK(BrowserThread::CurrentlyOn()); calls?

[tguilbert, 2016/08/11 00:28:59]

Added, but I don't know where else I should add this call.

+        WebContents::FromRenderFrameHost(render_frame_host_);
+    RenderProcessHost* host = web_contents->GetRenderProcessHost();
+    BrowserContext* context = host->GetBrowserContext();
+    StoragePartition* partition = host->GetStoragePartition();
+    storage::FileSystemContext* file_system_context =
+        partition ? partition->GetFileSystemContext() : nullptr;
+    media_resource_getter_.reset(
+        new MediaResourceGetterImpl(context, file_system_context, host->GetID(),
+                                    render_frame_host_->GetRoutingID()));
+  }
+  return media_resource_getter_.get();
+}
+
+media::MediaUrlInterceptor* MediaPlayerRenderer::GetMediaUrlInterceptor() {
+  return nullptr;

[DaleCurtis, 2016/08/10 21:14:04]

TODO plumb this through? Or is this overridden at a higher level?

[tguilbert, 2016/08/11 00:28:59]

Opened 636588.

+}
+
+void MediaPlayerRenderer::OnTimeUpdate(int player_id,
+                                       base::TimeDelta current_timestamp,
+                                       base::TimeTicks current_time_ticks) {}
+
+void MediaPlayerRenderer::OnMediaMetadataChanged(int player_id,
+                                                 base::TimeDelta duration,
+                                                 int width,
+                                                 int height,
+                                                 bool success) {
+  if (video_size_ != gfx::Size(width, height))
+    OnVideoSizeChanged(kUnusedAndIrrelevantPlayerId, width, height);
+
+  if (duration_ != duration) {
+    duration_ = duration;
+    // TODO(tguilbert): Add OnDurationChange to the RendererClient interface.

[DaleCurtis, 2016/08/10 21:14:05]

Probably this will be important. I think MP gets the duration wrong a lot.

[tguilbert, 2016/08/11 00:28:59]

The CL is already done, I just took it out of this review to keep this review
simpler.

+    // See crbug.com/635991.
+  }
+}
+
+void MediaPlayerRenderer::OnPlaybackComplete(int player_id) {
+  renderer_client_->OnEnded();
+}
+
+void MediaPlayerRenderer::OnMediaInterrupted(int player_id) {}
+
+void MediaPlayerRenderer::OnBufferingUpdate(int player_id, int percentage) {
+  // As per Android documentation, |percentage| actually indicates "percentage
+  // buffered or played". E.g. if we are at 50% playback and have 1%
+  // buffered, |percentage| will be equal to 51.
+  //
+  // MediaPlayer manages its own buffering and will pause internally if ever it
+  // runs out of data. Therefore, we can always return BUFFERING_HAVE_ENOUGH.
+  renderer_client_->OnBufferingStateChange(media::BUFFERING_HAVE_ENOUGH);
+}
+
+void MediaPlayerRenderer::OnSeekComplete(int player_id,
+                                         const base::TimeDelta& current_time) {}
+
+void MediaPlayerRenderer::OnError(int player_id, int error) {
+  DVLOG(2) << __func__ << " Error: " << error;

[DaleCurtis, 2016/08/10 21:14:05]

I'd make this a LOG(ERROR) for finding issues in the wild.

[DaleCurtis, 2016/08/10 21:14:05]

I'd make this a LOG(ERROR) for finding issues in the wild.

[tguilbert, 2016/08/11 00:28:59]

Changed this to early return, to prevent the non-errors from being logged.

+  // TODO(tguilbert): Use more detailed errors if needed.
+
+  // Some errors are forwarded to the MediaPlayerListener, but are of no
+  // importance to us. Ignore these errors, which are reported as error 0 by
+  // MediaPlayerListener.
+  if (error) {
+    has_error_ = true;
+    renderer_client_->OnError(media::PIPELINE_ERROR_COULD_NOT_RENDER);

[DaleCurtis, 2016/08/10 21:14:04]

We may want a special error for external renderers here. I.e.
PIPELINE_ERROR_EXTERNAL_RENDERER_FAILED.

[tguilbert, 2016/08/11 00:28:59]

Done. Is there anything that needs updating UMA?

+  }
+}
+
+void MediaPlayerRenderer::OnVideoSizeChanged(int player_id,
+                                             int width,
+                                             int height) {
+  video_size_ = gfx::Size(width, height);
+  renderer_client_->OnVideoNaturalSizeChange(video_size_);
+}
+
+void MediaPlayerRenderer::OnWaitingForDecryptionKey(int player_id) {
+  NOTREACHED();
+}
+
+media::MediaPlayerAndroid* MediaPlayerRenderer::GetFullscreenPlayer() {
+  NOTREACHED();
+  return nullptr;
+}
+
+media::MediaPlayerAndroid* MediaPlayerRenderer::GetPlayer(int player_id) {
+  NOTREACHED();
+  return nullptr;
+}
+
+bool MediaPlayerRenderer::RequestPlay(int player_id,
+                                      base::TimeDelta duration,
+                                      bool has_audio) {
+  return true;

[DaleCurtis, 2016/08/10 21:14:04]

May need to carry over some logic from WMPA/etc to make sure this doesn't allow
DoS of mediaserver.

[tguilbert, 2016/08/11 00:28:59]

I am not familiar with this area of the code at all. I will ask for a crash
course from you tomorrow, and have created 636615 in the meantime, to address
the issue in a different CL.

+}
+
+void MediaPlayerRenderer::OnDecoderResourcesReleased(int player_id) {
+  // Since we are not using a pool of MediaPlayerAndroid instances, this
+  // function is not relevant.
+
+  // TODO(tguilbert): Investigate whether the use of the MediaThrottler is

[DaleCurtis, 2016/08/10 21:14:05]

I think you can instead use an exponential back off scheme instead.

[tguilbert, 2016/08/11 00:28:59]

Created 636615 and added TODO.

+  // needed.
+}
+
+}  // namespace content