Bug 68742

Summary: [chromium] Update VideoLayerChromium textures after texture resources are reclaimed
Product: WebKit Reporter: Adrienne Walker <enne>
Component: New BugsAssignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: enne, hclam, jamesr, mihaip, scherkus, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch jamesr: review+

Description Adrienne Walker 2011-09-23 15:57:02 PDT
[chromium] Update VideoLayerChromium textures after texture resources are reclaimed
Comment 1 Adrienne Walker 2011-09-23 17:22:14 PDT
Created attachment 108564 [details]
Patch
Comment 2 Adrienne Walker 2011-09-23 17:25:51 PDT
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. :(
Comment 3 James Robinson 2011-09-23 17:42:51 PDT
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
Comment 4 Vangelis Kokkevis 2011-09-23 17:58:46 PDT
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.
Comment 5 Vangelis Kokkevis 2011-09-23 18:00:02 PDT
(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.
Comment 6 Adrienne Walker 2011-09-26 07:43:40 PDT
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.
Comment 7 Adrienne Walker 2011-09-26 09:15:24 PDT
Created attachment 108676 [details]
Patch
Comment 8 Adrienne Walker 2011-09-26 09:42:24 PDT
(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.
Comment 9 Adrienne Walker 2011-09-26 09:43:22 PDT
Committed r95957: <http://trac.webkit.org/changeset/95957>
Comment 10 Mihai Parparita 2011-09-26 17:23:02 PDT
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
Comment 11 Adrienne Walker 2011-09-27 10:31:21 PDT
(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.  :)