Bug 80471 - [chromium] wrong transform causing incorrect culling
Summary: [chromium] wrong transform causing incorrect culling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-06 18:48 PST by Shawn Singh
Modified: 2012-03-13 16:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.18 KB, patch)
2012-03-13 13:03 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (6.93 KB, patch)
2012-03-13 13:49 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-03-06 18:48:17 PST
To reproduce: 
  1. run chromium with --enable-partial-swap on osx   (should probably repro on cros and linux osmesa, too)
  2. visit www.apple.com/iphone

Things disappear awkwardly.   Its likely this is either a culling or damage tracking issue.

I also visualized paint and damage rects, and the rects go a little crazy.  Because of that, its reasonable to assume that its a damage tracking bug (or two).  If it becomes clear that its a culling issue I'll hand it off to Dana.
Comment 1 Shawn Singh 2012-03-13 12:54:28 PDT
Figured out the problem: in CCRenderPass, where the quad was being created for a RenderSurface, the quadTransforms was being initialized by the surface's drawTransform.   I believe this should actually be the originTransform instead, following the same pattern as other layers.

Without culling/damage tracking, it did not affect rendering because renderSurfaces draw themselves separately using the drawTransform, and they never used the quadTransform.

With both culling and damage tracking, however, things go wrong.  Culling (correctly) uses the quadTransform to determine the quad in target surface space; and, because the quadTransform was wrong, the quad becomes shifted incorrectly.  Therefore, when intersecting the damageRect, incorrect regions get culled.

Patch coming in just a moment.
Comment 2 Shawn Singh 2012-03-13 13:03:11 PDT
Created attachment 131699 [details]
Patch

This patch also shuffles code around so that the fix can be more easily tested
Comment 3 Dana Jansens 2012-03-13 13:11:07 PDT
Comment on attachment 131699 [details]
Patch

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

+1

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:41
>  #include "cc/CCLayerQuad.h"
> +#include "cc/CCRenderPass.h"

Can this file forward-declare class CCSharedQuadState, and the .cpp includes CCSharedQuadState.h?
Comment 4 James Robinson 2012-03-13 13:17:17 PDT
Comment on attachment 131699 [details]
Patch

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

R=me. I like the forward decl idea as well.

> Source/WebKit/chromium/tests/CCRenderSurfaceTest.cpp:105
> +    // for reference, remove before committing
> +    // return CCSharedQuadState::create(originTransform(), drawTransform(), contentRect(), clipRect(), drawOpacity(), isOpaque);

don't forget to do this
Comment 5 Shawn Singh 2012-03-13 13:49:00 PDT
Created attachment 131707 [details]
Patch for landing
Comment 6 Shawn Singh 2012-03-13 13:50:05 PDT
Cool - thanks to both of you for the quick review =)
Comment 7 WebKit Review Bot 2012-03-13 16:28:53 PDT
Comment on attachment 131707 [details]
Patch for landing

Clearing flags on attachment: 131707

Committed r110635: <http://trac.webkit.org/changeset/110635>
Comment 8 WebKit Review Bot 2012-03-13 16:28:58 PDT
All reviewed patches have been landed.  Closing bug.