Bug 91562 - [chromium] On Windows the compositor incorrectly flips videos
Summary: [chromium] On Windows the compositor incorrectly flips videos
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Ananta Iyengar
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-17 17:19 PDT by Ananta Iyengar
Modified: 2012-07-18 15:20 PDT (History)
7 users (show)

See Also:


Attachments
Initial patch (1.60 KB, patch)
2012-07-17 18:03 PDT, Ananta Iyengar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ananta Iyengar 2012-07-17 17:19:31 PDT
This can be reproduced in Chromium with the command line switch --enable-accelerated-video-decode
which enables accelerated video decoding.

With this flag on, if you navigate to http://w3schools.com/html5/tryit.asp?filename=tryhtml5_video_all
you can see that the video is flipped.

The flipped flag needs to be set to false in the CCVideoLayerImpl::appendQuads function in the CCVideoLayerImpl.cpp
file.

Will upload a patch in a bit.
Comment 1 Ananta Iyengar 2012-07-17 18:03:21 PDT
Created attachment 152893 [details]
Initial patch
Comment 2 Adrienne Walker 2012-07-17 18:06:04 PDT
What changed from https://bugs.webkit.org/show_bug.cgi?id=89189#c10? Was that just incorrect?

Also, that's unfortunate that there aren't any tests for hardware decode.  Why is that?
Comment 3 Ananta Iyengar 2012-07-17 18:09:53 PDT
No tests as yet as it is still under development and is manually enabled in Chrome via command line switches. Tests will come in soon hopefully.
Comment 4 Ananta Iyengar 2012-07-17 18:16:50 PDT
The elif defined(OS_WINDOWS) in that patch was wrong. For webkit we need to use OS(WINDOWS)
Comment 5 Adrienne Walker 2012-07-17 18:19:31 PDT
(In reply to comment #4)
> The elif defined(OS_WINDOWS) in that patch was wrong. For webkit we need to use OS(WINDOWS)

How did this work when you tested it locally, then?

(In reply to comment #3)
> No tests as yet as it is still under development and is manually enabled in Chrome via command line switches. Tests will come in soon hopefully.

Sorry to be reluctant, but I'm frustrated that this never worked.  If we had had tests in the first place, there'd be no need for this patch.

It seems like it shouldn't be that hard to add at least one layout test that draws a video with this patch.

You could either explicitly enable the hardware video path via a new function in Chromium's LayoutTestController.cpp (look for how we handle loseCompositorContext as an example).  Alternatively, you could create a (hypothetical) LayoutTests/platform/chromium/compositing/hardwarevideo/ directory and then modify TestShell::runFileTest to set these flags on anything in that directory.

Then I think you could just take a basic video layout test and run it through the hardware path.
Comment 6 Ananta Iyengar 2012-07-17 18:37:26 PDT
On Windows the VDA code uses the Microsoft media foundation API which brings with it COM and a dependency on Windows 7.

I agree that we need tests for this. I think a better place for this would be in content shell or something similar in Chromium where we can exercise this end to end. We have a basic test for the VDA stuff in the form of video_decode_accelerator_unittest.cc under src\content\common\gpu\media.

It would be great if we could land this patch. In any case this feature will not be enabled with comprehensive tests in place.
Comment 7 Ananta Iyengar 2012-07-17 18:58:02 PDT
Based on a discussion with Ami Fischman logged a webkit bug here https://bugs.webkit.org/show_bug.cgi?id=91569
Comment 8 Adrienne Walker 2012-07-17 19:34:55 PDT
Comment on attachment 152893 [details]
Initial patch

R=me.  Thanks for filing the bug about tests.
Comment 9 WebKit Review Bot 2012-07-18 15:20:16 PDT
Comment on attachment 152893 [details]
Initial patch

Clearing flags on attachment: 152893

Committed r123029: <http://trac.webkit.org/changeset/123029>
Comment 10 WebKit Review Bot 2012-07-18 15:20:29 PDT
All reviewed patches have been landed.  Closing bug.