[chromium] Move ownership of compositor VideoLayer to WebMediaPlayer
This implements the changes in https://bugs.webkit.org/show_bug.cgi?id=112483 without removing the headers, so that chromium can continue to compile until it stops including them. I chose to do this in order to reduce the number of ifdefs required in the chromium code during this transition.
Created attachment 194354 [details] Patch
This will depend on the following CLs rolling into WebKit: https://codereview.chromium.org/12596015/ https://codereview.chromium.org/12895006/ https://codereview.chromium.org/12676004/
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 194354 [details] Patch Attachment 194354 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17120672
Comment on attachment 194354 [details] Patch Attachment 194354 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17212875
Comment on attachment 194354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194354&action=review > Source/Platform/chromium/public/WebVideoFrame.h:36 > +#ifndef REMOVE_WEBVIDEOFRAME > +#define REMOVE_WEBVIDEOFRAME > +#endif Can you please just pick one header to #define this in and set it there? Maybe WebLayer.h ? > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:276 > +void WebMediaPlayerClientImpl::setWebLayer(WebLayer* layer) I still think we need a synthetic style recalc here, even if the old code didn't have it. Without that there's nothing to make sure we get into RenderLayerBacking to call setContentsToMedia() and hook the PlatformLayer into the compositing tree > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:356 > + ASSERT(m_videoLayer); > + return m_videoLayer; Hmm, I think it's reasonable to ask for the platformLayer() for a <video> element that is composited but doesn't have composited content. What happens if you have a page with a <video style="-webkit-transform:translateZ(0)" src="..."> that isn't in composited video playback? > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:703 > + Frame* frame = static_cast<HTMLMediaElement*>( > + m_mediaPlayer->mediaPlayerClient())->document()->frame(); Could any of these be null? Since this is a public API function I don't think we can make assumptions about the state.
Comment on attachment 194354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194354&action=review >> Source/Platform/chromium/public/WebVideoFrame.h:36 >> +#endif > > Can you please just pick one header to #define this in and set it there? Maybe WebLayer.h ? stream_texture_factory_impl_android.cc only includes WebStreamTextureClient.h and WGC3D.h. But I can just put it in WebVideoLayer.h and WebStreamTextureClient.h. Both of these are getting removed anyways, so it's easy to clean up. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:276 >> +void WebMediaPlayerClientImpl::setWebLayer(WebLayer* layer) > > I still think we need a synthetic style recalc here, even if the old code didn't have it. Without that there's nothing to make sure we get into RenderLayerBacking to call setContentsToMedia() and hook the PlatformLayer into the compositing tree I tried to figure out how this could happen, but with no luck. It always does a layout or something and you get a composited layer. Anyway, I've added it. I was trying stuff like this: <video style="width: 100px; height: 70px" id=v loop></video> <script> var v = document.getElementById('v'); v.src = 'http://promodj.com/download/3972016/Baauer%20%E2%80%93%20Harlem%20Shake%20(SoundRus%20remix)%20(promodj.com).mp3'; window.setTimeout(function() {document.getElementById('v').src = 'http://goo.gl/AD9aO';}, 2000); </script> Also, when the MediaPlayer is being shut down, m_mediaPlayer->mediaPlayerClient() is NULL, so we can't use that to find the HTMLMediaElement* anymore there. So I've guarded it on that being not null. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:356 >> + return m_videoLayer; > > Hmm, I think it's reasonable to ask for the platformLayer() for a <video> element that is composited but doesn't have composited content. What happens if you have a page with a <video style="-webkit-transform:translateZ(0)" src="..."> that isn't in composited video playback? Ya, I managed to hit this assert, thanks! I removed it. >> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:703 >> + m_mediaPlayer->mediaPlayerClient())->document()->frame(); > > Could any of these be null? Since this is a public API function I don't think we can make assumptions about the state. The frame is also grabbed in WebMediaPlayerClientImpl::loadInternal(). So it is assumed valid at that time. I can cache the value there instead of making the same assumption in a new codepath.
Created attachment 194432 [details] Patch
Created attachment 194434 [details] Patch Actually, I can just include WebVideoFrame.h on the chromium side for the define there, and we'll remove those includes later anyway. So just setting it in there.
Comment on attachment 194434 [details] Patch Attachment 194434 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17290138
Comment on attachment 194434 [details] Patch Attachment 194434 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17214621
Comment on attachment 194434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194434&action=review > Source/Platform/ChangeLog:8 > + Defines REMOVE_WEBVIDEOFRAME in relateed headers to enable this code typo 'relateed'->'related'
Created attachment 194644 [details] Patch fix typo
Comment on attachment 194644 [details] Patch Attachment 194644 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17221673
Created attachment 194775 [details] Patch for ews
Comment on attachment 194644 [details] Patch Attachment 194644 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17323024 New failing tests: compositing/geometry/limit-layer-bounds-transformed.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-clipping-ancestor.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-positioned.html compositing/geometry/limit-layer-bounds-clipping-ancestor.html compositing/geometry/limit-layer-bounds-fixed-positioned.html compositing/geometry/limit-layer-bounds-positioned.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-fixed-positioned.html compositing/geometry/limit-layer-bounds-transformed-overflow.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-overflow-root.html compositing/geometry/limit-layer-bounds-positioned-transition.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-transformed.html platform/chromium/virtual/softwarecompositing/layer-creation/scroll-partial-update.html compositing/geometry/limit-layer-bounds-overflow-root.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-positioned-transition.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-transformed-overflow.html compositing/layer-creation/scroll-partial-update.html
Created attachment 194780 [details] Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Hrm. These are failing with the patch. I'll have to take a look tomorrow, it's somewhat unexpected from the test names, but not sure how they're failing yet: Regressions: Unexpected text-only failures (16) compositing/geometry/limit-layer-bounds-clipping-ancestor.html [ Failure ] compositing/geometry/limit-layer-bounds-fixed-positioned.html [ Failure ] compositing/geometry/limit-layer-bounds-overflow-root.html [ Failure ] compositing/geometry/limit-layer-bounds-positioned-transition.html [ Failure ] compositing/geometry/limit-layer-bounds-positioned.html [ Failure ] compositing/geometry/limit-layer-bounds-transformed-overflow.html [ Failure ] compositing/geometry/limit-layer-bounds-transformed.html [ Failure ] compositing/layer-creation/scroll-partial-update.html [ Failure ] platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-clipping-ancestor.html [ Failure ] platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-fixed-positioned.html [ Failure ] platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-overflow-root.html [ Failure ] platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-positioned-transition.html [ Failure ] platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-positioned.html [ Failure ] platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-transformed-overflow.html [ Failure ] platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-transformed.html [ Failure ] platform/chromium/virtual/softwarecompositing/layer-creation/scroll-partial-update.html [ Failure ]
Oh, I guess the video tag isn't getting promoted to a composited layer in the tests anymore for some reason.. Will have to investigate.
Comment on attachment 194775 [details] Patch Attachment 194775 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17290606 New failing tests: compositing/geometry/limit-layer-bounds-transformed.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-clipping-ancestor.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-positioned.html compositing/geometry/limit-layer-bounds-clipping-ancestor.html compositing/geometry/limit-layer-bounds-fixed-positioned.html compositing/geometry/limit-layer-bounds-positioned.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-fixed-positioned.html compositing/geometry/limit-layer-bounds-transformed-overflow.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-overflow-root.html compositing/geometry/limit-layer-bounds-positioned-transition.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-transformed.html platform/chromium/virtual/softwarecompositing/layer-creation/scroll-partial-update.html compositing/geometry/limit-layer-bounds-overflow-root.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-positioned-transition.html platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-transformed-overflow.html compositing/layer-creation/scroll-partial-update.html
Created attachment 194792 [details] Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Created attachment 194919 [details] Patch For EWS again
Comment on attachment 194919 [details] Patch Clearing flags on attachment: 194919 Committed r146819: <http://trac.webkit.org/changeset/146819>
Comment on attachment 194919 [details] Patch Clearing flags on attachment: 194919 Committed r146843: <http://trac.webkit.org/changeset/146843>