Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2956713003:
Replaces VideoLayer with a SurfaceLayer in WebMediaPlayerImpl (Closed)
|
Created:
3 years, 6 months ago by CJ Modified:
3 years, 5 months ago Reviewers:
mlamouri (slow - plz ping), Stephen White, apacible, Justin Novosad, liberato (no reviews please), pdr., enne (OOO) CC:
blink-reviews, blink-reviews-html_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, fmalita+watch_chromium.org, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplaces VideoLayer with a SurfaceLayer in WebMediaPlayerImpl
This CL creates the necessary interfaces to allow WebMediaPlayer have a SuraceLayer that is able to obtain SurfaceIds via The SurfaceLayerBridge.
In this CL you will also find:
- Renames CanvasSurfaceLayerBridge to SurfaceLayerBridge.
- SurfaceLayerBridge now owns the WebLayer.
- A WebSurfaceLayerBridge interface that allows the creation of SurfaceLayerBridge and the access of its WebLayer in media/blink.
BUG=726619
Review-Url: https://codereview.chromium.org/2956713003
Cr-Commit-Position: refs/heads/master@{#485060}
Committed: https://chromium.googlesource.com/chromium/src/+/7f6009dff057f9a9c23672d642ef0dde90340369
Patch Set 1 #Patch Set 2 : Creates VideoSurfaceLayerBridge and neccessary interfaces. #Patch Set 3 : Replaces VideoLayer with SurfaceLayer. #
Total comments: 28
Patch Set 4 : Addresses comments 8-11. #Patch Set 5 : Addresses comment #13. #
Total comments: 5
Patch Set 6 : Moves some implementation into SurfaceLayerBridge. #
Total comments: 2
Patch Set 7 : Addresses comments 22-26. #Patch Set 8 : Adds check for bridge's WebLayer. #Patch Set 9 : Merge branch 'master' into Bridge #
Total comments: 1
Patch Set 10 : Fixes error in deconstructor. #
Total comments: 17
Patch Set 11 : Fix build issue. #Patch Set 12 : Addresses comment 45 #
Total comments: 14
Patch Set 13 : Addresses comments #56-58. #Messages
Total messages: 71 (36 generated)
lethalantidote@chromium.org changed reviewers: + apacible@chromium.org, enne@chromium.org, liberato@chromium.org, mlamouri@chromium.org
This CL swaps out the VideoLayer for a SurfaceLayer in WebMediaPlayerImpl. I will update the description soon. Beyond checking this to make sure it makes sense, I also have a question about the OnWebLayerReplaced function. First off, it is a sorta misnomer for the WebMediaPlayerImpl implemenation. 1) Should the WebVideoSurfaceLayerBridge own the WebLayer? 2) If not, what do we care about happening when the SurfaceLayer is changed? Do we have to do anything to the WebLayer? 3) What Should happen in OnWebLayerReplaced in either case?
Description was changed from ========== Creates SurfaceLayerBridge interface. BUG=726619 ========== to ========== Replaces VideoLayer with a SurfaceLayer in WebMediaPlayerImpl This CL creates the necessary interfaces to allow WebMediaPlayer have a SuraceLayer that is able to obtain SurfaceIds via The SurfaceLayerBridge. In this CL you will also find: - A new interface SurfaceLayerBridge, that helps share some of the implementation between the CanvasSurfaceLayerBridge and the new VideoSurfaceLayerBridge. - A WebVideoSurfaceLayerBridge interface that allows the creation of VideoSurfaceLayerBridge and the access of its SurfaceLayer in media/blink. - A WebSurfaceLayerBridgeObserver interface, allowing media/blink classes to observe SurfaceLayerBridge objects. - Switches to turn this functionality on/off (taken from an upstream patch, but due to some error, could not upload with it?) BUG=726619 ==========
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
re your questions: my thoughts on them are here: 1) please see comment in wmpi 2) please see comment in VideoSurfaceLayerBridge 3) same as #2 other notes: VideoSurfaceLayerBridge looks to be pretty generic. how much of it can be folded back into the common base class (eventually -- this is still prototyping). we also need to figure out where the common base class is going to live. it might make sense to move part of the offscreen canvas impl out of blink (again, eventually). it could create them similarly to how blink creates a media player now. there are some similarities; it seems like media and canvas are the only two elements that deal with weblayers directly today (based on a code search for "WebLayer f:WebKit f:Element"). not sure what other design considerations led it to be in blink in the first place, so maybe i'm missing some context. it's a good discussion to have with the owners. thanks -fl https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1356: video_weblayer_.reset(new cc_blink::WebLayerImpl(bridge_->GetLayer())); 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. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:84: observer_->OnWebLayerReplaced(); i'm not at all sure why the canvas impl calls this in the else case. |surface_layer| isn't changing anything that i can see that would require a blink compositing update. for video @ToT, WMPI creates the layer and sends it to blink, which causes a blink compositor update. this happens only once. if canvas didn't call OnWebLayerReplaced in the else case, then i think that it would be doing about the same thing as wmpi.
https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1356: video_weblayer_.reset(new cc_blink::WebLayerImpl(bridge_->GetLayer())); On 2017/06/26 at 15:04:03, liberato wrote: > 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. Yes, I think the same ownership model should apply for the surface layers that applies for the video layers now. > 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. 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. https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:237: void OnWebLayerReplaced() override; I think the only reason that CanvasSurfaceLayerBridge needs this is so that it can inform the HTMLCanvasElement to then call Element::SetNeedsCompositingUpdate(). However, I think HTMLMediaElement's SetWebLayer already has this covered: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML....
as for my previous comments about moving out of blink, offline conversation with mlamouri results in "don't move it out of blink". turns out that soup has bigger scope than i'd realized. sorry about that. thanks -fl https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:256: base::WrapUnique(blink::WebVideoSurfaceLayerBridge::Create(this))), maybe only do this if |kVideoSurface|? https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:237: void OnWebLayerReplaced() override; On 2017/06/26 18:35:58, enne wrote: > I think the only reason that CanvasSurfaceLayerBridge needs this is so that it > can inform the HTMLCanvasElement to then call > Element::SetNeedsCompositingUpdate(). > > However, I think HTMLMediaElement's SetWebLayer already has this covered: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML.... +1. that's what i tried, badly, to say in my previous round of comments. :) https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:38: mojom::blink::OffscreenCanvasProviderPtr provider; do you know yet if the the OffscreenCanvasProvider* going to be used verbatim between canvas and video? https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.h:26: class VideoSurfaceLayerBridge : public NON_EXPORTED_BASE(SurfaceLayerBridge), many of these classes could use a few lines of class-level documentation.
Some minor comments. It seems pretty close to what we discussed. Will leave the core review to enne@ and liberato@. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebSurfaceLayerBridgeObserver.h (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebSurfaceLayerBridgeObserver.h:10: class WebSurfaceLayerBridgeObserver { You might want to add some class level documentation. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebSurfaceLayerBridgeObserver.h:12: WebSurfaceLayerBridgeObserver() {} Do you need the ctor here? https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h:9: #include "cc/layers/layer.h" Instead of the #include, you should be able to forward declare. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h:10: #include "public/platform/WebSurfaceLayerBridgeObserver.h" ditto https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h:14: class BLINK_PLATFORM_EXPORT WebVideoSurfaceLayerBridge { same here for the class level documentation
https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:256: base::WrapUnique(blink::WebVideoSurfaceLayerBridge::Create(this))), On 2017/06/26 22:39:30, liberato wrote: > maybe only do this if |kVideoSurface|? Done. https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1356: video_weblayer_.reset(new cc_blink::WebLayerImpl(bridge_->GetLayer())); On 2017/06/26 18:35:58, enne wrote: > On 2017/06/26 at 15:04:03, liberato wrote: > > 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. > > Yes, I think the same ownership model should apply for the surface layers that > applies for the video layers now. > > > 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. > > 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. 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? https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:237: void OnWebLayerReplaced() override; On 2017/06/26 18:35:58, enne wrote: > I think the only reason that CanvasSurfaceLayerBridge needs this is so that it > can inform the HTMLCanvasElement to then call > Element::SetNeedsCompositingUpdate(). > > However, I think HTMLMediaElement's SetWebLayer already has this covered: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML.... This looks good, but what happens when we are updating the SurfaceInfo out of sequence? Right now we are updating SurfaceInfo whenever the Surface is created, which may or may not be after Metadata has loaded. Perhaps we should call SetWebLayer from OnWebLayerReplaced. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:38: mojom::blink::OffscreenCanvasProviderPtr provider; On 2017/06/26 22:39:30, liberato wrote: > do you know yet if the the OffscreenCanvasProvider* going to be used verbatim > between canvas and video? Not sure, was gonna handle that in another CL https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:84: observer_->OnWebLayerReplaced(); On 2017/06/26 15:04:03, liberato wrote: > i'm not at all sure why the canvas impl calls this in the else case. > |surface_layer| isn't changing anything that i can see that would require a > blink compositing update. > > for video @ToT, WMPI creates the layer and sends it to blink, which causes a > blink compositor update. this happens only once. if canvas didn't call > OnWebLayerReplaced in the else case, then i think that it would be doing about > the same thing as wmpi. If changing the CCLayer has no effect on the WebLayer, then I can remove. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.h:26: class VideoSurfaceLayerBridge : public NON_EXPORTED_BASE(SurfaceLayerBridge), On 2017/06/26 22:39:30, liberato wrote: > many of these classes could use a few lines of class-level documentation. Done. Please review my wording. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebSurfaceLayerBridgeObserver.h (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebSurfaceLayerBridgeObserver.h:10: class WebSurfaceLayerBridgeObserver { On 2017/06/27 18:26:37, mlamouri (slow) wrote: > You might want to add some class level documentation. Done. Please check my wording. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebSurfaceLayerBridgeObserver.h:12: WebSurfaceLayerBridgeObserver() {} On 2017/06/27 18:26:37, mlamouri (slow) wrote: > Do you need the ctor here? Done. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h:9: #include "cc/layers/layer.h" On 2017/06/27 18:26:37, mlamouri (slow) wrote: > Instead of the #include, you should be able to forward declare. Done. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h:10: #include "public/platform/WebSurfaceLayerBridgeObserver.h" On 2017/06/27 18:26:38, mlamouri (slow) wrote: > ditto Done. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebVideoSurfaceLayerBridge.h:14: class BLINK_PLATFORM_EXPORT WebVideoSurfaceLayerBridge { On 2017/06/27 18:26:37, mlamouri (slow) wrote: > same here for the class level documentation Done.
https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1356: video_weblayer_.reset(new cc_blink::WebLayerImpl(bridge_->GetLayer())); On 2017/06/29 at 19:51:33, CJ wrote: > On 2017/06/26 18:35:58, enne wrote: > > On 2017/06/26 at 15:04:03, liberato wrote: > > > 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. > > > > Yes, I think the same ownership model should apply for the surface layers that > > applies for the video layers now. > > > > > 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. > > > > 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. > > 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. 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. > We don't have to anything to the WebLayer when we update the Surfacelayer? 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. https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:84: observer_->OnWebLayerReplaced(); On 2017/06/29 at 19:51:33, CJ wrote: > On 2017/06/26 15:04:03, liberato wrote: > > i'm not at all sure why the canvas impl calls this in the else case. > > |surface_layer| isn't changing anything that i can see that would require a > > blink compositing update. > > > > for video @ToT, WMPI creates the layer and sends it to blink, which causes a > > blink compositor update. this happens only once. if canvas didn't call > > OnWebLayerReplaced in the else case, then i think that it would be doing about > > the same thing as wmpi. > > If changing the CCLayer has no effect on the WebLayer, then I can remove. I think the canvas bridge calls OnWebLayerReplaced because the bridge itself is just creating an orphan layer that the CompositedLayerMapping eventually assigns to the correct layer and puts it in the tree: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co.... So, without asking for a compositing update, changes in the bridge would never be picked up and put on screen. Video ends up doing the same thing a few lines above that, in that it's just creating a WebLayer but it's up to the compositor to figure out how to put that in the layer tree. However, HTMLMediaElement::SetWebLayer is the equivalent of OnWebLayerReplaced because it asks for a compositing update.
https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1356: video_weblayer_.reset(new cc_blink::WebLayerImpl(bridge_->GetLayer())); On 2017/06/29 20:29:29, enne wrote: > On 2017/06/29 at 19:51:33, CJ wrote: > > On 2017/06/26 18:35:58, enne wrote: > > > On 2017/06/26 at 15:04:03, liberato wrote: > > > > 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. > > > > > > Yes, I think the same ownership model should apply for the surface layers > that > > > applies for the video layers now. > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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. > > > We don't have to anything to the WebLayer when we update the Surfacelayer? > > 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. 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? https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:84: observer_->OnWebLayerReplaced(); On 2017/06/29 20:29:29, enne wrote: > On 2017/06/29 at 19:51:33, CJ wrote: > > On 2017/06/26 15:04:03, liberato wrote: > > > i'm not at all sure why the canvas impl calls this in the else case. > > > |surface_layer| isn't changing anything that i can see that would require a > > > blink compositing update. > > > > > > for video @ToT, WMPI creates the layer and sends it to blink, which causes a > > > blink compositor update. this happens only once. if canvas didn't call > > > OnWebLayerReplaced in the else case, then i think that it would be doing > about > > > the same thing as wmpi. > > > > If changing the CCLayer has no effect on the WebLayer, then I can remove. > > I think the canvas bridge calls OnWebLayerReplaced because the bridge itself is > just creating an orphan layer that the CompositedLayerMapping eventually assigns > to the correct layer and puts it in the tree: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co.... > So, without asking for a compositing update, changes in the bridge would never > be picked up and put on screen. > > Video ends up doing the same thing a few lines above that, in that it's just > creating a WebLayer but it's up to the compositor to figure out how to put that > in the layer tree. However, HTMLMediaElement::SetWebLayer is the equivalent of > OnWebLayerReplaced because it asks for a compositing update. Makes sense. Going to remove the Observer pattern stuff from this.
Made the discussed changes. PTAL.
I'd really like liberato to chime in here about whether this bridge should live in Blink or should live in media. It sounded like there were some concerns about having media reach out to Blink to create a surface and do some of this work when it could just live in media, but I could be misunderstanding the concerns. I think either place could work and it seems like Blink has some code reuse opportunities, but I want to make sure there's some agreement here. I'm a little unsure of what the differences are between these two bridge implementations. Is it just that the video version creates a cc layer and the canvas version creates a weblayer? It seems a little bit like the RegisterContentsLayer code should also apply to the video layer code as well. Mostly I'm asking if the CanvasSurfaceLayerBridge could be used directly for the VideoSurfaceLayerBridge, assuming that the WebLayerTreeView frame sink id got passed in some other way, and that it's possible to have a null observer, or if there's some fundamental difference between these two types that I'm missing.
On 2017/06/30 22:14:34, enne wrote: > I'd really like liberato to chime in here about whether this bridge should live > in Blink or should live in media. It sounded like there were some concerns blink is preferable, contrary to my previous comments. turns out that it'll work better for onion soup. sorry about the delay getting back to this. -fl
not too many comments, mostly just agreeing with enne@ about similarities in the bridge impls. thanks -fl https://codereview.chromium.org/2956713003/diff/80001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2956713003/diff/80001/media/base/media_switch... media/base/media_switches.cc:247: const base::Feature kVideoSurface{"video-surface", probably want to remove this in favor of the one that landed earlier. https://codereview.chromium.org/2956713003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:52: void VideoSurfaceLayerBridge::CreateSolidColorLayer() { is this used? https://codereview.chromium.org/2956713003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:83: void VideoSurfaceLayerBridge::SatisfyCallback( these look like they could move to the base class. seems like there's a lot of similarity elsewhere too.
On 2017/06/30 22:14:34, enne wrote: > I'd really like liberato to chime in here about whether this bridge should live > in Blink or should live in media. It sounded like there were some concerns > about having media reach out to Blink to create a surface and do some of this > work when it could just live in media, but I could be misunderstanding the > concerns. I think either place could work and it seems like Blink has some code > reuse opportunities, but I want to make sure there's some agreement here. > > I'm a little unsure of what the differences are between these two bridge > implementations. Is it just that the video version creates a cc layer and the > canvas version creates a weblayer? It seems a little bit like the > RegisterContentsLayer code should also apply to the video layer code as well. > Mostly I'm asking if the CanvasSurfaceLayerBridge could be used directly for the > VideoSurfaceLayerBridge, assuming that the WebLayerTreeView frame sink id got > passed in some other way, and that it's possible to have a null observer, or if > there's some fundamental difference between these two types that I'm missing. @liberato, does the webmediaplayer handle what RegisterContentsLayer does? This is just something I assumed.
On 2017/06/30 22:14:34, enne wrote: > I'd really like liberato to chime in here about whether this bridge should live > in Blink or should live in media. It sounded like there were some concerns > about having media reach out to Blink to create a surface and do some of this > work when it could just live in media, but I could be misunderstanding the > concerns. I think either place could work and it seems like Blink has some code > reuse opportunities, but I want to make sure there's some agreement here. > > I'm a little unsure of what the differences are between these two bridge > implementations. Is it just that the video version creates a cc layer and the > canvas version creates a weblayer? It seems a little bit like the > RegisterContentsLayer code should also apply to the video layer code as well. > Mostly I'm asking if the CanvasSurfaceLayerBridge could be used directly for the > VideoSurfaceLayerBridge, assuming that the WebLayerTreeView frame sink id got > passed in some other way, and that it's possible to have a null observer, or if > there's some fundamental difference between these two types that I'm missing. The major difference between the two Bridges is that CanvasSurfaceLayerBridge controls a web_layer, where as the VideoSurfaceLayerBridge does not.
Ive moved some of the implementation into SurfaceLayerBridge to be shared. If we rather have it such that we only have one implementer of SurfaceLayerBridge, let me know, but I feel like that is not as clean. https://codereview.chromium.org/2956713003/diff/80001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2956713003/diff/80001/media/base/media_switch... media/base/media_switches.cc:247: const base::Feature kVideoSurface{"video-surface", On 2017/06/30 22:27:16, liberato wrote: > probably want to remove this in favor of the one that landed earlier. Done. https://codereview.chromium.org/2956713003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/VideoSurfaceLayerBridge.cpp:52: void VideoSurfaceLayerBridge::CreateSolidColorLayer() { On 2017/06/30 22:27:16, liberato wrote: > is this used? No. I originally thought it might have something to do with producing a solid colored frame on the screen, but I guess not! Removing.
> The major difference between the two Bridges is that CanvasSurfaceLayerBridge controls a web_layer, where as the VideoSurfaceLayerBridge does not. But the media players immediately that layer in a WebLayer, which is why I wondered if they could share the same code. It doesn't seem like that fundamental of a difference.
On 2017/06/30 23:46:58, enne wrote: > > The major difference between the two Bridges is that CanvasSurfaceLayerBridge > controls a web_layer, where as the VideoSurfaceLayerBridge does not. > > But the media players immediately that layer in a WebLayer, which is why I > wondered if they could share the same code. It doesn't seem like that > fundamental of a difference. @liberato, how would you feel about the Bridge owning the WebLayer?
On 2017/07/01 00:05:26, CJ wrote: > On 2017/06/30 23:46:58, enne wrote: > > > The major difference between the two Bridges is that > CanvasSurfaceLayerBridge > > controls a web_layer, where as the VideoSurfaceLayerBridge does not. > > > > But the media players immediately that layer in a WebLayer, which is why I > > wondered if they could share the same code. It doesn't seem like that > > fundamental of a difference. > > @liberato, how would you feel about the Bridge owning the WebLayer? no objections. it doesn't even look like much is going to change in WMPI, since it's okay with a null |video_weblayer_|. the main thing that will be affected is the call to StopUsingProvider(), but that's guaranteed to change either way. the provider won't be owned by the SurfaceLayer any more once we use cc:Surfaces; that will move elsewhere. there's also a call to SetOpacity that we'd need to handle somehow, but that can just be a special case to instead tell the bridge. thanks -fl
On 2017/07/01 04:57:03, liberato wrote: > On 2017/07/01 00:05:26, CJ wrote: > > On 2017/06/30 23:46:58, enne wrote: > > > > The major difference between the two Bridges is that > > CanvasSurfaceLayerBridge > > > controls a web_layer, where as the VideoSurfaceLayerBridge does not. > > > > > > But the media players immediately that layer in a WebLayer, which is why I > > > wondered if they could share the same code. It doesn't seem like that > > > fundamental of a difference. > > > > @liberato, how would you feel about the Bridge owning the WebLayer? > > no objections. it doesn't even look like much is going to change in WMPI, since > it's okay with a null |video_weblayer_|. the main thing that will be affected > is the call to StopUsingProvider(), but that's guaranteed to change either way. > the provider won't be owned by the SurfaceLayer any more once we use > cc:Surfaces; that will move elsewhere. > > there's also a call to SetOpacity that we'd need to handle somehow, but that can > just be a special case to instead tell the bridge. > > thanks > -fl Question about the Bridge owning the WebLayer, the Bridge can only know about WebLayer, but we need it to be able to return WebLayerImpl's. Is there a way to convert between the two?
On 2017/07/02 00:39:52, CJ wrote: > On 2017/07/01 04:57:03, liberato wrote: > > On 2017/07/01 00:05:26, CJ wrote: > > > On 2017/06/30 23:46:58, enne wrote: > > > > > The major difference between the two Bridges is that > > > CanvasSurfaceLayerBridge > > > > controls a web_layer, where as the VideoSurfaceLayerBridge does not. > > > > > > > > But the media players immediately that layer in a WebLayer, which is why I > > > > wondered if they could share the same code. It doesn't seem like that > > > > fundamental of a difference. > > > > > > @liberato, how would you feel about the Bridge owning the WebLayer? > > > > no objections. it doesn't even look like much is going to change in WMPI, > since > > it's okay with a null |video_weblayer_|. the main thing that will be affected > > is the call to StopUsingProvider(), but that's guaranteed to change either > way. > > the provider won't be owned by the SurfaceLayer any more once we use > > cc:Surfaces; that will move elsewhere. > > > > there's also a call to SetOpacity that we'd need to handle somehow, but that > can > > just be a special case to instead tell the bridge. > > > > thanks > > -fl > > Question about the Bridge owning the WebLayer, the Bridge can only know about > WebLayer, but we need it to be able to return WebLayerImpl's. Is there a way to > convert between the two? where do you need a WebLayerImpl, and for which methods? thanks -fl
On 2017/07/05 13:50:09, liberato wrote: > On 2017/07/02 00:39:52, CJ wrote: > > On 2017/07/01 04:57:03, liberato wrote: > > > On 2017/07/01 00:05:26, CJ wrote: > > > > On 2017/06/30 23:46:58, enne wrote: > > > > > > The major difference between the two Bridges is that > > > > CanvasSurfaceLayerBridge > > > > > controls a web_layer, where as the VideoSurfaceLayerBridge does not. > > > > > > > > > > But the media players immediately that layer in a WebLayer, which is why > I > > > > > wondered if they could share the same code. It doesn't seem like that > > > > > fundamental of a difference. > > > > > > > > @liberato, how would you feel about the Bridge owning the WebLayer? > > > > > > no objections. it doesn't even look like much is going to change in WMPI, > > since > > > it's okay with a null |video_weblayer_|. the main thing that will be > affected > > > is the call to StopUsingProvider(), but that's guaranteed to change either > > way. > > > the provider won't be owned by the SurfaceLayer any more once we use > > > cc:Surfaces; that will move elsewhere. > > > > > > there's also a call to SetOpacity that we'd need to handle somehow, but that > > can > > > just be a special case to instead tell the bridge. > > > > > > thanks > > > -fl > > > > Question about the Bridge owning the WebLayer, the Bridge can only know about > > WebLayer, but we need it to be able to return WebLayerImpl's. Is there a way > to > > convert between the two? > > where do you need a WebLayerImpl, and for which methods? > > thanks > -fl We discussed this offline. SetContentsOpaqueIsFixed is the only thing we have to worry about, and we dont call it for now. Here is the rework with the Bridge owning the WebLayer. PTAL.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Thanks for iterating to get to this point. I think this is really clean. :) lgtm, although I'm not an owner of any of this. I have one nit, but since this is all behind a switch and has a TODO, it's just a side thought. https://codereview.chromium.org/2956713003/diff/160001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/160001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1370: bridge_->GetCCLayer()->SetContentsOpaque(opaque_); To be honest, I'm not sure that this is necessary? I think this plumbing ultimately just sets this bit on the video draw quad: https://cs.chromium.org/chromium/src/cc/layers/video_layer_impl.cc?rcl=8a4399... I'm mostly sure that setting the opaque rect correctly on the video quads that are generated will do the same thing here.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lethalantidote@chromium.org changed reviewers: + pdr@chromium.org, senorblanco@chromium.org
+senorblanco OWNERS on third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp, third_party/WebKit/Source/platform/* +pdr OWNERS on third_party/WebKit/public/*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM for WebKit/public
senorblanco@chromium.org changed reviewers: + junov@chromium.org - senorblanco@chromium.org
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
Better that Justin take a look at this.
overall looks good! just some relatively superficial comments. thanks -fl https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:271: if (base::FeatureList::IsEnabled(media::kUseSurfaceLayerForVideo)) please cache this, similar to |overlay_mode_|, since it's called quite a lot. https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:336: static_cast<cc::VideoLayer*>(bridge_->GetCCLayer())->StopUsingProvider(); the weblayer is a SurfaceLayer, not a VideoLayer. this functionality won't end up being routed to the bridge at all, but rather to whatever it is that's sending VideoFrame-backed quads to the frame sink that |bridge_| is embedding. we haven't really figured that out yet. probably just delete the whole if @334, open a bug, assign it to yourself, and add a TODO to reference it. as we design that part, we'll handle this. https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1372: // TODO(lethalantidote): Figure out how to persist opaque setting maybe open a bug for this, add it as a blocker to the tracking bug for the cc::Surfaces conversion, and add a reference to it here. https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1752: if (!video_weblayer_) maybe add a weblayer() helper that would return |video_weblayer_| or GetWebLayer(), as appropriate. we could use it in several places. everywhere, i think, except the ~1 place where we actually need the WebLayerImpl. https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1758: return pipeline_metadate_.natural_size; s/metadate/metadata https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:725: // Obtains and maintains SurfaceIds. '... for kWhateverThatFeatureNameIs'. also, this does quite a bit more than SurfaceIds, e.g., owning the weblayer. https://codereview.chromium.org/2956713003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:39: public WebVideoSurfaceLayerBridge { not sure that SurfaceLayerBridge should implement something called WebVideoSurfaceLayerBridge. SLB is more generic than that. perhaps WebVideoSurfaceLayerBridge should just be WebSurfaceLayerBridge, since there's nothing specific to video about it. https://codereview.chromium.org/2956713003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:53: cc::Layer* GetCCLayer() const override { return cc_layer_.get(); } is there any case where this isn't the same as web_layer_->CcLayer()? do we really need both?
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:271: if (base::FeatureList::IsEnabled(media::kUseSurfaceLayerForVideo)) On 2017/07/06 16:56:17, liberato wrote: > please cache this, similar to |overlay_mode_|, since it's called quite a lot. Done. https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:336: static_cast<cc::VideoLayer*>(bridge_->GetCCLayer())->StopUsingProvider(); On 2017/07/06 16:56:18, liberato wrote: > the weblayer is a SurfaceLayer, not a VideoLayer. this functionality won't end > up being routed to the bridge at all, but rather to whatever it is that's > sending VideoFrame-backed quads to the frame sink that |bridge_| is embedding. > we haven't really figured that out yet. > > probably just delete the whole if @334, open a bug, assign it to yourself, and > add a TODO to reference it. as we design that part, we'll handle this. Done. https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1372: // TODO(lethalantidote): Figure out how to persist opaque setting On 2017/07/06 16:56:18, liberato wrote: > maybe open a bug for this, add it as a blocker to the tracking bug for the > cc::Surfaces conversion, and add a reference to it here. Done. https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1752: if (!video_weblayer_) On 2017/07/06 16:56:17, liberato wrote: > maybe add a weblayer() helper that would return |video_weblayer_| or > GetWebLayer(), as appropriate. we could use it in several places. everywhere, > i think, except the ~1 place where we actually need the WebLayerImpl. Wont all the |video_weblayer_| code be going away anyway? Leaving it like this will make it easy to delete. https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1758: return pipeline_metadate_.natural_size; On 2017/07/06 16:56:18, liberato wrote: > s/metadate/metadata Thanks! I wonder why the compiler is not catching that... https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:725: // Obtains and maintains SurfaceIds. On 2017/07/06 16:56:18, liberato wrote: > '... for kWhateverThatFeatureNameIs'. > > also, this does quite a bit more than SurfaceIds, e.g., owning the weblayer. Done. https://codereview.chromium.org/2956713003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:39: public WebVideoSurfaceLayerBridge { On 2017/07/06 16:56:18, liberato wrote: > not sure that SurfaceLayerBridge should implement something called > WebVideoSurfaceLayerBridge. SLB is more generic than that. > > perhaps WebVideoSurfaceLayerBridge should just be WebSurfaceLayerBridge, since > there's nothing specific to video about it. Done. https://codereview.chromium.org/2956713003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:53: cc::Layer* GetCCLayer() const override { return cc_layer_.get(); } On 2017/07/06 16:56:18, liberato wrote: > is there any case where this isn't the same as web_layer_->CcLayer()? do we > really need both? For some reason I thought it was available on WebLayer. Done.
Description was changed from ========== Replaces VideoLayer with a SurfaceLayer in WebMediaPlayerImpl This CL creates the necessary interfaces to allow WebMediaPlayer have a SuraceLayer that is able to obtain SurfaceIds via The SurfaceLayerBridge. In this CL you will also find: - A new interface SurfaceLayerBridge, that helps share some of the implementation between the CanvasSurfaceLayerBridge and the new VideoSurfaceLayerBridge. - A WebVideoSurfaceLayerBridge interface that allows the creation of VideoSurfaceLayerBridge and the access of its SurfaceLayer in media/blink. - A WebSurfaceLayerBridgeObserver interface, allowing media/blink classes to observe SurfaceLayerBridge objects. - Switches to turn this functionality on/off (taken from an upstream patch, but due to some error, could not upload with it?) BUG=726619 ========== to ========== Replaces VideoLayer with a SurfaceLayer in WebMediaPlayerImpl This CL creates the necessary interfaces to allow WebMediaPlayer have a SuraceLayer that is able to obtain SurfaceIds via The SurfaceLayerBridge. In this CL you will also find: - Renames CanvasSurfaceLayerBridge to SurfaceLayerBridge. - SurfaceLayerBridge now owns the WebLayer. - A WebSurfaceLayerBridge interface that allows the creation of SurfaceLayerBridge and the access of its WebLayer in media/blink. BUG=726619 ==========
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
lgtm https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:41: explicit SurfaceLayerBridge(SurfaceLayerBridgeObserver*, WebLayerTreeView*); I know you are just moving this but it seems that you do not need `explicit`. https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:42: ~SurfaceLayerBridge(); Could you make the dtor `virtual` or make the class `final`?
media/ lgtm. thanks -fl https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/180001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1758: return pipeline_metadate_.natural_size; On 2017/07/06 20:45:18, CJ wrote: > On 2017/07/06 16:56:18, liberato wrote: > > s/metadate/metadata > > Thanks! I wonder why the compiler is not catching that... the entire group of fns is #if android. https://codereview.chromium.org/2956713003/diff/220001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/220001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1369: if (bridge_->GetWebLayer()) { } else if () {, mostly to reduce the indentation. https://codereview.chromium.org/2956713003/diff/220001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1574: if (bridge_->GetWebLayer()) { } else if () { https://codereview.chromium.org/2956713003/diff/220001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1757: if (!bridge_->GetWebLayer()) } else if () { https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:51: // Implementation of WebVideoSurfaceLayerBridge. s/WebVideo/Web
Been following along throughout the iterations -- looks nice! lgtm + nits https://codereview.chromium.org/2956713003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:42: nit: remove newline. https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp (left): https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp:55: nit: add back newline between namespace and closing brace https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp:133: if (observer_) I may have missed it in earlier comments -- with the change from CanvasSurfaceLayerBridgeObserver to SurfaceLayerBridgeObserver, when might |observer_| be nullptr? Is this just an extra check we could have had earlier?
~Ping~ Justin Novosad. https://codereview.chromium.org/2956713003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:42: On 2017/07/07 18:25:38, apacible wrote: > nit: remove newline. I think you are reviewing an older patch set. Threw me through a loop cause this class no longer exists. https://codereview.chromium.org/2956713003/diff/220001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2956713003/diff/220001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1757: if (!bridge_->GetWebLayer()) On 2017/07/07 15:26:43, liberato wrote: > } else if () { Done. https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp (left): https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp:55: On 2017/07/07 18:25:38, apacible wrote: > nit: add back newline between namespace and closing brace Done. https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp (right): https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.cpp:133: if (observer_) On 2017/07/07 18:25:38, apacible wrote: > I may have missed it in earlier comments -- with the change from > CanvasSurfaceLayerBridgeObserver to SurfaceLayerBridgeObserver, when might > |observer_| be nullptr? Is this just an extra check we could have had earlier? We dont use the |observer_|. That's only for the HTMLCanvasElement. We initialize the SurfaceLayerBridge with a nullptr. https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h (right): https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:41: explicit SurfaceLayerBridge(SurfaceLayerBridgeObserver*, WebLayerTreeView*); On 2017/07/07 14:42:38, mlamouri (slow - plz ping) wrote: > I know you are just moving this but it seems that you do not need `explicit`. Done. https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:42: ~SurfaceLayerBridge(); On 2017/07/07 14:42:38, mlamouri (slow - plz ping) wrote: > Could you make the dtor `virtual` or make the class `final`? Done. https://codereview.chromium.org/2956713003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/SurfaceLayerBridge.h:51: // Implementation of WebVideoSurfaceLayerBridge. On 2017/07/07 15:26:43, liberato wrote: > s/WebVideo/Web Done.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org, pdr@chromium.org, mlamouri@chromium.org, apacible@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/2956713003/#ps240001 (title: "Addresses comments #56-58.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1499463669397820,
"parent_rev": "ed0d20f7c347cc2ec55621918941556e141d8341", "commit_rev":
"7f6009dff057f9a9c23672d642ef0dde90340369"}
Message was sent while issue was closed.
Description was changed from ========== Replaces VideoLayer with a SurfaceLayer in WebMediaPlayerImpl This CL creates the necessary interfaces to allow WebMediaPlayer have a SuraceLayer that is able to obtain SurfaceIds via The SurfaceLayerBridge. In this CL you will also find: - Renames CanvasSurfaceLayerBridge to SurfaceLayerBridge. - SurfaceLayerBridge now owns the WebLayer. - A WebSurfaceLayerBridge interface that allows the creation of SurfaceLayerBridge and the access of its WebLayer in media/blink. BUG=726619 ========== to ========== Replaces VideoLayer with a SurfaceLayer in WebMediaPlayerImpl This CL creates the necessary interfaces to allow WebMediaPlayer have a SuraceLayer that is able to obtain SurfaceIds via The SurfaceLayerBridge. In this CL you will also find: - Renames CanvasSurfaceLayerBridge to SurfaceLayerBridge. - SurfaceLayerBridge now owns the WebLayer. - A WebSurfaceLayerBridge interface that allows the creation of SurfaceLayerBridge and the access of its WebLayer in media/blink. BUG=726619 Review-Url: https://codereview.chromium.org/2956713003 Cr-Commit-Position: refs/heads/master@{#485060} Committed: https://chromium.googlesource.com/chromium/src/+/7f6009dff057f9a9c23672d642ef... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/7f6009dff057f9a9c23672d642ef... |
