Bug 93677

Summary: [chromium] Add software bitmap resources to CCResourceProvider
Product: WebKit Reporter: Alexandre Elias <aelias>
Component: New BugsAssignee: Alexandre Elias <aelias>
Status: RESOLVED FIXED    
Severity: Normal CC: aelias, cc-bugs, danakj, enne, epenner, jamesr, nduca, piman, reveman, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 94550, 94657    
Bug Blocks: 93761    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexandre Elias 2012-08-09 19:07:15 PDT
[chromium] Add software bitmap resources to CCResourceProvider
Comment 1 Alexandre Elias 2012-08-09 19:20:42 PDT
Created attachment 157613 [details]
Patch
Comment 2 Alexandre Elias 2012-08-09 19:23:11 PDT
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.
Comment 3 Antoine Labour 2012-08-09 20:28:21 PDT
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?
Comment 4 Antoine Labour 2012-08-09 20:29:03 PDT
For tests, maybe we can reuse existing tests and have a way to execute twice, once with each policy?
Comment 5 Alexandre Elias 2012-08-09 20:48:36 PDT
Created attachment 157627 [details]
Patch

Introduce ResourceType and remove forward declarations
Comment 6 Alexandre Elias 2012-08-09 20:52:56 PDT
> > 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.
Comment 7 Alexandre Elias 2012-08-15 17:32:02 PDT
Created attachment 158670 [details]
Patch

Added unit tests and rewrote upload() to use SkCanvas::writePixels
Comment 8 Alexandre Elias 2012-08-17 12:46:34 PDT
Created attachment 159182 [details]
Patch

Rebased on Bug 93524
Comment 9 Alexandre Elias 2012-08-17 17:59:29 PDT
Created attachment 159246 [details]
Patch

Rebased
Comment 10 Adrienne Walker 2012-08-20 13:18:00 PDT
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?
Comment 11 Alexandre Elias 2012-08-20 14:16:17 PDT
(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.
Comment 12 Alexandre Elias 2012-08-20 14:28:32 PDT
Created attachment 159522 [details]
Patch

Rebased
Comment 13 Adrienne Walker 2012-08-20 15:03:56 PDT
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)
Comment 14 Alexandre Elias 2012-08-20 15:59:19 PDT
(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?
Comment 15 Alexandre Elias 2012-08-20 16:01:41 PDT
Created attachment 159551 [details]
Patch

Fix an upload bug when imageRect.location != 0, and add a new test case for it.
Comment 16 Dana Jansens 2012-08-20 16:06:07 PDT
(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
Comment 17 Alexandre Elias 2012-08-20 20:02:57 PDT
Created attachment 159597 [details]
Patch

Fix another assertion failure in CCResourceProviderTest by checking for glId != 0 in deleteResource()
Comment 18 Adrienne Walker 2012-08-21 09:04:05 PDT
Comment on attachment 159597 [details]
Patch

R=me.
Comment 19 WebKit Review Bot 2012-08-21 15:59:48 PDT
Comment on attachment 159597 [details]
Patch

Clearing flags on attachment: 159597

Committed r126202: <http://trac.webkit.org/changeset/126202>
Comment 20 WebKit Review Bot 2012-08-21 15:59:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Review Bot 2012-08-21 17:58:41 PDT
Re-opened since this is blocked by 94657
Comment 22 Alexandre Elias 2012-08-21 18:49:22 PDT
Created attachment 159842 [details]
Patch

Change back to initializing m_maxTextureSize to 0 in constructor, since glGetIntegerv asserts that it receives only zero values.
Comment 23 Adrienne Walker 2012-08-22 10:27:01 PDT
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.
Comment 24 WebKit Review Bot 2012-08-22 14:26:23 PDT
Comment on attachment 159842 [details]
Patch

Clearing flags on attachment: 159842

Committed r126351: <http://trac.webkit.org/changeset/126351>
Comment 25 WebKit Review Bot 2012-08-22 14:26:28 PDT
All reviewed patches have been landed.  Closing bug.