Add 3d transform support to BasicLayers

RESOLVED FIXED in mozilla9

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mattwoodrow, Unassigned)

Tracking

unspecified
mozilla9
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound][qa?])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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.
Attachment #549994 - Attachment is obsolete: true
Attachment #554805 - Flags: feedback?(roc)
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?
(Reporter)

Comment 3

6 years ago
> 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.
(Reporter)

Comment 4

6 years ago
Created attachment 555038 [details] [diff] [review]
Use pixman to transform content when accelerated layers aren't available v2
Attachment #554805 - Attachment is obsolete: true
Attachment #554805 - Flags: feedback?(roc)
Attachment #555038 - Flags: review?(roc)
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?
(Reporter)

Comment 6

6 years ago
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.
Attachment #555038 - Attachment is obsolete: true
Attachment #555038 - Flags: review?(roc)
Attachment #555216 - Flags: review?(roc)
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.
(Reporter)

Comment 8

6 years ago
(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.
(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.
(Reporter)

Comment 10

6 years ago
> 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.
(Reporter)

Comment 11

6 years ago
Created attachment 555326 [details] [diff] [review]
Only flush quartz surfaces in the success paths during cairo_quartz_get_image
Attachment #555326 - Flags: review?(roc)
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.
Attachment #555326 - Flags: review?(roc) → review+
(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.
(Reporter)

Comment 14

6 years ago
Created attachment 555331 [details] [diff] [review]
Add gfxContext::ClipContainsRect function
Attachment #555331 - Flags: review?(roc)
(Reporter)

Comment 15

6 years ago
Created attachment 555332 [details] [diff] [review]
Use pixman to transform content when accelerated layers aren't available v4
Attachment #555216 - Attachment is obsolete: true
Attachment #555216 - Flags: review?(roc)
Attachment #555332 - Flags: review?(roc)
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."
Attachment #555331 - Flags: review?(roc) → review+
Attachment #555332 - Flags: review?(roc) → review+
(Reporter)

Comment 17

6 years ago
> 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.
(Reporter)

Comment 18

6 years ago
Created attachment 555857 [details] [diff] [review]
Add gfxContext::ClipContainsRect function v2

Added correct documentation, carrying forward r=roc
Attachment #555331 - Attachment is obsolete: true
Attachment #555857 - Flags: review+
(Reporter)

Comment 19

6 years ago
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
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/1de7e151b2ad
http://hg.mozilla.org/mozilla-central/rev/40e4402d4972
http://hg.mozilla.org/mozilla-central/rev/7ccff3faa2a3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Comment 21

6 years ago
Can anyone please help me with a test case and/or STR that I could verify this bug with?
Whiteboard: [inbound] → [inbound][qa?]
(Reporter)

Comment 22

6 years ago
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

6 years ago
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?
You need to log in before you can comment on or make changes to this bug.