Closed Bug 572680 Opened 14 years ago Closed 14 years ago

Refactor image drawing

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b4

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(7 files, 10 obsolete files)

(deleted), patch
joe
: review+
Details | Diff | Splinter Review
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
I need this for -moz-element, bug 506826. Currently our image drawing works like this: 1. DrawImageInternal in nsLayoutUtils.cpp is called with an imgIContainer. 2. It calculates transforms and fill rects using the image snapping algorithm. 3. The results of this algorithm are passed to imgContainer::Draw, which passes them on to imgFrame::Draw. 4. imgFrame::Draw does this: 4a. Single color? Fill and return. 4b. For partial images, construct a new gfxASurface with the partial image and transparent margin, or, in the non-repeating case, use the existing surface and tweak the transforms and rects. 4c. Work around all kinds of bugs. 4d. Draw. So this all only works with imgIContainers, and inside imgFrame::Draw only with gfxASurfaces. However, in bug 506826 I need these things for all kinds of different drawable things: - gfxASurfaces (e.g. for out-of-document videos) - gfxPatterns (for SVG gradients and patterns) - drawing callbacks (for rendering a different element directly without temporary surface)
Attachment #451934 - Flags: review?(roc)
The next part will add a method to gfxUtils that takes a gfxPattern::GraphicsFilter parameter. So I need to include gfxPattern.h in gfxUtils.h. However, since gfxMatrix.h includes gfxUtils.h and gfxPattern.h includes gfxMatrix.h, this doesn't work. So I need to make gfxMatrix.h stop including gfxUtils.h. gfxMatrix.h only needed gfxUtils.h for its "FuzzyEqual" method, and that method isn't used anywhere else, so I'm making it a private method of gfxMatrix.
Attachment #451937 - Flags: review?
Attachment #451937 - Flags: review? → review?(joe)
This moves 4a in its own method in imgFrame.cpp ("DoSingleColorFastPath"), 4b into another method in imgFrame.cpp ("SurfaceForDrawing"), and 4c and 4d into gfxUtils::DrawPixelSnapped. It's a big patch, but it's only moving code and hopefully doesn't cause any functionality differences. It passes tests. I put the cairo / pixman bug workaround from bug 364968 in the constructor and destructor of a class because I think that makes it clearer: If the workaround pushed a group, it needs to pop it afterwards.
Attachment #451940 - Flags: review?(joe)
Attached patch part 4: create gfxDrawable (obsolete) (deleted) — Splinter Review
gfxUtils::DrawPixelSnapped from part 3 can only draw gfxASurfaces. However, as I said in comment 0, I don't always want to create a temporary surface when we don't really need one. For example an SVG gradient that fills the whole box should just be able to paint the gradient gfxPattern directly, and still use the same code path as all other -moz-element drawing. So I introduce a new interface: gfxDrawable. It represents something drawable that has an intrinsic integer size and can produce a gfxPattern. I'm also adding three convenience implementations for gfxDrawable: gfxSurfaceDrawable, gfxPatternDrawable and gfxCallbackDrawable. The surface flavour will be used by imgFrame::Draw and out-of-document images and videos in -moz-element; the pattern flavour will be used by SVG paint servers in -moz-element; and the callback one by normal HTML element paint servers. PreparePatternForUntiledDrawing does what SetExtendAndFilterOnPattern did in part 3; in part 5 I'll remove the latter. I'm using 4-space-indent because that's what the rest of gfx seems to be doing.
Attachment #451943 - Flags: review?(joe)
Attached patch part 5: use gfxDrawables for image drawing (obsolete) (deleted) — Splinter Review
Straightforward.
Attachment #451944 - Flags: review?(joe)
This will be used for all -moz-element drawing. It just runs the image snapping algorithm and pushes the result through gfxUtils::DrawPixelSnapped. gfxUtils.h has a comment why we need both and what the difference is.
Attachment #451945 - Flags: review?(roc)
Attachment #451937 - Flags: review?(joe) → review+
Comment on attachment 451940 [details] [diff] [review] part 3: create gfxUtils::DrawPixelSnapped and make imgFrame use it In general, I really like the way this patch is going, but it has a couple of implementation issues. >diff --git a/gfx/thebes/public/gfxUtils.h b/gfx/thebes/public/gfxUtils.h >+ * Draw a surface while working around limitations like bad support >+ * for EXTEND_PAD, lack of source-clipping, or cairo / pixman bugs with >+ * extreme user-space-to-image-space transforms. >+ * >+ * The input paramaters here usually come from the output of our image ^parameters >diff --git a/gfx/thebes/src/gfxUtils.cpp b/gfx/thebes/src/gfxUtils.cpp >+// EXTEND_PAD won't help us here; we have to create a temporary surface to hold >+// the subimage of pixels we're allowed to sample. >+static gfxPattern* >+CreateSamplingRestrictedPattern(gfxASurface* aSurface, >+ gfxContext* aContext, >+ const gfxMatrix& aUserSpaceToImageSpace, >+ const gfxRect& aSourceRect, >+ const gfxRect& aSubimage, >+ const gfxImageSurface::gfxImageFormat aFormat) This has got leaks written all over it. Better return an already_AddRefed<gfxPattern>, and... >+ gfxPattern* resultPattern = new gfxPattern(temp); >+ if (!resultPattern) >+ return nsnull; ...here, use a nsRefPtr<gfxPattern>, and... >+ >+ resultPattern->SetMatrix( >+ gfxMatrix(aUserSpaceToImageSpace).Multiply(gfxMatrix().Translate(-needed.pos))); >+ return resultPattern; ... here, return resultPattern.forget(). >+// working around cairo/pixman bug (bug 364968) >+// Our device-space-to-image-space transform may not be acceptable to pixman. >+struct AutoCairoPixmanBugWorkaround { This should be an NS_STACK_CLASS. >diff --git a/modules/libpr0n/src/imgFrame.cpp b/modules/libpr0n/src/imgFrame.cpp >--- a/modules/libpr0n/src/imgFrame.cpp >+++ b/modules/libpr0n/src/imgFrame.cpp >@@ -39,16 +39,17 @@ > #include "imgFrame.h" > > #include <limits.h> > > #include "prmem.h" > #include "prenv.h" > > #include "gfxPlatform.h" >+#include "gfxUtils.h" > > static PRBool gDisableOptimize = PR_FALSE; > > /*XXX get CAIRO_HAS_DDRAW_SURFACE */ > #include "cairo.h" > > #ifdef CAIRO_HAS_DDRAW_SURFACE > #include "gfxDDrawSurface.h" >@@ -370,288 +371,133 @@ nsresult imgFrame::Optimize() > #ifdef XP_MACOSX > mQuartzSurface = nsnull; > #endif > } > > return NS_OK; > } > >-static PRBool >-IsSafeImageTransformComponent(gfxFloat aValue) >+static void >+DoSingleColorFastPath(gfxContext* aContext, >+ const gfxRGBA& aSinglePixelColor, >+ const gfxRect& aFill) > { >- return aValue >= -32768 && aValue <= 32767; >+ // if a == 0, it's a noop >+ if (aSinglePixelColor.a == 0.0) >+ return; >+ >+ gfxContext::GraphicsOperator op = aContext->CurrentOperator(); >+ if (op == gfxContext::OPERATOR_OVER && aSinglePixelColor.a == 1.0) { >+ aContext->SetOperator(gfxContext::OPERATOR_SOURCE); >+ } >+ >+ aContext->SetDeviceColor(aSinglePixelColor); >+ aContext->NewPath(); >+ aContext->Rectangle(aFill); >+ aContext->Fill(); >+ aContext->SetOperator(op); >+ aContext->SetDeviceColor(gfxRGBA(0,0,0,0)); >+} >+ >+imgFrame::SurfaceWithFormat >+imgFrame::SurfaceForDrawing(PRBool aDoPadding, >+ gfxASurface* surface = >+ gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format).get(); This should be an nsRefPtr<gfxASurface>. >+ if (surfaceResult.mContinue) { Change this as mentioned below. >+ gfxUtils::DrawPixelSnapped(aContext, surfaceResult.mSurface, >+ userSpaceToImageSpace, >+ subimage, sourceRect, imageRect, fill, >+ surfaceResult.mFormat, aFilter); > } >diff --git a/modules/libpr0n/src/imgFrame.h b/modules/libpr0n/src/imgFrame.h >+ struct SurfaceWithFormat { >+ PRBool mContinue; >+ gfxASurface* mSurface; This should be an nsRefPtr<gfxASurface>. And mContinue should be removed; instead, add a method here called IsValid(), which just checks to see if mSurface is non-null.
Attachment #451940 - Flags: review?(joe) → review-
Comment on attachment 451943 [details] [diff] [review] part 4: create gfxDrawable >diff --git a/gfx/thebes/public/gfxDrawable.h b/gfx/thebes/public/gfxDrawable.h >+/** >+ * gfxDrawingCallback >+ * An Interface representing something that can draw itself. >+ */ >+class THEBES_API gfxDrawingCallback { >+ NS_INLINE_DECL_REFCOUNTING(gfxDrawingCallback) >+public: >+ virtual ~gfxDrawingCallback() {} >+ >+ /** >+ * Draw into aContext filling aFillRect, possibly repeating, using aFilter. >+ * aTransform is a userspace to "image"space matrix. For example, if Draw >+ * draws using a gfxPattern, this is the matrix that should be set on the >+ * pattern prior to rendering it. >+ * @return whether drawing was successful >+ */ >+ virtual PRBool Draw(gfxContext* aContext, >+ const gfxRect aFillRect, >+ const PRBool aRepeat, >+ const gfxPattern::GraphicsFilter& aFilter, >+ const gfxMatrix aTransform = gfxMatrix())=0; >+ >+}; >+ >+/** >+ * gfxDrawable >+ * An Interface representing something that has an intrinsic size, can draw >+ * itself and can generate a repeatable pattern. >+ */ >+class THEBES_API gfxDrawable : public gfxDrawingCallback { It's very odd to me that Drawables both _are_ callbacks, and also (in the case of gfxCallbackDrawable) can _have_ callbacks. I think what you should do here is combine gfxDrawable and gfxDrawingCallback into one class, and then make gfxCallbackDrawable take a separate functor (or, failing that, function pointer) as an argument.
Attachment #451943 - Flags: review?(joe) → review-
Comment on attachment 451944 [details] [diff] [review] part 5: use gfxDrawables for image drawing This'll need changes that result from the earlier patches, but at first glance looks OK. I'm only minusing this because I want to look at it again once the other changes are made.
Attachment #451944 - Flags: review?(joe) → review-
Comment on attachment 451934 [details] [diff] [review] part 1: pull image snapping algorithm out of DrawImageInternal +// helper function to convert a nsRect to a gfxRect +// borrowed from nsCSSRendering.cpp move to nsLayoutUtils? + // Whether there's anything to draw at all + PRBool mShouldDraw; PRPackedBool and move it down to mResetCTM (which should also be PRPackedBool) + // An image-space to device-space transform (translation and scale) + gfxMatrix mUserSpaceToImageSpace; The comment doesn't match the name. I believe this transforms from either device space or user space (depending on mResetCTM) to image space? Otherwise looks good.
Why does gfxUtils::DrawPixelSnapped need to take an aFormat parameter? Can't you get the format from the surface? By the way, I think the work you've done here is *fantastic*.
Comment on attachment 451934 [details] [diff] [review] part 1: pull image snapping algorithm out of DrawImageInternal From part 1: >diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp >+ SnappedImageDrawingParameters(gfxMatrix aUserSpaceToImageSpace, >+ gfxRect aFillRect, >+ nsIntRect aSubimage, >+ PRBool aResetCTM) >+ : mShouldDraw(PR_TRUE) > [etc] Those first three args should probably be const references, right? (to reduce unnecessary copying of large-ish data structures)
Attached patch part 1, v2 [landed] (deleted) — Splinter Review
(In reply to comment #10) > (From update of attachment 451934 [details] [diff] [review]) > +// helper function to convert a nsRect to a gfxRect > +// borrowed from nsCSSRendering.cpp > > move to nsLayoutUtils? That would mean that I have to prefix all invocations with nsLayouUtils:: and in many cases this means I have to wrap the line because of the 80-column restriction, and then I've lost the main advantage of having a helper in the first place. Should I do it anyway? I haven't done it in this patch. > + // Whether there's anything to draw at all > + PRBool mShouldDraw; > > PRPackedBool and move it down to mResetCTM (which should also be PRPackedBool) Done. > + // An image-space to device-space transform (translation and scale) > + gfxMatrix mUserSpaceToImageSpace; > > The comment doesn't match the name. I believe this transforms from either > device space or user space (depending on mResetCTM) to image space? Yes I think that's correct. Fixed. (In reply to comment #12) > (From update of attachment 451934 [details] [diff] [review]) > From part 1: > >diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp > >+ SnappedImageDrawingParameters(gfxMatrix aUserSpaceToImageSpace, > >+ gfxRect aFillRect, > >+ nsIntRect aSubimage, > >+ PRBool aResetCTM) > >+ : mShouldDraw(PR_TRUE) > > [etc] > > Those first three args should probably be const references, right? Done.
Attachment #451934 - Attachment is obsolete: true
Attachment #453023 - Flags: review?(roc)
Attachment #451934 - Flags: review?(roc)
Attached patch part 3, v2 (obsolete) (deleted) — Splinter Review
Attachment #451940 - Attachment is obsolete: true
Attachment #453029 - Flags: review?(joe)
(In reply to comment #8) > (From update of attachment 451943 [details] [diff] [review]) > It's very odd to me that Drawables both _are_ callbacks, and also (in the case > of gfxCallbackDrawable) can _have_ callbacks. That allows gfxPatternDrawables to do this: mCallbackDrawable = new gfxCallbackDrawable(this, mSize); They can just pass themselves as the callback to a gfxCallbackDrawable and then delegate pattern creation and tiled drawing to it. > I think what you should do here > is combine gfxDrawable and gfxDrawingCallback into one class, and then make > gfxCallbackDrawable take a separate functor So I'd need a new functor base class. What should I call that class? gfxDrawingCallback? ;) And I'd need an implementation of that functor that wraps a gfxDrawable so that I can do the magic for patterns that I described above. All in all, this doesn't seem any simpler to me, but maybe I'm missing something?
Attached patch part 4, gfxDrawable (obsolete) (deleted) — Splinter Review
This update doesn't make the changes you requested, see previous comment. But there was a different issue that caused tests to fail on Linux: PreparePatternForUntiledDrawing was never called on the pattern created in CreateSamplingRestrictedPattern because it's only called for gfxSurfaceDrawables and CreateSamplingRestrictedPattern used a gfxPatternDrawable. In order to make it possible for it to use a gfxSurfaceDrawable, I had to add an additional mTransform field to gfxSurfaceDrawable that gets applied to the gfxPattern that is created by when drawing. That's what this patch does.
Attachment #451943 - Attachment is obsolete: true
Attachment #453031 - Flags: review?(joe)
Attached patch part 5, v2 (obsolete) (deleted) — Splinter Review
Updated for changes in part 3 and to use a gfxSurfaceDrawable in CreateSamplingRestrictedPattern.
Attachment #451944 - Attachment is obsolete: true
(In reply to comment #11) > Why does gfxUtils::DrawPixelSnapped need to take an aFormat parameter? Can't > you get the format from the surface? I don't think I can. If I could then imgFrame wouldn't need its mFormat field, for example. We could add a GetFormat() method to gfxASurface that returns ImageFormatUnknown by default and make those surface implementations that know their format override it to return the correct format. But I'd rather like to do that in a different bug after this.
+ const gfxRect aFillRect, + const PRBool aRepeat, + const gfxPattern::GraphicsFilter& aFilter, + const gfxMatrix aTransform = gfxMatrix()); aFillRect should be a const gfxRect&. aTransform should be const gfxMatrix&. PRBool aRepeat need not be const.
Comment on attachment 453031 [details] [diff] [review] part 4, gfxDrawable I've changed my mind and agree that splitting gfxDrawingCallback from gfxDrawable is a good idea. That way we can say gfxDrawingCallback only needs to be able to draw itself *once*, but gfxDrawable must also handle drawing *repeatedly*.
Attachment #453031 - Flags: review?(joe)
Attachment #451937 - Attachment description: part 2: shuffle some gfx dependencies → part 2: shuffle some gfx dependencies [landed]
Attachment #453023 - Attachment description: part 1, v2 → part 1, v2 [landed]
Attached patch part 4, gfxDrawable (obsolete) (deleted) — Splinter Review
I've called the functor base class gfxDrawingCallback. In gfxDrawable.cpp there's a class called DrawingCallbackFromDrawable that works as an adapter from gfxDrawable to gfxDrawingCallback and just passes false to aRepeat.
Attachment #453031 - Attachment is obsolete: true
Attachment #455483 - Flags: review?(joe)
Attached patch part 5, updated to trunk (obsolete) (deleted) — Splinter Review
Attachment #453032 - Attachment is obsolete: true
Attachment #455486 - Flags: review?(joe)
Comment on attachment 453029 [details] [diff] [review] part 3, v2 >diff --git a/gfx/thebes/src/gfxUtils.cpp b/gfx/thebes/src/gfxUtils.cpp >+// working around cairo/pixman bug (bug 364968) >+// Our device-space-to-image-space transform may not be acceptable to pixman. >+struct NS_STACK_CLASS AutoCairoPixmanBugWorkaround { Put the { on its own line, please. >diff --git a/modules/libpr0n/src/imgFrame.cpp b/modules/libpr0n/src/imgFrame.cpp >+imgFrame::SurfaceForDrawing(PRBool aDoPadding, >+ nsRefPtr<gfxASurface> surface = >+ gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format).get(); You don't need this .get() here. Otherwise awesome.
Attachment #453029 - Flags: review?(joe) → review+
Comment on attachment 455483 [details] [diff] [review] part 4, gfxDrawable General comment: please put braces for new classes on their own line: class Foo { // some stuff }; >diff --git a/gfx/thebes/gfxDrawable.cpp b/gfx/thebes/gfxDrawable.cpp >+gfxCallbackDrawable::Draw(gfxContext* aContext, >+ const gfxRect& aFillRect, >+ PRBool aRepeat, >+ const gfxPattern::GraphicsFilter& aFilter, >+ const gfxMatrix& aTransform) >+{ >+ if (aRepeat) { >+ MakeSurfaceDrawable(aFilter); Here, we should say mSurfaceDrawable = MakeSurfaceDrawable(...), and have MakeSurfaceDrawable just return an already_AddRefed<gfxSurfaceDrawable>. >+class DrawingCallbackFromDrawable : public gfxDrawingCallback { >+ virtual PRBool operator()(gfxContext* aContext, >+ const gfxRect& aFillRect, >+ const gfxPattern::GraphicsFilter& aFilter, >+ const gfxMatrix& aTransform = gfxMatrix()) { Add a newline before the brace here too. >+gfxPatternDrawable::Draw(gfxContext* aContext, >+ if (aRepeat) { >+ // We can't use mPattern directly: We want our repeated tiles to have >+ // the size mSize, which might not be the case in mPattern. >+ nsRefPtr<gfxDrawable> cbDrawable = CallbackDrawable(); >+ return cbDrawable->Draw(aContext, aFillRect, PR_TRUE, aFilter, >+ aTransform); >+ } This feels wrong. I've traced the code, and it seems like all we do is eventually call Draw() on ourselves with aRepeat = PR_FALSE. If we can't handle repeat, we should just do so explicitly, instead of hiding it like this. I'd like it for CallbackDrawable() to be removed altogether. >diff --git a/gfx/thebes/gfxDrawable.h b/gfx/thebes/gfxDrawable.h >+/** >+ * gfxDrawingCallback >+ * A simple drawing functor. >+ */ >+class THEBES_API gfxDrawingCallback { Maybe move this functor definition to just above gfxCallbackDrawable? >+ /** >+ * Draw into aContext filling aFillRect, without tiling, using aFilter. >+ * aTransform is a userspace to "image"space matrix. For example, if Draw >+ * draws using a gfxPattern, this is the matrix that should be set on the >+ * pattern prior to rendering it. >+ * @return whether drawing was successful >+ */ >+ virtual PRBool operator()(gfxContext* aContext, >+ const gfxRect& aFillRect, >+ const gfxPattern::GraphicsFilter& aFilter, >+ const gfxMatrix& aTransform = gfxMatrix())=0; Add spaces in here, "= 0;". Also, I don't think it's necessary to say "without tiling" here, since it doesn't make any sense for a drawing callback, which is conceptually of infinite size, to tile. >+ >+}; >+ >+/** >+ * gfxDrawable >+ * An Interface representing something that has an intrinsic size and can draw >+ * itself repeatedly. >+ */ >+class THEBES_API gfxDrawable { >+ NS_INLINE_DECL_REFCOUNTING(gfxDrawable) >+public: >+ gfxDrawable(const gfxIntSize aSize) >+ : mSize(aSize) {} >+ virtual ~gfxDrawable() {} >+ >+ /** >+ * Draw into aContext filling aFillRect, possibly repeating, using aFilter. >+ * aTransform is a userspace to "image"space matrix. For example, if Draw >+ * draws using a gfxPattern, this is the matrix that should be set on the >+ * pattern prior to rendering it. >+ * @return whether drawing was successful >+ */ >+ virtual PRBool Draw(gfxContext* aContext, >+ const gfxRect& aFillRect, >+ PRBool aRepeat, >+ const gfxPattern::GraphicsFilter& aFilter, >+ const gfxMatrix& aTransform = gfxMatrix())=0; Add spaces here, ") = 0;". >+/** >+ * gfxSurfaceDrawable >+ * A convenience implementation of gfxDrawable for callbacks. >+ */ Whoops, copy and paste error here - gfxCallbackDrawable, not gfxSurfaceDrawable. r- because of the CallbackDrawable() stuff, but it's otherwise great -- exactly the changes I wanted in the earlier review!
Attachment #455483 - Flags: review?(joe) → review-
Comment on attachment 455486 [details] [diff] [review] part 5, updated to trunk >diff --git a/gfx/thebes/gfxUtils.cpp b/gfx/thebes/gfxUtils.cpp >+ nsRefPtr<gfxDrawable> restrictedPattern = Maybe rename this restrictedDrawable? It's not really important, but it's what it actually is. If you think that way too, you should probably rename CreateSamplingRestrictedPattern to CreateSamplingRestrictedDrawable. Great otherwise.
Attachment #455486 - Flags: review?(joe) → review+
(In reply to comment #26) > >+gfxCallbackDrawable::Draw(gfxContext* aContext, > >+ const gfxRect& aFillRect, > >+ PRBool aRepeat, > >+ const gfxPattern::GraphicsFilter& aFilter, > >+ const gfxMatrix& aTransform) > >+{ > >+ if (aRepeat) { > >+ MakeSurfaceDrawable(aFilter); > > Here, we should say mSurfaceDrawable = MakeSurfaceDrawable(...), and have > MakeSurfaceDrawable just return an already_AddRefed<gfxSurfaceDrawable>. Where should I put the if (!mSurfaceDrawable) {} caching guard, then? > >+gfxPatternDrawable::Draw(gfxContext* aContext, > > >+ if (aRepeat) { > >+ // We can't use mPattern directly: We want our repeated tiles to have > >+ // the size mSize, which might not be the case in mPattern. > >+ nsRefPtr<gfxDrawable> cbDrawable = CallbackDrawable(); > >+ return cbDrawable->Draw(aContext, aFillRect, PR_TRUE, aFilter, > >+ aTransform); > >+ } > > This feels wrong. I've traced the code, and it seems like all we do is > eventually call Draw() on ourselves with aRepeat = PR_FALSE. Right. We construct a surface (done by gfxCallbackDrawable), paint into that surface by calling Draw() with aRepeat = PR_FALSE on ourselves, create a pattern with the resulting surface and repeat that pattern (done by gfxSurfaceDrawable). > If we can't handle repeat, we should just do so explicitly, What exactly do you mean? Drawing to a surface, putting that into a pattern and repeating that pattern is exactly what needs to be done in this case, so do you want me to duplicate that code here? > I'd like it for CallbackDrawable() to be removed altogether. I need it in bug 506826 part 3 for normal HTML paint servers: If we have background-image:-moz-element(#someelement), we use a callback to draw #someelement. If we don't need to repeat, this can happen without a temporary surface by using gfxCallbackDrawable::Draw() with aRepeat = PR_FALSE; if we *do* need to repeat (controlled by background-repeat) we need to draw into a surface first.
(In reply to comment #28) > (In reply to comment #26) > > >+gfxCallbackDrawable::Draw(gfxContext* aContext, > > >+ const gfxRect& aFillRect, > > >+ PRBool aRepeat, > > >+ const gfxPattern::GraphicsFilter& aFilter, > > >+ const gfxMatrix& aTransform) > > >+{ > > >+ if (aRepeat) { > > >+ MakeSurfaceDrawable(aFilter); > > > > Here, we should say mSurfaceDrawable = MakeSurfaceDrawable(...), and have > > MakeSurfaceDrawable just return an already_AddRefed<gfxSurfaceDrawable>. > > Where should I put the if (!mSurfaceDrawable) {} caching guard, then? In the same block (if (!mSurfaceDrawable) mSurfaceDrawable = ...). The thing I'm objecting to is the fact that it hides the initialization of mSurfaceDrawable. > > >+gfxPatternDrawable::Draw(gfxContext* aContext, > > > > >+ if (aRepeat) { > > >+ // We can't use mPattern directly: We want our repeated tiles to have > > >+ // the size mSize, which might not be the case in mPattern. > > >+ nsRefPtr<gfxDrawable> cbDrawable = CallbackDrawable(); > > >+ return cbDrawable->Draw(aContext, aFillRect, PR_TRUE, aFilter, > > >+ aTransform); > > >+ } > > > > This feels wrong. I've traced the code, and it seems like all we do is > > eventually call Draw() on ourselves with aRepeat = PR_FALSE. > > Right. We construct a surface (done by gfxCallbackDrawable), paint into that > surface by calling Draw() with aRepeat = PR_FALSE on ourselves, create a > pattern with the resulting surface and repeat that pattern (done by > gfxSurfaceDrawable). Hm, I must have misread. How about you change the comment in the aRepeat case to say that? :)
Attached patch part 3, v3 (deleted) — Splinter Review
Attachment #453029 - Attachment is obsolete: true
Attached patch part 4, gfxDrawable (obsolete) (deleted) — Splinter Review
Attachment #455483 - Attachment is obsolete: true
Attachment #456467 - Flags: review?(joe)
Attached patch part 5 (deleted) — Splinter Review
Attachment #455486 - Attachment is obsolete: true
Attachment #456467 - Flags: review?(joe) → review+
code simplification + necessary groundwork for -moz-element (bug 506826)
Attachment #459475 - Flags: approval2.0?
Comment on attachment 459475 [details] [diff] [review] part 3 to 6 combined for approval I'll request approval when I've finished the rest of the -moz-element work.
Attachment #459475 - Flags: approval2.0?
Attached patch part 4, gfxDrawable (deleted) — Splinter Review
I had to make a change to this patch in order to fix a leak caused by a reference cycle. gfxPatternDrawable now no longer caches the gfxCallbackDrawable it creates when drawing with repeat; otherwise there would be a cycle like this: gfxPatternDrawable::mCallbackDrawable -> gfxCallbackDrawable::mCallback -> DrawingCallbackFromDrawable::mDrawable -> gfxPatternDrawable
Attachment #456467 - Attachment is obsolete: true
Depends on: 600476
This is believed to have caused a ~1MB/tab memory usage increase, see: bug 598466 comment 106
Depends on: 616280
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: