Bug 112971 – [chromium] Move ownership of compositor VideoLayer to WebMediaPlayer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Move ownership of compositor VideoLayer to WebMediaPlayer

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 194354 [details]
Patch

[Dana Jansens]

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/

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[James Robinson]

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.

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 194432 [details]
Patch

[Dana Jansens]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[James Robinson]

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'

[Dana Jansens]

Created attachment 194644 [details]
Patch

fix typo

[Peter Beverloo (cr-android ews)]

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

[Dana Jansens]

Created attachment 194775 [details]
Patch

for ews

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dana Jansens]

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 ]

[Dana Jansens]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dana Jansens]

Created attachment 194919 [details]
Patch

For EWS again

[WebKit Review Bot]

Comment on attachment 194919 [details]
Patch

Clearing flags on attachment: 194919

Committed r146819: <http://trac.webkit.org/changeset/146819>

[WebKit Review Bot]

Comment on attachment 194919 [details]
Patch

Clearing flags on attachment: 194919

Committed r146843: <http://trac.webkit.org/changeset/146843>