Bug 72760 - [chromium] compositing/masks layout tests fail with accelerated drawing
Summary: [chromium] compositing/masks layout tests fail with accelerated drawing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on: 73247
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-18 14:48 PST by Alok Priyadarshi
Modified: 2011-12-13 13:13 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (13.82 KB, patch)
2011-12-07 14:24 PST, Alok Priyadarshi
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
proposed patch (13.42 KB, patch)
2011-12-07 21:43 PST, Alok Priyadarshi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2011-11-18 14:48:49 PST
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.
Comment 1 Alok Priyadarshi 2011-12-07 14:24:19 PST
Created attachment 118278 [details]
proposed patch
Comment 2 James Robinson 2011-12-07 16:35:07 PST
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?
Comment 3 James Robinson 2011-12-07 16:35:56 PST
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.
Comment 4 WebKit Review Bot 2011-12-07 17:58:54 PST
Comment on attachment 118278 [details]
proposed patch

Attachment 118278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10806003
Comment 5 Alok Priyadarshi 2011-12-07 21:43:42 PST
Created attachment 118327 [details]
proposed patch

- Removed unrelated changes
- Fixed compile error
- Better ChangeLog
Comment 6 Alok Priyadarshi 2011-12-07 21:44:57 PST
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.
Comment 7 Alok Priyadarshi 2011-12-07 21:48:58 PST
(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.
Comment 8 Alok Priyadarshi 2011-12-09 14:34:49 PST
ping!
Comment 9 Stephen White 2011-12-13 12:13:33 PST
Comment on attachment 118327 [details]
proposed patch

Looks good.  r=me
Comment 10 WebKit Review Bot 2011-12-13 13:12:58 PST
Comment on attachment 118327 [details]
proposed patch

Clearing flags on attachment: 118327

Committed r102698: <http://trac.webkit.org/changeset/102698>
Comment 11 WebKit Review Bot 2011-12-13 13:13:12 PST
All reviewed patches have been landed.  Closing bug.