Bug 93677 – [chromium] Add software bitmap resources to CCResourceProvider

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

[Alexandre Elias]

[chromium] Add software bitmap resources to CCResourceProvider

[Alexandre Elias]

Created attachment 157613 [details]
Patch

[Alexandre Elias]

This approach doesn't have anything in the way of static type safety, but it seems hard to avoid that given the desired model.  In practice, since a single CCRenderer subclass will use most of the resources, it shouldn't come up that often though.

I'll write tests before landing.

[Antoine Labour]

Comment on attachment 157613 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:323
> +    , m_creationPolicy(CreateGLTextures)

Should we choose this based on the context?

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:48
> +class ScopedWriteLockGL;

You moved the definitions inside of CCResourceProvider, so these forward declarations are superfluous / wrong.

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:87
> +    ResourceId createBitmap(int pool, const IntSize&);

It would probably be useful to add a resourceType(ResourceID). That kinda calls CreationPolicy to be renamed ResourceType, and setCreationPolicy to setDefaultResourceType?

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:154
> +    class ScopedWriteLockSoftware {

Are we going to be using this in layers? If yes, can we make it work on texture-based resources too (e.g. add a pixel backing sore on the lock, and upload on the unlock), so that we don't need to have 2 paths based on the resource type?

[Antoine Labour]

For tests, maybe we can reuse existing tests and have a way to execute twice, once with each policy?

[Alexandre Elias]

Created attachment 157627 [details]
Patch

Introduce ResourceType and remove forward declarations

[Alexandre Elias]

> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:323
> > +    , m_creationPolicy(CreateGLTextures)
> 
> Should we choose this based on the context?

Possibly; I'll change this in a later patch when it becomes feasible to run with a different policy.

> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:154
> > +    class ScopedWriteLockSoftware {
> 
> Are we going to be using this in layers? If yes, can we make it work on texture-based resources too (e.g. add a pixel backing sore on the lock, and upload on the unlock), so that we don't need to have 2 paths based on the resource type?

This isn't intended for use by layers, CCRenderer will be the primary user.

> For tests, maybe we can reuse existing tests and have a way to execute twice, once with each policy?

Seems reasonable, I'll take a look.

[Alexandre Elias]

Created attachment 158670 [details]
Patch

Added unit tests and rewrote upload() to use SkCanvas::writePixels

[Alexandre Elias]

Created attachment 159182 [details]
Patch

Rebased on Bug 93524

[Alexandre Elias]

Created attachment 159246 [details]
Patch

Rebased

[Adrienne Walker]

Comment on attachment 159246 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:262
> +        uint8_t* pixels;

Is it possible to use OwnArrayPtr<uint8> here?

[Alexandre Elias]

(In reply to comment #10)
> (From update of attachment 159246 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159246&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:262
> > +        uint8_t* pixels;
> 
> Is it possible to use OwnArrayPtr<uint8> here?

I'm running into hashmap-related errors.  It looks like there's a trick to allow OwnPtrs in hashmaps, but only if they're at the top level (not in a struct).  

We could explore various ways of tackling this, but it seems like the details will change when we move away from WTF data structures, and also ResourceProvider needs to manage explicit create/delete calls for GL resources anyway.

[Alexandre Elias]

Created attachment 159522 [details]
Patch

Rebased

[Adrienne Walker]

Can you run double check that webkit_unit_tests passes in a debug build? I see an assert with your patch.

[ RUN      ] CCLayerTreeHostImplTest.dontUseOldResourcesAfterLostContext
ASSERTION FAILED: m_textureId
../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp(272) : WebCore::CCResourceProvider::ScopedReadLockGL::ScopedReadLockGL(WebCore::CCResourceProvider*, unsigned int)

[Alexandre Elias]

(In reply to comment #13)
> Can you run double check that webkit_unit_tests passes in a debug build? I see an assert with your patch.
> 
> [ RUN      ] CCLayerTreeHostImplTest.dontUseOldResourcesAfterLostContext
> ASSERTION FAILED: m_textureId
> ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp(272) : WebCore::CCResourceProvider::ScopedReadLockGL::ScopedReadLockGL(WebCore::CCResourceProvider*, unsigned int)

Hmm, this is because I added a new assertion that we don't try to lock a zero textureId.  The root cause is in CCTextureLayerImpl:

void CCTextureLayerImpl::didLoseContext()
{
    m_textureId = 0;
}

then it goes ahead and creates an external texture resource with that ID.  What would be the best place to fix that?

[Alexandre Elias]

Created attachment 159551 [details]
Patch

Fix an upload bug when imageRect.location != 0, and add a new test case for it.

[Dana Jansens]

(In reply to comment #14)
> Hmm, this is because I added a new assertion that we don't try to lock a zero textureId.  The root cause is in CCTextureLayerImpl:
> 
> void CCTextureLayerImpl::didLoseContext()
> {
>     m_textureId = 0;
> }
> 
> then it goes ahead and creates an external texture resource with that ID.  What would be the best place to fix that?

Hm, probably early-out in willDraw(), and then early out in appendQuads and didDraw if !m_externalTextureResource

[Alexandre Elias]

Created attachment 159597 [details]
Patch

Fix another assertion failure in CCResourceProviderTest by checking for glId != 0 in deleteResource()

[Adrienne Walker]

Comment on attachment 159597 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 159597 [details]
Patch

Clearing flags on attachment: 159597

Committed r126202: <http://trac.webkit.org/changeset/126202>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94657

[Alexandre Elias]

Created attachment 159842 [details]
Patch

Change back to initializing m_maxTextureSize to 0 in constructor, since glGetIntegerv asserts that it receives only zero values.

[Adrienne Walker]

Comment on attachment 159842 [details]
Patch

R=me, assuming you've tested directly that this fixes the problem.  I'm also leaving CQ+ for you to set yourself after you land 94266.

[WebKit Review Bot]

Comment on attachment 159842 [details]
Patch

Clearing flags on attachment: 159842

Committed r126351: <http://trac.webkit.org/changeset/126351>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.