Bug 68742 – [chromium] Update VideoLayerChromium textures after texture resources are reclaimed

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

[Adrienne Walker]

[chromium] Update VideoLayerChromium textures after texture resources are reclaimed

[Adrienne Walker]

Created attachment 108564 [details]
Patch

[Adrienne Walker]

http://code.google.com/p/chromium/issues/detail?id=95226

After a visibility change, textures aren't getting updated during the next commit. Added a test that demonstrates this.

I also noticed that an RGB video (with one plane) would get skipped in pushPropertiesTo.  Unfortunately, there is no RGB video testing (see http://crbug.com/97847), so this is just a speculative fix. :(

[James Robinson]

Comment on attachment 108564 [details]
Patch

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

R=me

> Source/WebCore/ChangeLog:16
> +        Additionally, fix a small bug in pushPropertiesTo where 1 plane RGB
> +        videos would not get drawn because all 3 planes didn't have valid
> +        textures.

do we ever actually have such a thing? can we just kill this codepath and always use the YUV path?

> Source/WebCore/ChangeLog:18
> +        Reviewed by NOBODY (OOPS!).

I dunno how you got 2 "reviewed by" lines here but I expect it'll kill some script somewhere

[Vangelis Kokkevis]

Comment on attachment 108564 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:86
> +    if (m_dirtyRect.isEmpty() && texturesValid())

Don't we need to reserve the textures to prevent them from being recycled before we get to use them?

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:150
> +    for (unsigned i = m_planes; i < MaxPlanes; ++i)

This can also be moved to updateCompositorResources() and only be run if the number of planes changed.

[Vangelis Kokkevis]

(In reply to comment #4)
> (From update of attachment 108564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108564&action=review
> 
> > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:86
> > +    if (m_dirtyRect.isEmpty() && texturesValid())
> 
> Don't we need to reserve the textures to prevent them from being recycled before we get to use them?
> 
> > Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:150
> > +    for (unsigned i = m_planes; i < MaxPlanes; ++i)
> 
> This can also be moved to updateCompositorResources() and only be run if the number of planes changed.

Sorry I accidentally reverted the r+ you got from James.  But I do think the lack of a reserve here is an issue.

[Adrienne Walker]

Comment on attachment 108564 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +        textures.
> 
> do we ever actually have such a thing? can we just kill this codepath and always use the YUV path?

I had the same thought and talked to hclam about this.  Apparently this codepath is still used in some places, but isn't being tested.  (Sad trombone noise.)  I had hclam file http://crbug.com/97847 to fix this state of affairs.

>>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:86
>>> +    if (m_dirtyRect.isEmpty() && texturesValid())
>> 
>> Don't we need to reserve the textures to prevent them from being recycled before we get to use them?
> 
> Sorry I accidentally reverted the r+ you got from James.  But I do think the lack of a reserve here is an issue.

Great catch, thanks!

>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:150
>> +    for (unsigned i = m_planes; i < MaxPlanes; ++i)
> 
> This can also be moved to updateCompositorResources() and only be run if the number of planes changed.

I wanted this here so that I could explicitly clear the texture ids and stale sizes on the compositor thread so that they wouldn't get accidentally used.  I think it's slightly more simple here, but it doesn't seem like a big issue.

[Adrienne Walker]

Created attachment 108676 [details]
Patch

[Adrienne Walker]

(In reply to comment #7)
> Created an attachment (id=108676) [details]
> Patch

Oops.  There's an obvious logic typo in the "m_skipsDraw = false" line I added if the reserve code fails.  I'll fix that on landing.

[Adrienne Walker]

Committed r95957: <http://trac.webkit.org/changeset/95957>

[Mihai Parparita]

BTW, the new test was failing due to very slight image diffs on Windows/Mesa (http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=video-page-visibility.html&showExpectations=true), so I added Windows GPU baselines with http://trac.webkit.org/changeset/96048

[Adrienne Walker]

(In reply to comment #10)
> BTW, the new test was failing due to very slight image diffs on Windows/Mesa (http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=video-page-visibility.html&showExpectations=true), so I added Windows GPU baselines with http://trac.webkit.org/changeset/96048

Thanks, Mihai.  :)