Closed
Bug 572680
Opened 14 years ago
Closed 14 years ago
Refactor image drawing
Categories
(Core :: Graphics, defect)
Core
Graphics
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)
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #451934 -
Flags: review?(roc)
Assignee | ||
Comment 2•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #451937 -
Flags: review? → review?(joe)
Assignee | ||
Comment 3•14 years ago
|
||
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)
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Comment 6•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #451937 -
Flags: review?(joe) → review+
Comment 7•14 years ago
|
||
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 8•14 years ago
|
||
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 9•14 years ago
|
||
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 12•14 years ago
|
||
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)
Assignee | ||
Comment 13•14 years ago
|
||
(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)
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #451940 -
Attachment is obsolete: true
Attachment #453029 -
Flags: review?(joe)
Assignee | ||
Comment 15•14 years ago
|
||
(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?
Assignee | ||
Comment 16•14 years ago
|
||
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)
Assignee | ||
Comment 17•14 years ago
|
||
Updated for changes in part 3 and to use a gfxSurfaceDrawable in CreateSamplingRestrictedPattern.
Attachment #451944 -
Attachment is obsolete: true
Assignee | ||
Comment 18•14 years ago
|
||
(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.
Hmm, I guess that's right.
Attachment #453023 -
Flags: review?(roc) → review+
Attachment #451945 -
Flags: review?(roc) → review+
+ 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.
Assignee | ||
Comment 21•14 years ago
|
||
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)
Assignee | ||
Comment 22•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #451937 -
Attachment description: part 2: shuffle some gfx dependencies → part 2: shuffle some gfx dependencies [landed]
Assignee | ||
Updated•14 years ago
|
Attachment #453023 -
Attachment description: part 1, v2 → part 1, v2 [landed]
Assignee | ||
Comment 23•14 years ago
|
||
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)
Assignee | ||
Comment 24•14 years ago
|
||
Attachment #453032 -
Attachment is obsolete: true
Attachment #455486 -
Flags: review?(joe)
Comment 25•14 years ago
|
||
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 26•14 years ago
|
||
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 27•14 years ago
|
||
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+
Assignee | ||
Comment 28•14 years ago
|
||
(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.
Comment 29•14 years ago
|
||
(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? :)
Assignee | ||
Comment 30•14 years ago
|
||
Attachment #453029 -
Attachment is obsolete: true
Assignee | ||
Comment 31•14 years ago
|
||
Attachment #455483 -
Attachment is obsolete: true
Attachment #456467 -
Flags: review?(joe)
Assignee | ||
Comment 32•14 years ago
|
||
Attachment #455486 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #456467 -
Flags: review?(joe) → review+
Assignee | ||
Comment 33•14 years ago
|
||
code simplification + necessary groundwork for -moz-element (bug 506826)
Attachment #459475 -
Flags: approval2.0?
Assignee | ||
Comment 34•14 years ago
|
||
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?
Assignee | ||
Comment 35•14 years ago
|
||
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
Assignee | ||
Comment 36•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/97188fb7b44a
http://hg.mozilla.org/mozilla-central/rev/0fa683b7233d
http://hg.mozilla.org/mozilla-central/rev/b5f727a62c7c
http://hg.mozilla.org/mozilla-central/rev/91f0d2cd19e8
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
Comment 37•14 years ago
|
||
This is believed to have caused a ~1MB/tab memory usage increase, see:
bug 598466 comment 106
You need to log in
before you can comment on or make changes to this bug.
Description
•