Bug 117067 – [BlackBerry] Accelerated compositing layers that intersect the image plane are incorrectly transformed

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Arvid Nilsson]

The BlackBerry port mathematically projects each corner of each layer bounds rectangle, which gives the wrong results when the layer intersects the image plane.

For display lists, we still get the correct appearance of the layer contents, since the display list results in geometry that's transformed in the vertex shader, on the GPU.

However, the transformed bounds are used for drawing the debug border, performing visibility calculations on the CPU and drawing of WebGL, masks/reflections and filters. Using the mathematical projection of points that lie behind the image plane gives the wrong results. The easiest way to visualise the error is to just enable the debug border and make some layers intersect the image plane, e.g. the clouds on https://developer.mozilla.org/en-US/demos/detail/the-box/launch.

The most important consequence of this bug is that the wrong area of the layer will be considered visible, so a tiled layer will not populate the right tiles (or sometimes, any tiles at all).

However, this is reasonably easy to fix - the recipe can be found in section 6.2 of http://www.w3.org/TR/css3-transforms.

[Arvid Nilsson]

Created attachment 203430 [details]
Some clouds with matching bounding boxes

[Arvid Nilsson]

Created attachment 203431 [details]
Some clouds with broken bounding boxes, visualising the bug

[Arvid Nilsson]

Created attachment 203433 [details]
Patch

[Arvid Nilsson]

Andrew just added a setting in the browser GUI to enable the AC layer debug borders, so this bug is now much easier to reproduce and the fix easier to verify.

[Jakob Petsovits]

Comment on attachment 203433 [details]
Patch

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

Slightly complex, but looks alright to me. Informal r+ and bonus kudos for writing clear code to tackle a not-so-straightforward problem.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:207
> +        static FloatPoint data[4] = { FloatPoint(0, 1),  FloatPoint(1, 1),  FloatPoint(1, 0),  FloatPoint(0, 0) };
> +        static Vector<FloatPoint>* coordinates = 0;

For a moment I was wondering whether this will always return the same coordinates even when called with a different value for upsideDown, because it's initialized only when coordinates is 0.
Then I realized that data and coordinates only exist in this scope (within "if (upsideDown)") and therefore they are two distinct pairs of static variables, even though they bear the same name.
Putting different prefixes on the variable names would help avoid this confusion, imho.

[Arvid Nilsson]

Created attachment 204500 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 204500 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/EGLImageLayerCompositingThreadClient.cpp:58
> +    glVertexAttribPointer(program.texCoordLocation(), 2, GL_FLOAT, GL_FALSE, 0, layer->textureCoordinates(true /*upsideDown*/).data());

You could use an enum instead of a bool and you don't need a comment to make the code readable :-)

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:157
> +        polygon = intersect(quad, LayerClipPlane(FloatPoint3D(matrix.m14(), matrix.m24(), matrix.m34()), matrix.m44() - epsilon));

Could this be Vector<FloatPoint3D, 4> polygon = ...?

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:159
> +        // Compute the clipped texture coordinates

Nit: finish comment with a period.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:172
> +            p = multVecMatrix(matrix, p, w);

This looks a bit weird to me because p is a reference, maybe we could do:

FloatPoint3D p = multVecMatrix(matrix, polygon[i], w);

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:176
> +                p.setX(p.x() / w);
> +                p.setY(p.y() / w);
> +                p.setZ(p.z() / w);

This could be p.scale(1 / w); Why not reusing the existing TransformationMatrix::multVecMatrix()?

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:207
> +        static FloatPoint data[4] = { FloatPoint(0, 1),  FloatPoint(1, 1),  FloatPoint(1, 0),  FloatPoint(0, 0) };
> +        static Vector<FloatPoint>* upsideDownCoordinates = 0;

I'm not sure making data static is a good idea, since the points are going to be copied to the vector and the vector is static, we will keep two copies always in memory, no?

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:216
> +    static FloatPoint data[4] = { FloatPoint(0, 0),  FloatPoint(1, 0),  FloatPoint(1, 1),  FloatPoint(0, 1) };
> +    static Vector<FloatPoint>* coordinates = 0;

Ditto.

[Arvid Nilsson]

Comment on attachment 204500 [details]
Patch

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

>> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:176
>> +                p.setZ(p.z() / w);
> 
> This could be p.scale(1 / w); Why not reusing the existing TransformationMatrix::multVecMatrix()?

The reason I did it like this, is I want the improved numeric stability of dividing p.x() with w instead of multiplying by 1/w. I thought 1/w would lose precision, but perhaps a floating point math expert can tell me otherwise.

The reason I can't use the existing TransformationMatrix::multVecMatrix() is because it discards the w:s, and we need those in the next patch to perform unprojection.

>> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:207
>> +        static Vector<FloatPoint>* upsideDownCoordinates = 0;
> 
> I'm not sure making data static is a good idea, since the points are going to be copied to the vector and the vector is static, we will keep two copies always in memory, no?

Unfortunately, there's no way to make the Vector be the only copy of the data... And it's important that the common cases of full-rect texturing should use static data instead of being allocated on each call. I agree this code is a bit clunky, but this was the only way I could find of having the cake (static full-rect texture coordinate data) and eating it too (being able to return non-full-rect coordinate data in case of clipping).

[Arvid Nilsson]

Created attachment 204691 [details]
Patch

[Arvid Nilsson]

Comment on attachment 204691 [details]
Patch

Wrong again

[Arvid Nilsson]

Created attachment 204692 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 204692 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:170
> +            if (w != 1) {

You should probably also check w != 0 to a void a zero divison.

[Arvid Nilsson]

Comment on attachment 204692 [details]
Patch

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

>> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:170
>> +            if (w != 1) {
> 
> You should probably also check w != 0 to a void a zero divison.

Thanks for the review =) Due to the construction above, where any point with w < epsilon is clipped to w = epsilon, w can never be 0 here.

[WebKit Commit Bot]

Comment on attachment 204692 [details]
Patch

Clearing flags on attachment: 204692

Committed r151591: <http://trac.webkit.org/changeset/151591>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.