Bug 144782 - 3D-transformed video does not display on platforms without accelerated video rendering
Summary: 3D-transformed video does not display on platforms without accelerated video ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: daegyu.lee@navercorp.com
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-07 19:29 PDT by daegyu.lee@navercorp.com
Modified: 2016-03-30 14:08 PDT (History)
7 users (show)

See Also:


Attachments
Set GraphicsLayer::m_drawsContent = true (1.61 KB, patch)
2015-05-07 19:37 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (2.27 KB, patch)
2015-05-08 01:56 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (5.31 KB, patch)
2015-05-08 02:06 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (5.00 KB, patch)
2015-05-08 02:15 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (7.34 KB, patch)
2015-05-10 21:03 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (8.09 KB, patch)
2015-05-11 01:43 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (8.09 KB, patch)
2015-05-11 02:06 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (10.47 KB, patch)
2015-06-01 20:21 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (11.68 KB, patch)
2015-06-05 01:12 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff
patch (10.45 KB, patch)
2015-06-09 20:45 PDT, daegyu.lee@navercorp.com
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description daegyu.lee@navercorp.com 2015-05-07 19:29:00 PDT
I checked same behavior on EFL port.
I checked same behavior on GTK port with disable GSTREAMER_GL and ENABLE_THREADED_COMPOSITOR.

Video tag does not get RenderLayerBacking when MediaPlayerPrivate::supportsAcceleratedRendering() returns false which means no using accelerated video decoding.
With changing CSS Style by javascript, video tag gets RenderLayerBacking.
But there  is no way to set GraphicsLayer::setDrawsContent(true) for updating video content.

Reproduce: http://codepen.io/anon/pen/qdbBep
Comment 1 daegyu.lee@navercorp.com 2015-05-07 19:37:33 PDT
Created attachment 252676 [details]
Set GraphicsLayer::m_drawsContent = true
Comment 2 Simon Fraser (smfr) 2015-05-07 20:24:36 PDT
Comment on attachment 252676 [details]
Set GraphicsLayer::m_drawsContent = true

View in context: https://bugs.webkit.org/attachment.cgi?id=252676&action=review

> Source/WebCore/ChangeLog:7
> +

There's no explanation here of why being CSS transformed from JavaScript matters, and what the code change is doing.

Is this testable?
Comment 3 Alexey Proskuryakov 2015-05-07 23:42:28 PDT
> Reproduce: http://codepen.io/anon/pen/qdbBep

The video at this URL fails to play in shipping Safari, so it's difficult to see what the regression is. Could you please clarify how to reproduce the problem?
Comment 4 daegyu.lee@navercorp.com 2015-05-08 00:23:38 PDT
Ok, I'm on the way to make the test case in layouttest.
Comment 5 daegyu.lee@navercorp.com 2015-05-08 01:56:33 PDT
Created attachment 252702 [details]
patch
Comment 6 daegyu.lee@navercorp.com 2015-05-08 02:06:31 PDT
Created attachment 252704 [details]
patch
Comment 7 daegyu.lee@navercorp.com 2015-05-08 02:15:04 PDT
Created attachment 252707 [details]
patch
Comment 8 Simon Fraser (smfr) 2015-05-08 11:55:40 PDT
Comment on attachment 252707 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252707&action=review

The code change seems OK, but the test will break everyone.

> LayoutTests/media/video-transformed-by-javascript-expected.txt:12
> +          (drawsContent 1)

This result will be platform-specific, because this layer will only be drawsContent=1 on platforms that don't have supportsAcceleratedRendering().

> LayoutTests/media/video-transformed-by-javascript.html:29
> +    <video id="videoRotate" width="480" height="270" type="video/mp4" src="test.mp4"/>

test.mp4 doesn't exist in that directory. You need to use the functions in media-file.js to load a media type that works on all platforms, like the other tests in this directory.
Comment 9 daegyu.lee@navercorp.com 2015-05-08 12:05:53 PDT
Comment on attachment 252707 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252707&action=review

>> LayoutTests/media/video-transformed-by-javascript-expected.txt:12
>> +          (drawsContent 1)
> 
> This result will be platform-specific, because this layer will only be drawsContent=1 on platforms that don't have supportsAcceleratedRendering().

Agreed. Do you have any recommendation like pixel-level comparisons?

