Bug 72760 – [chromium] compositing/masks layout tests fail with accelerated drawing

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alok Priyadarshi]

The mask is rendered upside down. Compositor assumes that the textures are top-down (opposite to OpenGL convention). The accelerated path creates textures in bottom-up orientation, which gets flipped in the shader and then flipped again by the projection matrix.

The mask does not go through the shader and projection matrix, so it does not get a chance to end up in top-down orientation. I think it will be better to just always have textures in bottom-up orientation.

[Alok Priyadarshi]

Created attachment 118278 [details]
proposed patch

[James Robinson]

Comment on attachment 118278 [details]
proposed patch

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

> Source/WebCore/ChangeLog:17
> +        Ideally the compositor should not do so many flips. It is confusing and unnecessary.
> +        If the textures are always in the bottom-up orientation then we also do not need to distinguish between whether we are rendering to an FBO (render-to-texture) or the window.
> +        I tried removing all the flips, but the computation in the tiler becomes a little involved. I still think we should try to remove all the flips and will get back to it eventually.
> +        

I'm not sure if this section is describing a future improvement or not. Could you file such a thing as a bug rather than as commentary in a ChangeLog?  The latter is not very actionable.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:-217
> -    // this = this * mat

the changes in this file seem unrelated to this patch. could you make this a separate patch?

[James Robinson]

This bug title isn't very descriptive - it looks like what's actually going on here is you are moving y-flips to a different place. Could you please update the bug title and description to make it more obvious what is going on?  In particular, the vast majority of this patch is not specific to accelerated drawing.

[WebKit Review Bot]

Comment on attachment 118278 [details]
proposed patch

Attachment 118278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10806003

[Alok Priyadarshi]

Created attachment 118327 [details]
proposed patch

- Removed unrelated changes
- Fixed compile error
- Better ChangeLog

[Alok Priyadarshi]

Comment on attachment 118278 [details]
proposed patch

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

>> Source/WebCore/ChangeLog:17
>> +        
> 
> I'm not sure if this section is describing a future improvement or not. Could you file such a thing as a bug rather than as commentary in a ChangeLog?  The latter is not very actionable.

https://bugs.webkit.org/show_bug.cgi?id=74052

>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:-217
>> -    // this = this * mat
> 
> the changes in this file seem unrelated to this patch. could you make this a separate patch?

DONE.

[Alok Priyadarshi]

(In reply to comment #3)
> This bug title isn't very descriptive - it looks like what's actually going on here is you are moving y-flips to a different place. Could you please update the bug title and description to make it more obvious what is going on?  In particular, the vast majority of this patch is not specific to accelerated drawing.

I think the bug title is pretty accurate. The bug title simply states what the problem is. The following description and ChangeLog describe the reasons and solution. If you have any suggestions, I would be happy to incorporate.

This patch is very specific to accelerated drawing. Yes it is moving the y-flip to a different location, but only for the accelerated drawing path. Now that both - software and accelerated path produce texture in the same orientation, there is no need for LayerTextureUpdater::Orientation.

[Alok Priyadarshi]

ping!

[Stephen White]

Comment on attachment 118327 [details]
proposed patch

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 118327 [details]
proposed patch

Clearing flags on attachment: 118327

Committed r102698: <http://trac.webkit.org/changeset/102698>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.