[chromium] Add software bitmap resources to CCResourceProvider
Created attachment 157613 [details] Patch
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 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?
For tests, maybe we can reuse existing tests and have a way to execute twice, once with each policy?
Created attachment 157627 [details] Patch Introduce ResourceType and remove forward declarations
> > 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.
Created attachment 158670 [details] Patch Added unit tests and rewrote upload() to use SkCanvas::writePixels
Created attachment 159182 [details] Patch Rebased on Bug 93524
Created attachment 159246 [details] Patch Rebased
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?
(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.
Created attachment 159522 [details] Patch Rebased
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)
(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?
Created attachment 159551 [details] Patch Fix an upload bug when imageRect.location != 0, and add a new test case for it.
(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
Created attachment 159597 [details] Patch Fix another assertion failure in CCResourceProviderTest by checking for glId != 0 in deleteResource()
Comment on attachment 159597 [details] Patch R=me.
Comment on attachment 159597 [details] Patch Clearing flags on attachment: 159597 Committed r126202: <http://trac.webkit.org/changeset/126202>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 94657
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 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 on attachment 159842 [details] Patch Clearing flags on attachment: 159842 Committed r126351: <http://trac.webkit.org/changeset/126351>