Bug 120448 – [Windows] Video inside a web page always falls back to non-hardware accelerated

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Brent Fulgham]

If you render a web page that embeds a video element, WebKit is turning off compositing mode forcing the system to render through the non-hardware accelerated path.

This is happening because RenderLayerCompositor::computeCompositingRequirements initially decides to turn compositing off (since the media element is usually represented as a static image before playing).

Once the user interacts with the media element (to scrub video or play the movie), RenderLayerCompositor::computeCompositingRequirements recognizes that it needs to begin compositing and switches to the correct mode.  However, the media player element has already been constructed in "non-hardware accelerated mode", and continues its lifetime without using hardware acceleration.

We need to do one of the following:
1. When the render tree switches to compositing mode, destroy/recreate the player
-or-
2. Somehow attache the D3D device to the player after construction?

[Radar WebKit Bug Importer]

<rdar://problem/14863525>

[Brent Fulgham]

*** Bug 120447 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

Created attachment 210119 [details]
Patch

[Eric Carlson]

Comment on attachment 210119 [details]
Patch

Nice simple fix!

[Brent Fulgham]

Unscientific numbers:

Before patch:
Video playback consumed ~5% CPU on test page.

After patch:
Video playback consumed ~1% CPU on test page.

[Brent Fulgham]

Committed r154890: <http://trac.webkit.org/changeset/154890>

[Simon Fraser (smfr)]

Comment on attachment 210119 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:415
> +        player()->frameView()->enterCompositingMode();

Layering violation :(

[Brent Fulgham]

(In reply to comment #7)
> (From update of attachment 210119 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210119&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:415
> > +        player()->frameView()->enterCompositingMode();
> 
> Layering violation :(

Jer and I were discussing this, too.

We could add a mediaPlayerClient method to handle this logic to avoid this happening.

[Brent Fulgham]

This has triggered a regression.  The contents around the video are not being repainted properly after flipping into compositing mode.

[Brent Fulgham]

Created attachment 210143 [details]
Patch

[Brent Fulgham]

Created attachment 210155 [details]
Patch

[Eric Carlson]

Comment on attachment 210155 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2076
> +        return video->requiresImmediateCompositing() || (video->shouldDisplayVideo() && canAccelerateVideoRendering(video));

Parens would make this clearer: 

return (video->requiresImmediateCompositing() || (video->shouldDisplayVideo()) && canAccelerateVideoRendering(video));

> Source/WebCore/rendering/RenderVideo.cpp:287
> +    MediaPlayer* p = mediaElement()->player();
> +    if (p)

This can be done on a single line.

[Simon Fraser (smfr)]

Comment on attachment 210155 [details]
Patch

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

Is this testable?

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:2076
>> +        return video->requiresImmediateCompositing() || (video->shouldDisplayVideo() && canAccelerateVideoRendering(video));
> 
> Parens would make this clearer: 
> 
> return (video->requiresImmediateCompositing() || (video->shouldDisplayVideo()) && canAccelerateVideoRendering(video));

WebKit style is to not add the outer parens.

[Brent Fulgham]

Created attachment 210165 [details]
Patch

[Darin Adler]

Comment on attachment 210165 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.h:88
> +    virtual bool requiresImmediateCompositing() const { return true; }

I’d like you to use OVERRIDE.

I’d like to see a comment explaining why this player requires this; the comment in the change log is good, but I think it should be in the code too. To have a good place for the comment, I suggest not inlining this, since virtual calls won’t be inlined anyway.

> Source/WebCore/rendering/RenderVideo.cpp:287
> +    MediaPlayer* p = mediaElement()->player();
> +    return p ? p->requiresImmediateCompositing() : false;

I would write it like this:

    MediaPlayer* player = mediaElement()->player();
    return player && player->requiresImmediateCompositing();

[Darin Adler]

Did you see my comments? Looks like you landed this as-is and decided not to do the things I mentioned in the comments.

[Brent Fulgham]

(In reply to comment #16)
> Did you see my comments? Looks like you landed this as-is and decided not to do the things I mentioned in the comments.

I'm making the changes now.  I had set "webkit-patch land" to go after simon reviewed it, and didn't see your review until I had hit <enter>.  Whoops!

[Brent Fulgham]

Landed in two steps:
http://trac.webkit.org/changeset/154914
http://trac.webkit.org/changeset/154915