Bug 114276 – [BlackBerry] LayerTexture refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Arvid Nilsson]

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

[Arvid Nilsson]

Created attachment 197268 [details]
Patch

[Jakob Petsovits]

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.

[Arvid Nilsson]

(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.

[Arvid Nilsson]

(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.

[Arvid Nilsson]

Filed PR 325040 to investigate setting the opaque hint some time in the future.

[Arvid Nilsson]

Created attachment 197530 [details]
Patch

[Arvid Nilsson]

(In reply to comment #6)
> Created an attachment (id=197530) [details]
> Patch

Same patch as last time, only now it applies =)

[Carlos Garcia Campos]

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.

[Arvid Nilsson]

Created attachment 197566 [details]
Patch

[Arvid Nilsson]

Comment on attachment 197566 [details]
Patch

Adressed cgarcia's comments, straight to cq+

[WebKit Commit Bot]

Comment on attachment 197566 [details]
Patch

Clearing flags on attachment: 197566

Committed r148192: <http://trac.webkit.org/changeset/148192>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.