Last Comment Bug 675837 - Add 3d transform support to BasicLayers
: Add 3d transform support to BasicLayers
Status: RESOLVED FIXED
[inbound][qa?]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla9
Assigned To: Nobody; OK to take it and work on it
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 505115
  Show dependency treegraph
 
Reported: 2011-08-01 18:56 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2011-12-09 00:58 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch (20.25 KB, patch)
2011-08-01 18:56 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Use pixman to transform content when accelerated layers aren't available (28.85 KB, patch)
2011-08-22 01:55 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Use pixman to transform content when accelerated layers aren't available v2 (18.58 KB, patch)
2011-08-22 23:09 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Use pixman to transform content when accelerated layers aren't available v3 (21.63 KB, patch)
2011-08-23 15:07 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Only flush quartz surfaces in the success paths during cairo_quartz_get_image (4.32 KB, patch)
2011-08-23 22:18 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Add gfxContext::ClipContainsRect function (2.03 KB, patch)
2011-08-23 23:27 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Use pixman to transform content when accelerated layers aren't available v4 (20.52 KB, patch)
2011-08-23 23:28 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Add gfxContext::ClipContainsRect function v2 (2.14 KB, patch)
2011-08-25 14:48 PDT, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2011-08-01 18:56:35 PDT
Created attachment 549994 [details] [diff] [review]
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) 2011-08-22 01:55:28 PDT
Created attachment 554805 [details] [diff] [review]
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) 2011-08-22 04:06:25 PDT
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) 2011-08-22 23:05:47 PDT
> 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) 2011-08-22 23:09:05 PDT
Created attachment 555038 [details] [diff] [review]
Use pixman to transform content when accelerated layers aren't available v2
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-23 05:15:54 PDT
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) 2011-08-23 15:07:17 PDT
Created attachment 555216 [details] [diff] [review]
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) 2011-08-23 17:34:14 PDT
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) 2011-08-23 18:46:47 PDT
(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) 2011-08-23 19:27:18 PDT
(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) 2011-08-23 22:05:52 PDT
> 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) 2011-08-23 22:18:21 PDT
Created attachment 555326 [details] [diff] [review]
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) 2011-08-23 22:23:16 PDT
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) 2011-08-23 22:26:58 PDT
(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) 2011-08-23 23:27:50 PDT
Created attachment 555331 [details] [diff] [review]
Add gfxContext::ClipContainsRect function
Comment 15 Matt Woodrow (:mattwoodrow) 2011-08-23 23:28:14 PDT
Created attachment 555332 [details] [diff] [review]
Use pixman to transform content when accelerated layers aren't available v4
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-24 17:21:24 PDT
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) 2011-08-25 14:47:54 PDT
> 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) 2011-08-25 14:48:27 PDT
Created attachment 555857 [details] [diff] [review]
Add gfxContext::ClipContainsRect function v2

Added correct documentation, carrying forward r=roc
Comment 21 Ioana (away) 2011-12-08 05:56:35 PST
Can anyone please help me with a test case and/or STR that I could verify this bug with?
Comment 22 Matt Woodrow (:mattwoodrow) 2011-12-08 12:18:14 PST
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) 2011-12-09 00:58:23 PST
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?

Note You need to log in before you can comment on or make changes to this bug.