Closed Bug 1252324 Opened 8 years ago Closed 8 years ago

3D transform code in basic layers needs optimizations and cleanups

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 3 obsolete files)

The 3D transform code is literally duplicated in two places in basic layers: BasicCompositor and BasicLayerManager. It exposes dependencies inside the layers code on both Skia and Pixman internals, and also can cause unnecessary readbacks in the Xrender case.
This almost entirely removes all the 3D transform backend code out of BasicCompositor and BasicLayerManager.

It adds an API to DrawTarget that works as follows:

Ideally you should call DrawTransform with a temporary source DT you have buffered you BGRA input content into as a source, giving a 3D transform and the transformed bounds you want it to appear in on the destination DT.

This will internally call TransformRect with the transform and bounds on the source DT to get a transformed subrect of the snapshot, that can finally be composited to the destination with DrawSurface.

There are some hooks DTs can use to optimize the process: CreateTransformDrawTarget (intended to be used like CreateShadowDrawTarget and similar), to create DTs of optimal format for the TransformRect step, and TransformSurface which is used to take the created surface and fill the DT with it, using the transform.

A DT can also override DrawTransform directly like in the Skia case if it knows it can directly handle drawing a source clipped to the destination with the transform with no intermediate buffers in the way.

CreateTransformDrawTarget right now is mainly used to select a Cairo or Skia image DT depending on which is available at compile time, with Skia being preferred.

Cairo also implements a direct XRender path which can skip any readback from the X server, using the X render transform API where available.

So, basically, the Skia-all-the-way-through case is now faster, since it skips a copy, and the all XRender case is now faster, since it skips a readback, and we have half as much 3D transform code to worry about. Win, win, win.
Attachment #8725043 - Flags: review?(jmuizelaar)
Status: NEW → ASSIGNED
Comment on attachment 8725043 [details] [diff] [review]
add DrawTarget API for 3D transforms for use in layers

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

::: gfx/2d/2D.h
@@ +1063,5 @@
> +  /**
> +   * Create a draw target optimized for drawing a 3D transform.
> +   */
> +  virtual already_AddRefed<DrawTarget>
> +    CreateTransformDrawTarget(const IntSize &aSize) const;

In my opinion there should be a way of hiding this inside the backends which can't easily provide this. I don't really like this.

@@ +1086,5 @@
> +  /**
> +   * Transforms the snapshot of this draw target by the 3D transform aMatrix and returns a
> +   * aSourceSurface clipped to aBounds. If the transform, nullptr is returned.
> +   */
> +  already_AddRefed<SourceSurface> TransformRect(const Matrix4x4& aMatrix, const Rect& aBounds);

I'm not really sure I'd want this API exposed to the outside. Let's talk about this tomorrow.
Comment on attachment 8725043 [details] [diff] [review]
add DrawTarget API for 3D transforms for use in layers

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

::: gfx/2d/DrawTarget.cpp
@@ +38,5 @@
> +already_AddRefed<DrawTarget>
> +DrawTarget::CreateTransformDrawTarget(const IntSize &aSize) const
> +{
> +  const SurfaceFormat format = SurfaceFormat::B8G8R8A8;
> +#ifdef UUSE_SKIA

UUSE_SKIA is probably wrong
Did as requested by Bas and implemented Draw3DTransformedSurface as the only public API exposed for DrawTargets. You pass in a SourceSurface and a Matrix4x4, and it does the rest. It respects both the existing transform and clip on the DrawTarget.

The Skia version is sweet. The pixman version is a bit sad. The xrender version is a bit sad as well, but definitely helpful in getting rid of the 3D transform performance cliff we had with X server readbacks if someone is still insisting on wanting to run in GTK2/xrender land.
Attachment #8725043 - Attachment is obsolete: true
Attachment #8725043 - Flags: review?(jmuizelaar)
Attachment #8726097 - Flags: review?(jmuizelaar)
Attachment #8726097 - Flags: feedback?(bas)
This version adds support for preferentially using Skia as the fallback 3D transform, and only punts to Cairo if Skia is not built in.

It also implements the pass-through hack (that we can tell Skia or Cairo the format is just A8R8G8B8_UINT32 so long as the source and destination are the same) that allows our code to despite the fact that we inconsistently use B8G8R8A8 and A8R8G8B8_UINT32 throughout the codebase as if they are the same format, even though they are technically incompatible formats.
Attachment #8726097 - Attachment is obsolete: true
Attachment #8726097 - Flags: review?(jmuizelaar)
Attachment #8726097 - Flags: feedback?(bas)
Attachment #8726479 - Flags: review?(jmuizelaar)
Attachment #8726479 - Flags: feedback?(bas)
Oops, accidentally exported wrong patch. It was a long day...
Attachment #8726479 - Attachment is obsolete: true
Attachment #8726479 - Flags: review?(jmuizelaar)
Attachment #8726479 - Flags: feedback?(bas)
Attachment #8726480 - Flags: review?(jmuizelaar)
Attachment #8726480 - Flags: feedback?(bas)
Comment on attachment 8726480 [details] [diff] [review]
add DrawTarget API for 3D transforms for use in layers

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

You have made me a happy man.

::: gfx/2d/DrawTargetSkia.cpp
@@ +675,5 @@
>    mCanvas->drawBitmap(bitmap, aOffset.x, aOffset.y, &paint.mPaint);
>  }
>  
> +bool
> +DrawTarget::Draw3DTransformedSurface(SourceSurface* aSurface, const Matrix4x4& aMatrix)

Hrm, implementing this in DrawTargetSkia.cpp is slightly dodgy, but I suppose it will have to do for now.
Attachment #8726480 - Flags: feedback?(bas) → feedback+
Comment on attachment 8726480 [details] [diff] [review]
add DrawTarget API for 3D transforms for use in layers

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +930,3 @@
>  
> +    RefPtr<SourceSurface> untransformedSurf = untransformedDT->Snapshot();
> +    RefPtr<DrawTarget> xformDT =

Why do we need this temporary?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 8726480 [details] [diff] [review]
> add DrawTarget API for 3D transforms for use in layers
> 
> Review of attachment 8726480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicLayerManager.cpp
> @@ +930,3 @@
> >  
> > +    RefPtr<SourceSurface> untransformedSurf = untransformedDT->Snapshot();
> > +    RefPtr<DrawTarget> xformDT =
> 
> Why do we need this temporary?

The existing code can possibly also do masking and blending/opacity, so it build a pattern to represent the transformed surface. This just preserves that behavior. It is easier to see there now, but it was going on before in Transform3D to compensate for that just the same.
Comment on attachment 8726480 [details] [diff] [review]
add DrawTarget API for 3D transforms for use in layers

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +699,5 @@
> +  RefPtr<DataSourceSurface> dstSurf =
> +    Factory::CreateDataSourceSurface(xformBounds.Size(),
> +                                     srcBitmap.alphaType() == kPremul_SkAlphaType ?
> +                                       aSurface->GetFormat() : SurfaceFormat::A8R8G8B8_UINT32,
> +                                     true);

We can add LockBits support for the destination in the future.
Attachment #8726480 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/51102a2a44b5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1266380
Depends on: 1275504
No longer depends on: 1275504
You need to log in before you can comment on or make changes to this bug.