675837 - Add 3d transform support to BasicLayers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP patch

We should be able to render layers directly into pixman and use it's 3x3 matrix to get perspective transforms without hardware acceleration.

WIP patch attached that was working for rendering untransformed content, but fails with transforms.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Use pixman to transform content when accelerated layers aren't available

Works for all tests that I've checked so far (Not many, but does the safari cylinder perfectly!)

Looking for feedback on approach, and the TODO's mentioned. Well aware of the cleanup required before an actual review :)

The cairo/thebes changes adding GetSize() implementations are only there because we need to know the size of the destination surface. If we can get this another way, I'll remove these parts. Probably useful as a separate patch though, as gfxASurface::DumpAsDataURL() also needs to know a size.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 554805 [details] [diff] [review]
Use pixman to transform content when accelerated layers aren't available

Review of attachment 554805 [details] [diff] [review]:
-----------------------------------------------------------------

Basically looks good although I didn't check the details yet...

::: gfx/layers/basic/BasicLayers.cpp
@@ +1371,5 @@
> +  if (!aTarget)
> +    return nsnull;
> +  nsRefPtr<gfxASurface> current = aPushed->CurrentSurface();
> +  if (mCachedSurface.IsSurface(current)) {
> +      mCachedSurfaceInUse = PR_FALSE;

This is confusing. We never actually hit this case, right? We should just ensure that 3D transforms never use the cached surface and error out here.

@@ +1861,5 @@
> +
> +        sourceSurface->SetDeviceOffset(gfxPoint(0, 0));
> +        ctx->SetSource(sourceSurface);
> +        ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
> +        ctx->Paint();

Create a helper function somewhere that copies to a new image surface if GetAsImageSurface fails (if we don't have that somewhere already!)

@@ +1873,5 @@
> +       * TODO: Can we get the destination rectangle an easier way?
> +       */
> +      nsRefPtr<gfxASurface> targetSurf = aTarget->CurrentSurface();
> +      gfxRect destSurface(-targetSurf->GetDeviceOffset(),
> +                          targetSurf->GetSize());

Why don't you just use the device-space clip extents of aTarget instead of trying to get the surface size here?

Comment 3

[Matt Woodrow (:mattwoodrow)]

> Create a helper function somewhere that copies to a new image surface if
> GetAsImageSurface fails (if we don't have that somewhere already!)

We don't have one of these because not all surface types can accurately report their size. My patch started to implement these as I needed them, but I don't think we should block this patch on having them all implemented.

We could always pass the size to use if GetAsImageSurface() fails, but that feels a little weird, since you'd only necessarily get a surface that size on the fallback path.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Use pixman to transform content when accelerated layers aren't available v2

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 555038 [details] [diff] [review]
Use pixman to transform content when accelerated layers aren't available v2

Review of attachment 555038 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/basic/BasicLayers.cpp
@@ +1365,5 @@
>    }
>  }
>  
> +already_AddRefed<gfxASurface>
> +BasicLayerManager::PopGroupWithCachedSurface(gfxContext *aTarget, gfxContext *aPushed)

We should change the name since this is never the cached surface...

PopGroupToSurface?

@@ +1689,5 @@
> +                const gfx3DMatrix& aTransform, 
> +                gfxPoint aDestOffset)
> +{
> +  gfxIntSize destSize = aDest->GetSize();
> +  pixman_image_t* dest = pixman_image_create_bits(aDest->Format() == gfxASurface::ImageFormatARGB32 ? PIXMAN_a8r8g8b8 : PIXMAN_x8r8g8b8,

Perhaps we should just assert that aDest->Format() is ARGB? We can't realistically support anything else here since an arbitrary 3D transform is going to leave some part of the destination surface uncovered.

@@ +1769,5 @@
>    gfxMatrix transform;
> +  // Will return an identity matrix for 3d transforms, and is handled separately below.
> +  PRBool is2D = effectiveTransform.CanDraw2D(&transform);
> +  NS_ABORT_IF_FALSE(is2D || needsGroup || !aLayer->GetFirstChild(), "Must PushGroup for 3d transforms!");
> +  if (!is2D) {

Flip branches to avoid ! usage

@@ +1838,5 @@
>    if (needsGroup) {
> +    if (is2D) {
> +      PopGroupToSourceWithCachedSurface(aTarget, groupTarget);
> +    } else {
> +      nsRefPtr<gfxASurface> sourceSurface = PopGroupWithCachedSurface(aTarget, groupTarget);

This else block should mostly go into a helper function. Something that takes the sourceSurface and a transform as input and returns the transformed surface.

@@ +1875,5 @@
> +
> +      /* Transform the content and offset it such that the content begins at the origin. */
> +      PixmanTransform(dest, sourceImage, translation * effectiveTransform, destRect.TopLeft());
> +
> +      aTarget->SetSource(dest, destRect.TopLeft());

Seems to me that in some cases we could get direct access to an image surface for the destination surface and have pixman draw directly into it instead of copying through the destination surface. Is it worth trying to do that?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Use pixman to transform content when accelerated layers aren't available v3

Fixed suggestions.

The cairo fix is because GetAsImageSurface() was flushing the quartz surface, which is usually the window. This was breaking our double buffering, and a massive perf regression.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 555216 [details] [diff] [review]
Use pixman to transform content when accelerated layers aren't available v3

Review of attachment 555216 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/cairo/cairo/src/cairo-quartz-surface.c
@@ +1917,2 @@
>      if (surface->imageSurfaceEquiv) {
> +	CGContextFlush(surface->cgContext);

Put the cairo fix in its own patch, I think. And make sure you do the cairo patchery dance.

::: gfx/layers/basic/BasicLayers.cpp
@@ +1772,5 @@
> +  nsRefPtr<gfxImageSurface> destImage = dest->GetAsImageSurface();
> +  destImage = nsnull;
> +  gfxPoint offset;
> +  PRBool blitComplete;
> +  if (!destImage || aLayer->GetEffectiveOpacity() != 1.0f) {

Don't we also need to check that the current clip on aDest doesn't need to clip our drawing here?

@@ +1931,5 @@
>      // ApplyDoubleBuffering guarantees that this container layer can't
>      // intersect any other leaf layers, so if the transaction is not yet marked
>      // incomplete, the contents of this container layer are the final contents
>      // for the window.
> +    if (!mTransactionIncomplete && !blitComplete) {

Why do you need to test this here.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 555216 [details] [diff] [review]
> Use pixman to transform content when accelerated layers aren't available v3
> 
> Review of attachment 555216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/cairo/cairo/src/cairo-quartz-surface.c
> @@ +1917,2 @@
> >      if (surface->imageSurfaceEquiv) {
> > +	CGContextFlush(surface->cgContext);
> 
> Put the cairo fix in its own patch, I think. And make sure you do the cairo
> patchery dance.
> 
> ::: gfx/layers/basic/BasicLayers.cpp
> @@ +1772,5 @@
> > +  nsRefPtr<gfxImageSurface> destImage = dest->GetAsImageSurface();
> > +  destImage = nsnull;
> > +  gfxPoint offset;
> > +  PRBool blitComplete;
> > +  if (!destImage || aLayer->GetEffectiveOpacity() != 1.0f) {
> 
> Don't we also need to check that the current clip on aDest doesn't need to
> clip our drawing here?

drawRect is already intersected with aDest->GetClipExtents(), what else do I need to check?

> 
> @@ +1931,5 @@
> >      // ApplyDoubleBuffering guarantees that this container layer can't
> >      // intersect any other leaf layers, so if the transaction is not yet marked
> >      // incomplete, the contents of this container layer are the final contents
> >      // for the window.
> > +    if (!mTransactionIncomplete && !blitComplete) {
> 
> Why do you need to test this here.

If blitComplete is true, then Transform3D has already drawn to aTarget, otherwise we've just set a source and we need to call Paint() here.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> drawRect is already intersected with aDest->GetClipExtents(), what else do I
> need to check?

The problem is if drawRect isn't entirely inside the clip region (not its extents!), then drawing directly to the surface won't be properly clipped.

> > @@ +1931,5 @@
> > >      // ApplyDoubleBuffering guarantees that this container layer can't
> > >      // intersect any other leaf layers, so if the transaction is not yet marked
> > >      // incomplete, the contents of this container layer are the final contents
> > >      // for the window.
> > > +    if (!mTransactionIncomplete && !blitComplete) {
> > 
> > Why do you need to test this here.
> 
> If blitComplete is true, then Transform3D has already drawn to aTarget,
> otherwise we've just set a source and we need to call Paint() here.

I see.

Comment 10

[Matt Woodrow (:mattwoodrow)]

> The problem is if drawRect isn't entirely inside the clip region (not its
> extents!), then drawing directly to the surface won't be properly clipped.
> 

Right. How do I check the clip region? gfxAContext only has GetClipExtents(). I don't think implementing proper clipping is feasible, so we just need to know if the clip region == clip extents. If not, fallback to using cairo for the final blit.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Only flush quartz surfaces in the success paths during cairo_quartz_get_image

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 555326 [details] [diff] [review]
Only flush quartz surfaces in the success paths during cairo_quartz_get_image

Review of attachment 555326 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to add to the cairo patch README file.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> > The problem is if drawRect isn't entirely inside the clip region (not its
> > extents!), then drawing directly to the surface won't be properly clipped.
> 
> Right. How do I check the clip region? gfxAContext only has
> GetClipExtents(). I don't think implementing proper clipping is feasible, so
> we just need to know if the clip region == clip extents. If not, fallback to
> using cairo for the final blit.

cairo_copy_clip_rectangle_list is the only way I know of to get that information. You probably should create a gfxContext::GetClipRegion wrapper around that. Note however that cairo_copy_clip_rectangle_list can fail with AIRO_STATUS_CLIP_NOT_REPRESENTABLE (e.g. because the clip contains a path). So GetClipRegion will have to be able to fail too. Of course, when it fails you just use your fallback path.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Add gfxContext::ClipContainsRect function

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Use pixman to transform content when accelerated layers aren't available v4

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 555331 [details] [diff] [review]
Add gfxContext::ClipContainsRect function

Review of attachment 555331 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxContext.h
@@ +606,5 @@
>      gfxRect GetClipExtents();
>  
>      /**
> +     * Returns true if the clip is a single rectangle that is
> +     * identical to the clip extents.

It can also return true when the clip has multiple rectangles, one of which contains the given rectangle.

I would just say
"Returns true if the given rectangle is fully contained in the current clip. This is conservative; it may return false even when the given rectangle is fully contained by the current clip."

Comment 17

[Matt Woodrow (:mattwoodrow)]

> I would just say
> "Returns true if the given rectangle is fully contained in the current clip.
> This is conservative; it may return false even when the given rectangle is
> fully contained by the current clip."

Sorry, this was the comment for my previous function, forgot to update it.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Add gfxContext::ClipContainsRect function v2

Added correct documentation, carrying forward r=roc

Comment 19

[Matt Woodrow (:mattwoodrow)]

Landed on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/1de7e151b2ad
http://hg.mozilla.org/integration/mozilla-inbound/rev/40e4402d4972
http://hg.mozilla.org/integration/mozilla-inbound/rev/7ccff3faa2a3

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/1de7e151b2ad
http://hg.mozilla.org/mozilla-central/rev/40e4402d4972
http://hg.mozilla.org/mozilla-central/rev/7ccff3faa2a3

Comment 21

[Ioana (away)]

Can anyone please help me with a test case and/or STR that I could verify this bug with?

Comment 22

[Matt Woodrow (:mattwoodrow)]

The existing 3d transforms reftest (layout/reftest/transform-3d) should cover this, on platforms where we don't have accelerated layers. Look for 'layersGPUaccelerated' in the reftest log.

Comment 23

[Ioana (away)]

Matt,
According to what can be seen here https://tbpl.mozilla.org/, the reftests are not run on Windows 7 x64 (the OS this bug has been logged on). Can I find reftests logs anywhere else? Is there any other way I could verify this fix?