Bug 116616 – [BlackBerry] backface-visibility: hidden doesn't work properly with masks and filters

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Arvid Nilsson]

When we switched from Skia to BlackBerry::Platform::Graphics::GraphicsContext, we switched the winding order for culling, since the BB::P::G::GC uses clockwise geometry and requires glFrontFace(GL_CW). However, masks and filters still use counterclockwise geometry as we used to do with Skia. So they get erroneously culled out when -webkit-backface-visibility: hidden is applied.

[Arvid Nilsson]

To further complicate matters, a right-side-up transform is used when rendering layers into surfaces, so if we switch the winding order of layer geometry, instead of erroneously culling the surface when drawn to the screen, it will cull the layer when it's drawn into the surface =D

[Arvid Nilsson]

Created attachment 202800 [details]
Patch

[Arvid Nilsson]

Comment on attachment 202800 [details]
Patch

Oops, I forgot to mention a PR number in the changelog.

[Arvid Nilsson]

Created attachment 202801 [details]
Patch

[Arvid Nilsson]

I think an informal review from jpetso or someone else who knows this stuff is needed before an upstream reviewer can r+ it.

[Rob Buis]

Comment on attachment 202801 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:292
> +        static float texcoords[4 * 2] = { 0, 0,  1, 0,  1, 1,  0, 1 };

We can probably make these global in the cpp, like in LayerFilterRenderer.cpp

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:165
> +    FloatPoint origin() const { return FloatPoint(m_bounds.width() / 2.0f, m_bounds.height() / 2.0f); }

I wonder if FloatPoint::center would make sense...

[Jakob Petsovits]

Comment on attachment 202801 [details]
Patch

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

The code looks good to me. Seconding Rob's comments.

> Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:336
>      // If some layers should be drawed on temporary surfaces, we should do it first.
> -    drawLayersOnSurfaces(surfaceLayers);
> +    if (!surfaceLayers.isEmpty())
> +        drawLayersOnSurfaces(surfaceLayers);

If you're changing this code, you might want to consider also correcting the grammatical mistake in the comment.

> Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:503
>      // If there are layers drawed on surfaces, we need to switch to default framebuffer.
>      // Otherwise, we just need to set viewport.

More "drawed" that could be drawn instead. Maybe the comment should now be rephrased anyway though, because we switch back to the default framebuffer unconditionally if the function is called at all.

[Arvid Nilsson]

(In reply to comment #6)
> (From update of attachment 202801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202801&action=review
> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:292
> > +        static float texcoords[4 * 2] = { 0, 0,  1, 0,  1, 1,  0, 1 };
> 
> We can probably make these global in the cpp, like in LayerFilterRenderer.cpp


The texture coordinates will depend on the geometry after clipping in a future patch, so there's no point in moving this declaration around right now. There will be a LayerCompositingThread::textureCoordinates() accessor that replaces this code later in the week.

> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:165
> > +    FloatPoint origin() const { return FloatPoint(m_bounds.width() / 2.0f, m_bounds.height() / 2.0f); }
> 
> I wonder if FloatPoint::center would make sense...

[Arvid Nilsson]

(In reply to comment #6)
> (From update of attachment 202801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202801&action=review
> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:292
> > +        static float texcoords[4 * 2] = { 0, 0,  1, 0,  1, 1,  0, 1 };
> 
> We can probably make these global in the cpp, like in LayerFilterRenderer.cpp
> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:165
> > +    FloatPoint origin() const { return FloatPoint(m_bounds.width() / 2.0f, m_bounds.height() / 2.0f); }
> 
> I wonder if FloatPoint::center would make sense...

Apparently there is no FloatPoint::center method, I don't think this patch is the right time to add one.

[Arvid Nilsson]

Created attachment 203032 [details]
Patch

[Arvid Nilsson]

Created attachment 203034 [details]
Patch

Remove one redundant instance of 1.0 in favor of 1. In other places, the .0f is required to force floating point math, and is thus allowed according to style guidelines

[Arvid Nilsson]

(In reply to comment #11)
> Created an attachment (id=203034) [details]
> Patch
> 
> Remove one redundant instance of 1.0 in favor of 1. In other places, the .0f is required to force floating point math, and is thus allowed according to style guidelines

Thanks to cagarcia for spotting this.

[Carlos Garcia Campos]

Comment on attachment 203034 [details]
Patch

Looks a lot cleaner!

[WebKit Commit Bot]

Comment on attachment 203034 [details]
Patch

Clearing flags on attachment: 203034

Committed r150809: <http://trac.webkit.org/changeset/150809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.