Bug 114276

Summary: [BlackBerry] LayerTexture refactoring
Product: WebKit Reporter: Arvid Nilsson <anilsson>
Component: WebKit BlackBerryAssignee: Arvid Nilsson <anilsson>
Status: RESOLVED FIXED    
Severity: Normal CC: anilsson, anlo, cgarcia, commit-queue, jpetsovits, mifenton, rwlbuis, tonikitoo, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 95781    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Arvid Nilsson 2013-04-09 06:15:53 PDT
Simplify LayerTexture - it's overly complicated due to having supported both Skia and BlackBerry::Platform::Graphics::GraphicsContext for a while.

Also refactor the code for getting an actual OpenGL texture ID out of these LayerTexture things into LayerTexture to keep the code DRY. Add the required state save/restore incantations for when the texture is initially conjured out of thin air by drawing the GraphicsContext stuff to FBO.

PR 322882
Comment 1 Arvid Nilsson 2013-04-10 08:05:49 PDT
Created attachment 197268 [details]
Patch
Comment 2 Jakob Petsovits 2013-04-10 09:09:21 PDT
Comment on attachment 197268 [details]
Patch

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

Looks good to me.

> Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:-39
> -    , m_isOpaque(false)

With the disappearance of m_isOpaque, can we instead call BlackBerry::Platform::Graphics::setBufferOpaqueHint() in the appropriate place(s) so this information is not irreversibly lost? Or was there another reason that necessitates the wholesale removal of this concept? I guess we don't currently distinguish between opaque and non-opaque textures in the compositor to control the blend mode, although that's a pity.
Comment 3 Arvid Nilsson 2013-04-10 10:08:13 PDT
(In reply to comment #2)
> (From update of attachment 197268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197268&action=review
> 
> Looks good to me.
> 
> > Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:-39
> > -    , m_isOpaque(false)
> 
> With the disappearance of m_isOpaque, can we instead call BlackBerry::Platform::Graphics::setBufferOpaqueHint() in the appropriate place(s) so this information is not irreversibly lost? Or was there another reason that necessitates the wholesale removal of this concept? I guess we don't currently distinguish between opaque and non-opaque textures in the compositor to control the blend mode, although that's a pity.

That's a great idea, I'll add that tomorrow around where we used to detect the opacity instead of dropping it all together.
Comment 4 Arvid Nilsson 2013-04-11 02:09:16 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 197268 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=197268&action=review
> > 
> > Looks good to me.
> > 
> > > Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:-39
> > > -    , m_isOpaque(false)
> > 
> > With the disappearance of m_isOpaque, can we instead call BlackBerry::Platform::Graphics::setBufferOpaqueHint() in the appropriate place(s) so this information is not irreversibly lost? Or was there another reason that necessitates the wholesale removal of this concept? I guess we don't currently distinguish between opaque and non-opaque textures in the compositor to control the blend mode, although that's a pity.
> 
> That's a great idea, I'll add that tomorrow around where we used to detect the opacity instead of dropping it all together.

Unfortunately, setting the opacity hint breaks the CSS Filters, probably because platform changes the blend mode or other GL state in a way webkit isn't ready for. I prefer to do that change separately.
Comment 5 Arvid Nilsson 2013-04-11 02:19:06 PDT
Filed PR 325040 to investigate setting the opaque hint some time in the future.
Comment 6 Arvid Nilsson 2013-04-11 02:28:45 PDT
Created attachment 197530 [details]
Patch
Comment 7 Arvid Nilsson 2013-04-11 02:29:10 PDT
(In reply to comment #6)
> Created an attachment (id=197530) [details]
> Patch

Same patch as last time, only now it applies =)
Comment 8 Carlos Garcia Campos 2013-04-11 02:52:31 PDT
Comment on attachment 197530 [details]
Patch

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

Nice cleanup!

> Source/WebCore/ChangeLog:11
> +        now it's just BB::P::G::Buffer all around.

Better say BlackBerry::Platform::Graphics::Buffer to make it clearer, I think.

> Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:104
> +    // Force creation if it's 0

Nit: finish the comment with a period.

> Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:112
> +        platformTexture = reinterpret_cast<GLuint>(platformBufferHandle(m_buffer));

Shouldn't it be reinterpret_cast<Platform3DObject> instead?

> Source/WebCore/platform/graphics/blackberry/LayerTexture.h:61
> +    // This will ensure a buffer is allocated for this texture with the requested backing and size

Period here too.

> Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.cpp:110
> +        Buffer* buffer = createBuffer(size, type);
> +        if (!buffer)
>              return false;

I think createBuffer is not expected to return 0, there's an assert in such case, so maybe we can skip this check, or add an assert here too.

> Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.cpp:130
> +    Buffer* buffer = createBuffer(size, type);
> +    if (!buffer)
>          return false;

Ditto.

> Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:54
> +    PassRefPtr<LayerTexture> textureForContents(BlackBerry::Platform::Graphics::Buffer* contents);

I think we can omit the parameter name in this case.

> Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:58
> +    PassRefPtr<LayerTexture> updateContents(const RefPtr<LayerTexture>&, BlackBerry::Platform::Graphics::Buffer* contents);

Ditto.
Comment 9 Arvid Nilsson 2013-04-11 03:25:09 PDT
Created attachment 197566 [details]
Patch
Comment 10 Arvid Nilsson 2013-04-11 03:25:57 PDT
Comment on attachment 197566 [details]
Patch

Adressed cgarcia's comments, straight to cq+
Comment 11 WebKit Commit Bot 2013-04-11 04:10:16 PDT
Comment on attachment 197566 [details]
Patch

Clearing flags on attachment: 197566

Committed r148192: <http://trac.webkit.org/changeset/148192>
Comment 12 WebKit Commit Bot 2013-04-11 04:10:19 PDT
All reviewed patches have been landed.  Closing bug.