Bug 39446 – Video elements show no video on Windows machines that do not support accelerated compositing

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

[Jer Noble]

Video elements show no video on Windows machines that do not support accelerated compositing

[Jer Noble]

rdar://problem/7999794

[Jer Noble]

Created attachment 56629 [details]
Patch

[Jer Noble]

Created attachment 56632 [details]
Patch

Patch was against an old version of the file; updated and regenerated the patch.  Hopefully, this will let the bots run correctly now.

[Jer Noble]

Created attachment 56634 [details]
Patch

[Jer Noble]

Committed r59871: <http://trac.webkit.org/changeset/59871>

[Eric Seidel (no email)]

Looks like this broke windows compile.  Sorry the win-ews bot has been down for so long, it just restarted running yesterday.

[Jer Noble]

Committed r59873: <http://trac.webkit.org/changeset/59873>

[Eric Carlson]

Problem not fixed by these changes.

[Jer Noble]

We are not creating the visual context at the right time, and in addition, we are creating a CAImageQueue-based visual context even when the MediaPlayer is non-accelerated.

[Jer Noble]

Created attachment 57138 [details]
Patch (Part2)

[WebKit Review Bot]

Attachment 57138 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/win/QTMovieVisualContext.cpp:87:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/win/QTMovieVisualContext.cpp:88:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:991:  Mismatching spaces inside () in if  [whitespace/parens] [5]
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:1004:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 57138 [details]
Patch (Part2)

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 60251)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,23 @@
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +		Video elements show no video on Windows machines that do not support accelerated compositing
> +		https://bugs.webkit.org/show_bug.cgi?id=39446
> +		rdar://problem/7999794
> +		

TABS!


> +    CFDictionaryRef options = 0;
> +    // If CAImageQueue prerequisites are not satisfied, pass in visual context pixelbuffer
> +    // options which will instruct the visual context to generate CGImage compatible 
> +    // pixel buffers (i.e. RGBA).
> +    if (!requiredDllsAvailable() || preferredMode != MediaRenderingMovieLayer )

Extra space.

> +        options = QTMovieVisualContext::getCGImageOptions();
> +    m_visualContext = adoptRef(new QTMovieVisualContext(m_visualContextClient.get(), options));

This should use a create() method.

> +    m_visualContext->setMovie(m_movie.get());
>  }


>  QTMovieVisualContextPriv::~QTMovieVisualContextPriv()
>  {
> +	if (m_visualContext)
> +		QTVisualContextSetImageAvailableCallback(m_visualContext, 0, 0);
TABS!


Other than the nits. r=me.

[Jer Noble]

Created attachment 57182 [details]
Patch

[Adele Peterson]

Comment on attachment 57182 [details]
Patch

When testing with hardware acceleration turned off in D3D, this crashed several times.  Posting a new patch that I think should address the issues.

[Adele Peterson]

Created attachment 57190 [details]
updated patch

This should really be reviewed by someone who knows this code.

[Adele Peterson]

hmm a more accurate description in the ChangeLog would be "Patch edited by Adele Peterson and Mark Rowe"

[Eric Carlson]

http://trac.webkit.org/changeset/60272

[WebKit Review Bot]

Comment on attachment 57138 [details]
Patch (Part2)

Attachment 92543 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8590160

New failing tests:
http/tests/websocket/tests/url-with-nonascii-query.html

[Eric Carlson]

(In reply to comment #18)
> (From update of attachment 57138 [details])
> Attachment 92543 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8590160
> 
> New failing tests:
> http/tests/websocket/tests/url-with-nonascii-query.html

This patch causes an unrelated test to fail a year after it was submitted?

[Eric Seidel (no email)]

I suspect the patch flaked twice in a row.  We have flaky test detection on all of the bots, but that just means they run the test multiple times... if it flakes multiple times in a row, there is little we can do. :(

[Eric Seidel (no email)]

Actually, this was a differnet bug.  the cr-linux-ews was briefly posting results using the *bug* number when it meant to use the *patch* number.  This can be completely ignored.  Sorry for the spam and confusion!