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)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: lsalzman, Assigned: lsalzman)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 3 obsolete files)
36.56 KB,
patch
|
jrmuizel
:
review+
bas.schouten
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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?
Assignee | ||
Comment 9•8 years ago
|
||
(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 10•8 years ago
|
||
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+
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/51102a2a44b5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•