Replaces VideoLayer with a SurfaceLayer in WebMediaPlayerImpl

media/blink/webmediaplayer_impl.cc

Index: media/blink/webmediaplayer_impl.cc
diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
index 52ee0e8e7cb426264f7b91c0169f9e6e99283dd1..0e19c536b7d456a0146e6728a891e686f0c89672 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -61,6 +61,7 @@
 #include "third_party/WebKit/public/platform/WebSize.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
+#include "third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
@@ -251,6 +252,8 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
       is_encrypted_(false),
       preroll_attempt_pending_(false),
       observer_(params->media_observer()),
+      bridge_(
+          base::WrapUnique(blink::WebVideoSurfaceLayerBridge::Create(this))),

[liberato (no reviews please), 2017/06/26 22:39:30]

maybe only do this if |kVideoSurface|?

[CJ, 2017/06/29 19:51:33]

Done.

       max_keyframe_distance_to_disable_background_video_(
           params->max_keyframe_distance_to_disable_background_video()),
       max_keyframe_distance_to_disable_background_video_mse_(
@@ -323,7 +326,7 @@ WebMediaPlayerImpl::~WebMediaPlayerImpl() {
 
   // Destruct compositor resources in the proper order.
   client_->SetWebLayer(nullptr);
-  if (video_weblayer_)
+  if (video_weblayer_ && !base::FeatureList::IsEnabled(kVideoSurface))
     static_cast<cc::VideoLayer*>(video_weblayer_->layer())->StopUsingProvider();
   compositor_task_runner_->DeleteSoon(FROM_HERE, compositor_);
 
@@ -1349,8 +1352,12 @@ void WebMediaPlayerImpl::OnMetadata(PipelineMetadata metadata) {
     }
 
     DCHECK(!video_weblayer_);
-    video_weblayer_.reset(new cc_blink::WebLayerImpl(cc::VideoLayer::Create(
-        compositor_, pipeline_metadata_.video_rotation)));
+    if (base::FeatureList::IsEnabled(media::kVideoSurface)) {
+      video_weblayer_.reset(new cc_blink::WebLayerImpl(bridge_->GetLayer()));

[liberato (no reviews please), 2017/06/26 15:04:03]

for your SurfaceLayer ownership question, i'd expect that we'd want a similar
ownership model here as we have now: WMPI would create and own it.

wmpi probably shouldn't need to call back into blink to create the layer, then
immediately wrap it back up and hand it back to blink in a WebLayerImpl.  i'm
not sure that one needs |bridge_| at all.

[enne (OOO), 2017/06/26 18:35:58]

Yes, I think the same ownership model should apply for the surface layers that
applies for the video layers now.
It seems like this code would want to get used by both WebMediaPlayerImpl and
WebMediaPlayerMS, which both create cc::VideoLayer and interact with
WebMediaPlayerClient.  I could imagine a helper class in content/renderer/media
that does this bridge code that both players own.  Maybe it could own or have a
pointer to the surface layer and update the surface id when that changed.  I'm
not really an expert in this code though, so take that thought with a grain of
salt.

[CJ, 2017/06/29 19:51:33]

So, to summarize, we want to pull out all SurfaceLayer creation/modification out
of bridge, and have that owned by a helper class that updates itself via bridge
callbacks.

We don't have to anything to the WebLayer when we update the Surfacelayer?

[enne (OOO), 2017/06/29 20:29:29]

I was suggesting that you don't need a bridge at all, or the bridge and the
helper become the same thing.  Said differently, WebMediaPlayerImpl/MS own
VideoSurfaceHelper.  VideoSurfaceHelper creates and owns the cc::SurfaceLayer
and handles the surface callbacks that the bridge does.  WebMediaPlayer can
access the cc::SurfaceLayer off of the helper and call SetWebLayer when
applicable.

Just trying to clarify what I was suggesting.  I was trying to elaborate a
thought off of liberato's suggestion of not having the bridge live in Blink.
If the WebLayer hasn't changed to a different cc::Layer, then there shouldn't be
anything to do.  I suspect CanvasSurfaceLayerBridge calls OnWebLayerReplaced in
too many cases.  It should only need it when the web_layer_ changes and not when
values change on the cc::SurfaceLayer.

[CJ, 2017/06/29 20:46:06]

So, is this just a naming issue? (Sorry if I keep missing the point.) I feel
like what this helper would be doing is what the bridge is already doing. I am
confused.

Okay cool, that means we would not need the WebSurfaceLayerBridgeObserver class?

+    } else {
+      video_weblayer_.reset(new cc_blink::WebLayerImpl(cc::VideoLayer::Create(
+          compositor_, pipeline_metadata_.video_rotation)));
+    }
     video_weblayer_->layer()->SetContentsOpaque(opaque_);
     video_weblayer_->SetContentsOpaqueIsFixed(true);
     client_->SetWebLayer(video_weblayer_.get());
@@ -2383,6 +2390,8 @@ void WebMediaPlayerImpl::ActivateViewportIntersectionMonitoring(bool activate) {
   client_->ActivateViewportIntersectionMonitoring(activate);
 }
 
+void WebMediaPlayerImpl::OnWebLayerReplaced() {}
+
 bool WebMediaPlayerImpl::ShouldPauseVideoWhenHidden() const {
   // If suspending background video, pause any video that's not remoted or
   // not unlocked to play in the background.