>> LayoutTests/media/video-transformed-by-javascript.html:29
>> +    <video id="videoRotate" width="480" height="270" type="video/mp4" src="test.mp4"/>
> 
> test.mp4 doesn't exist in that directory. You need to use the functions in media-file.js to load a media type that works on all platforms, like the other tests in this directory.

Yes, you are right.
Comment 10 daegyu.lee@navercorp.com 2015-05-10 21:03:39 PDT
Created attachment 252839 [details]
patch
Comment 11 Gyuyoung Kim 2015-05-10 23:33:03 PDT
Comment on attachment 252839 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252839&action=review

> Source/WebCore/testing/Internals.cpp:2372
> +    return element->player()->supportsAcceleratedRendering();

It seems that element->player() needs to be checked if it is null or not.
Comment 12 daegyu.lee@navercorp.com 2015-05-11 01:18:54 PDT
Comment on attachment 252839 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252839&action=review

>> Source/WebCore/testing/Internals.cpp:2372
>> +    return element->player()->supportsAcceleratedRendering();
> 
> It seems that element->player() needs to be checked if it is null or not.

Ok, thanks.
Comment 13 daegyu.lee@navercorp.com 2015-05-11 01:43:26 PDT
Created attachment 252848 [details]
patch
Comment 14 daegyu.lee@navercorp.com 2015-05-11 02:06:03 PDT
Created attachment 252850 [details]
patch
Comment 15 daegyu.lee@navercorp.com 2015-05-13 18:37:18 PDT
Ping...
Comment 16 Gyuyoung Kim 2015-05-20 23:48:00 PDT
(In reply to comment #3)
> > Reproduce: http://codepen.io/anon/pen/qdbBep
> 
> The video at this URL fails to play in shipping Safari, so it's difficult to
> see what the regression is. Could you please clarify how to reproduce the
> problem?

Daegyu, is there any other way to reproduce this bug ?

Simon, we need to fix this bug. So could you take a look this patch again ?
Comment 17 daegyu.lee@navercorp.com 2015-05-21 00:33:28 PDT
http://codepen.io/anon/pen/pJEROO

I have tested above url on EFL. Video is not shown when you click "Flip Video" button.

I have tested above url on GTK without accelerated video rendering. Video is not shown when you click "Flip Video" button.

I have tested above url on Mac. Video is shown correctly.
Comment 18 Simon Fraser (smfr) 2015-05-26 11:28:58 PDT
Comment on attachment 252850 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252850&action=review

> LayoutTests/media/video-transformed-by-javascript.html:22
> +                if (window.internals.supportsAcceleratedRendering(document.getElementById("videoRotate")) == false) {
> +                    layerTreeAsText = window.internals.layerTreeAsText(document);
> +                    if (layerTreeAsText.indexOf('(drawsContent 1') == -1)
> +                        document.getElementById('outText').innerText = "FAIL";
> +                }
> +                testRunner.notifyDone();

I'm not sure it's worth going to the trouble of adding internals.supportsAcceleratedRendering(). Why not just land different results for different platforms?
Comment 19 daegyu.lee@navercorp.com 2015-06-01 20:21:30 PDT
Created attachment 254042 [details]
patch
Comment 20 daegyu.lee@navercorp.com 2015-06-05 01:12:21 PDT
Created attachment 254346 [details]
patch
Comment 21 Gyuyoung Kim 2015-06-09 20:15:45 PDT
Comment on attachment 254346 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254346&action=review

Simon already set r+ed. Please only request cq? after fixing Simon's comment.

> LayoutTests/ChangeLog:21
> +2015-06-05  daegyu lee  <daegyu.lee@navercorp.com>

Duplicated changelog.
Comment 22 daegyu.lee@navercorp.com 2015-06-09 20:45:38 PDT
Created attachment 254624 [details]
patch
Comment 23 WebKit Commit Bot 2015-06-09 21:24:42 PDT
Comment on attachment 254624 [details]
patch

Rejecting attachment 254624 [details] from commit-queue.

daegyu.lee@navercorp.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 24 Gyuyoung Kim 2015-06-09 21:48:42 PDT
Comment on attachment 254624 [details]
patch

cq=me. win-ews seems to be broken by other patch.
Comment 25 WebKit Commit Bot 2015-06-09 22:38:36 PDT
Comment on attachment 254624 [details]
patch

Clearing flags on attachment: 254624

Committed r185402: <http://trac.webkit.org/changeset/185402>
Comment 26 WebKit Commit Bot 2015-06-09 22:38:43 PDT
All reviewed patches have been landed.  Closing bug.