Bug 116616

Summary: [BlackBerry] backface-visibility: hidden doesn't work properly with masks and filters
Product: WebKit Reporter: Arvid Nilsson <anilsson>
Component: WebKit BlackBerryAssignee: Arvid Nilsson <anilsson>
Status: RESOLVED FIXED    
Severity: Normal CC: anilsson, anlo, cgarcia, commit-queue, jpetsovits, rwlbuis, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 116604    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Arvid Nilsson 2013-05-22 07:18:49 PDT
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.
Comment 1 Arvid Nilsson 2013-05-24 03:51:27 PDT
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
Comment 2 Arvid Nilsson 2013-05-24 04:11:34 PDT
Created attachment 202800 [details]
Patch
Comment 3 Arvid Nilsson 2013-05-24 04:13:27 PDT
Comment on attachment 202800 [details]
Patch

Oops, I forgot to mention a PR number in the changelog.
Comment 4 Arvid Nilsson 2013-05-24 04:19:26 PDT
Created attachment 202801 [details]
Patch
Comment 5 Arvid Nilsson 2013-05-24 04:44:26 PDT
I think an informal review from jpetso or someone else who knows this stuff is needed before an upstream reviewer can r+ it.
Comment 6 Rob Buis 2013-05-27 08:09:29 PDT
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...
Comment 7 Jakob Petsovits 2013-05-27 08:35:36 PDT
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.
Comment 8 Arvid Nilsson 2013-05-27 09:07:02 PDT
(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...
Comment 9 Arvid Nilsson 2013-05-28 02:32:48 PDT
(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.
Comment 10 Arvid Nilsson 2013-05-28 02:36:51 PDT
Created attachment 203032 [details]
Patch
Comment 11 Arvid Nilsson 2013-05-28 03:29:05 PDT
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
Comment 12 Arvid Nilsson 2013-05-28 03:29:29 PDT
(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.
Comment 13 Carlos Garcia Campos 2013-05-28 10:06:07 PDT
Comment on attachment 203034 [details]
Patch

Looks a lot cleaner!
Comment 14 WebKit Commit Bot 2013-05-28 10:28:23 PDT
Comment on attachment 203034 [details]
Patch

Clearing flags on attachment: 203034

Committed r150809: <http://trac.webkit.org/changeset/150809>
Comment 15 WebKit Commit Bot 2013-05-28 10:28:26 PDT
All reviewed patches have been landed.  Closing bug.