This bug tracks the implementation of the Direct2D Azure backend.

See also the feature page: https://wiki.mozilla.org/Platform/Features/AzureD2DCanvas

This is a patch of the azure API and Direct2D backend rolled into a single patch for review tracking. Review is probably best done straight on the project branch.

Comment on attachment 538051 [details] [diff] [review] Azure API and D2D backend I'm just going to stream comments as I have them. 1. It would be to come up with a better name than FinalizeRTForOperator. The current name doesn't really suggest that drawing happens. 2. Make sure you hook up ReportFailure 3. FILTER_NEAREST is more familiar terminology than FILTER_POINT 4. Some enums are initialized starting at 0. There's no need for this.

(In reply to comment #3) > Comment on attachment 538051 [details] [diff] [review] [review] > Azure API and D2D backend > 3. FILTER_NEAREST is more familiar terminology than FILTER_POINT Maybe in 2D API's it is, I'm not sure, in signal analysis it certainly isn't :). 'Nearest' is not a filter, it might be a sampling method, in that case we should name it 'SAMPLING_NEAREST' or something like that, like Direct2D does. I personally wouldn't like to use something as 'wrong' terminology wise as FILTER_NEAREST.

Comment on attachment 538051 [details] [diff] [review] Azure API and D2D backend Review of attachment 538051 [details] [diff] [review]: ----------------------------------------------------------------- BasePoint2D/BaseMargin/BaseRect/BaseSize need to be integrated with the versions that are currently in gfx/src. Maybe the ones in gfx/src need to move into gfx/2d and you use those. So I'm not going to look at those right now. RefPtr and RefCountable should be in mfbt (see bug 661973) (with changes; e.g. RefCountable should probably be RefCounted<T> for greater efficiency and naming compatibility with Webkit), and Azure should depend on mfbt. So I won't review your implementations here. I think we should probably go with Point/IntPoint/Matrix without the 2D suffix. The rest of our code assumes 2D and adds a "3D" suffix for the 3D objects, and we may as well follow that. I think we need a BaseMatrix template that your Matrix and gfxMatrix can both derive from, with the API of gfxMatrix. I'm sorry if you don't like gfxMatrix, but I recently spent a lot of time unifying our Point/Size/Margin/Rect types because the previous set of gfx hackers didn't like the existing types, and I don't want to go through that again. So I'm leaving Matrix2D aside for now too. We don't have any equivalent to the cairo pattern matrix. You can get most pattern transforms by setting the user-space transform and adjusting the path to compensate, but for example it seems there is no way to transform a pattern independent of the stroke width. Right now I'm not sure if that matters to us, though, so let's not worry about it. We can add a pattern transform later (possibly just on SurfacePattern and/or GradientPatterns) if necessary. ::: gfx/2d/2D.h @@ +81,5 @@ > + * mAntiAliasMode - The AntiAlias mode used for this drawing operation. > + * mSnapping - Whether this operation is snapped to pixel boundaries. > + */ > +struct DrawOptions { > + DrawOptions(float aAlpha = 1.0f, Float? @@ +97,5 @@ > + AntialiasMode mAntialiasMode : 2; > + Snapping mSnapping : 1; > +}; > + > +struct StrokeOptions { Needs a comment @@ +138,5 @@ > +{ > +public: > + virtual ~GradientStops() {} > + > + virtual BackendType GetBackendType() const = 0; Wouldn't it be better to store the backend type as a field, so checking it doesn't require a virtual call? I'm sure we don't care about the extra space. @@ +147,5 @@ > + > +/* > + * This is the base class for 'patterns' patterns describe the pixels used as > + * the source for a masked composition operation that is done by the different > + * drawing commands. Need to make it clear here that Patterns are not backend-specific, unlike most of our other objects. @@ +154,5 @@ > +{ > +public: > + virtual ~Pattern() {} > + > + virtual PatternType Type() const = 0; As above, I think this should be a field with an inline getter. Need to be consistent about whether you use Get as the getter prefix. I suggest you do. @@ +163,5 @@ > + > +class ColorPattern : public Pattern > +{ > +public: > + ColorPattern() {} Why do we want to allow an empty constructor? @@ +175,5 @@ > + Color mColor; > +}; > + > +/* > + * This stack-based class is used for Linear Gradient Patterns, the gradient I don't think we need to describe these classes as "stack-based". We need to say they can be used on the stack, but they don't have to be. @@ +181,5 @@ > + */ > +class LinearGradientPattern : public Pattern > +{ > +public: > + LinearGradientPattern() {} Again, why is this needed? @@ +224,5 @@ > + * backend type of the draw target this pattern will be used with. > + */ > + RadialGradientPattern(const Point2D &aCenter, > + const Point2D &aOrigin, > + Float aRadius) Why do we only have one radius here? Shouldn't we have two, one for each circle? The comment is inconsistent with the names in the code, but I suggest changing the names in the code. It's totally unclear what aCenter and aOrigin mean here. @@ +264,5 @@ > + Filter mFilter; > +}; > + > +/* > + * This base class is implemented by SourceSurfaces, these objects are surfaces "This is the base class for source surfaces. These objects are surfaces" You have a habit of starting a new sentence after a comma, that habit is annoying and ungrammatical :-) @@ +275,5 @@ > + virtual ~SourceSurface() {} > + > + virtual SurfaceType Type() const = 0; > + virtual IntSize GetSize() const = 0; > + virtual SurfaceFormat GetFormat() const = 0; I suspect these should all be fields with inline accessors. @@ +291,5 @@ > + /* Get the raw bitmap data of the surface */ > + virtual unsigned char *GetData() = 0; > + /* > + * Stride of the surface, distance in bytes between the start of the image > + * data belonging to row y and row y+1. Can it be negative? Say whether it can be or not. @@ +293,5 @@ > + /* > + * Stride of the surface, distance in bytes between the start of the image > + * data belonging to row y and row y+1. > + */ > + virtual uint32_t Stride() = 0; Fields, I think. @@ +298,5 @@ > + > + virtual RefPtr<DataSourceSurface> GetDataSurface() { return this; } > +}; > + > +/* This class provides write-only access to a path during its lifetime. */ "its" is ambiguous here. For this class, I think you should just say that it's an abstract object that accepts path segments. PathSink itself doesn't have any requirements to be related to a particular path. @@ +324,5 @@ > + */ > + virtual void Close() = 0; > + /* Add an arc to the current figure */ > + virtual void Arc(const Point2D &aOrigin, float aRadius, float aStartAngle, > + float aEndAngle, bool aAntiClockwise = false) = 0; Shouldn't these be Float instead of float? @@ +341,5 @@ > +{ > +public: > + virtual ~Path() {} > + > + virtual BackendType GetBackendType() const = 0; Field @@ +346,5 @@ > + > + /* This returns a PathBuilder object that contains a copy of the contents of > + * this path and is still writable. > + */ > + virtual RefPtr<PathBuilder> GetPathBuilder() const = 0; Call this CopyPath instead? Assuming it always creates a new object, it certainly shouldn't be Get. @@ +347,5 @@ > + /* This returns a PathBuilder object that contains a copy of the contents of > + * this path and is still writable. > + */ > + virtual RefPtr<PathBuilder> GetPathBuilder() const = 0; > + virtual RefPtr<PathBuilder> GetTransformedBuilder(const Matrix2D &aTransform) const = 0; CopyPathWithTransform? @@ +349,5 @@ > + */ > + virtual RefPtr<PathBuilder> GetPathBuilder() const = 0; > + virtual RefPtr<PathBuilder> GetTransformedBuilder(const Matrix2D &aTransform) const = 0; > + > + virtual bool ContainsPoint(const Point2D &aPoint, const Matrix2D &aTransform) const = 0; Need a comment to describe how the transform is used @@ +356,5 @@ > +class PathBuilder : public PathSink > +{ > +public: > + /* Finish writing to the path and return a Path object that can be used for > + * drawing Note that future writes to the PathSink will be ignored and future calls to Finish() will return null (or if you prefer, return the same path). @@ +363,5 @@ > +}; > + > +struct Glyph > +{ > + uint16_t mIndex; Should be a uint32_t, I think. No win from it being a uint16_t here, anyway. @@ +365,5 @@ > +struct Glyph > +{ > + uint16_t mIndex; > + Point2D mRelativePosition; > + Float mAdvance; Why do you need the advance as well as the relative position here? For this simple glyph buffer, why not just have an mPosition for each glyph? @@ +372,5 @@ > +/* This class functions as a glyph buffer that can be drawn to a DrawTarget. > + * XXX - This should probably contain the guts of gfxTextRun in the future as > + * roc suggested. But for now it's a simple container for a glyph vector. > + */ > +class GlyphBuffer Make this a struct if you expect direct access to the members. @@ +381,5 @@ > + std::vector<Glyph> mGlyphs; > + bool mIsRTL; > +}; > + > +class ScaledFont : public RefCountable Needs a comment explaining exactly what ScaledFont is an abstraction of. @@ +386,5 @@ > +{ > +public: > + virtual ~ScaledFont() {} > + > + virtual FontType Type() const = 0; Field @@ +388,5 @@ > + virtual ~ScaledFont() {} > + > + virtual FontType Type() const = 0; > + > + virtual RefPtr<Path> GetPathForGlyphs(const GlyphBuffer &aBuffer, const DrawTarget *aTarget) = 0; Add a comment explaining the role of aTarget here. @@ +394,5 @@ > +protected: > + ScaledFont() {} > +}; > + > +class DrawTarget : public RefCountable Definitely needs a comment explaining what this class is! @@ +400,5 @@ > +public: > + DrawTarget() : mTransformDirty(false) {} > + virtual ~DrawTarget() {} > + > + virtual BackendType Type() const = 0; Field @@ +402,5 @@ > + virtual ~DrawTarget() {} > + > + virtual BackendType Type() const = 0; > + virtual RefPtr<SourceSurface> Snapshot() = 0; > + virtual IntSize GetSize() = 0; Probably doesn't need a field, since I don't know who's going to use it @@ +417,5 @@ > + * > + * aSurface Source surface to draw > + * aDest Destination rectangle that this drawing operation should draw to > + * aSource Source rectangle in aSurface coordinates, this area of aSurface > + * will be stretched to the size of aDest. Do we guarantee not to sample outside of aSource? I very very much hope the answer is yes! @@ +425,5 @@ > + virtual void DrawSurface(SourceSurface *aSurface, > + const Rect &aDest, > + const Rect &aSource, > + const DrawOptions &aOptions = DrawOptions(), > + const DrawSurfaceOptions &aSurfOptions = DrawSurfaceOptions()) = 0; Should we make DrawSurfaceOptions inherit from DrawOptions so you only need to pass one parameter here? If not, then DrawSurfaceOptions should come before DrawOptions for consistency with the stroke APIs. @@ +430,5 @@ > + > + /* > + * Blend a surface to the draw target with a shadow. The shadow is drawn as a > + * gaussian blur using a specified sigma. > + * NOTE: This function works in device space! Why? Can we not make it work in user space to be consistent? @@ +433,5 @@ > + * gaussian blur using a specified sigma. > + * NOTE: This function works in device space! > + * > + * aSurface Source surface to draw. > + * aDest Destination rectangle that this drawing operation should draw to. So the entire surface is stretched to fill aDest? Or what? @@ +465,5 @@ > + */ > + virtual void StrokeRect(const Rect &aRect, > + const Pattern &aPattern, > + const StrokeOptions &aStrokeOptions = StrokeOptions(), > + const DrawOptions &aOptions = DrawOptions()) = 0; We could make StrokeOptions inherit from DrawOptions so only one parameter is needed. Or are we going to add non-drawing operations like convert-stroke-to-path that would take a StrokeOptions but not a DrawOptions? @@ +475,5 @@ > + * aEnd End point of the line > + * aPattern Pattern that forms the source of this stroking operation > + * aOptions Options that are applied to this operation > + */ > + virtual void StrokeLine(const Point2D &aStart, Who uses StrokeLine? I don't know of any use for it. @@ +510,5 @@ > + * Fill a series of clyphs on the draw target with a certain source pattern. > + */ > + virtual void FillGlyphs(ScaledFont *aFont, > + const GlyphBuffer &aBuffer, > + const Point2D &aBaselineOrigin, If we make aBuffer contain absolute positions, we would get rid of this parameter. @@ +516,5 @@ > + const DrawOptions &aOptions = DrawOptions()) = 0; > + > + /* > + * Push a clip to the DrawTarget. Performing a larger amounts of pops than > + * clips will cause the pops to be silently ignored. The second sentence should move to PopClip. And you would say "A pop without a corresponding Push is ignored." @@ +535,5 @@ > + SurfaceFormat aFormat) const = 0; > + > + /* > + * Create a SourceSurface optimized for use with this DrawTarget from > + * an arbitrary other SourceSurface. Add "May return aSourceSurface or some other existing surface." @@ +540,5 @@ > + */ > + virtual RefPtr<SourceSurface> OptimizeSourceSurface(SourceSurface *aSurface) const = 0; > + > + /* > + * Create a SourceSurface for a type of NativeSurface, this may fail if the The dreaded comma! @@ +551,5 @@ > + /* > + * Create a DrawTarget whose snapshot is optimized for use with this DrawTarget. > + */ > + virtual RefPtr<DrawTarget> > + CreateOptimalDrawTarget(const IntSize &aSize, SurfaceFormat aFormat) const = 0; "Optimal" is overselling things. How about "CreateSimilarDrawTarget"? @@ +586,5 @@ > +#ifdef USE_CAIRO > + static RefPtr<DrawTarget> CreateDrawTargetForCairoSurface(cairo_surface_t* aSurface); > +#endif > + > + static RefPtr<DrawTarget> CreateDrawTarget(BackendType aBackend, const IntSize &aSize, SurfaceFormat aFormat); I'm not sure how this would work in general. What does it mean to call this with aBackend == BACKEND_CAIRO? Would we call gfxPlatform::CreateOffscreenSurface or something like that? Who actually needs this? @@ +599,5 @@ > + static ID3D10Device1 *mD3D10Device; > +#endif > +}; > + > +extern Factory sFactory; What's this for? I don't think it should be exposed. ::: gfx/2d/Matrix2D.h @@ +98,5 @@ > + > + return *this; > + } > + > + Matrix2D &Translate(Float aX, Float aY) Comment as for Scale. ::: gfx/2d/Types.h @@ +34,5 @@ > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ > + > +#pragma once I think we don't rely on #pragma once, use #ifdefs instead. @@ +50,5 @@ > + DATA = 0, /* Data surface - bitmap in memory */ > + D2D1_BITMAP, /* Surface wrapping a ID2D1Bitmap */ > + D2D1_DRAWTARGET, /* Surface made from a D2D draw target */ > + CAIRO_SURFACE, /* Surface wrapping a cairo surface */ > + COREGRAPHICS_IMAGE /* Surface wrapping a CoreGraphics Image */ I think SURFACE_ prefixes on all these. Which means of course, SURFACE_CAIRO and SURFACE_COREGRAPHICS_IMAGE. @@ +57,5 @@ > +enum SurfaceFormat > +{ > + B8G8R8A8 = 0, > + B8G8R8X8, > + A8, I think FORMAT_ prefixes on all these. You need comments explaining the actual byte layout. @@ +64,5 @@ > +enum BackendType > +{ > + DIRECT2D = 0, > + COREGRAPHICS, > + CAIRO, BACKEND_DIRECT2D @@ +69,5 @@ > +}; > + > +enum FontType > +{ > + DWRITE = 0, FONT_DWRITE @@ +74,5 @@ > +}; > + > +enum NativeSurfaceType > +{ > + D3D10_TEXTURE = 0, NATIVE_SURFACE_D3D10_TEXTURE @@ +79,5 @@ > +}; > + > +enum NativeFontType > +{ > + DWRITE_FONT_FACE = 0, NATIVE_FONT_DWRITE @@ +82,5 @@ > +{ > + DWRITE_FONT_FACE = 0, > +}; > + > +enum CompositionOp { OP_OVER = 0, OP_ADD, OP_ATOP, OP_OUT, OP_IN, OP_SOURCE, OP_CLEAR, OP_DEST_IN, OP_DEST_OUT, OP_DEST_OVER, OP_DEST_ATOP, OP_XOR, MAX_COMPOSITIONOP }; OP_MAX ... and shouldn't OP_MAX = OP_XOR? @@ +88,5 @@ > +enum FillRule { FILL_WIND = 0, FILL_EVEN_ODD }; > +enum AntialiasMode { AA_NONE = 0, AA_GRAY, AA_SUBPIXEL }; > +enum Snapping { SNAP_NONE = 0, SNAP_ALIGNED }; > +enum Filter { FILTER_LINEAR = 0, FILTER_POINT }; > +enum PatternType { COLOR, SURFACE, LINEAR_GRADIENT, RADIAL_GRADIENT }; PATTERN_COLOR @@ +109,5 @@ > + { > + r = ((aColor >> 0) & 0xff) * (1.0f / 255.0f); > + g = ((aColor >> 8) & 0xff) * (1.0f / 255.0f); > + b = ((aColor >> 16) & 0xff) * (1.0f / 255.0f); > + a = ((aColor >> 24) & 0xff) * (1.0f / 255.0f); Instead of having a constructor I would rather have a named static method that includes the format, e.g. Color::FromABGR(PRUint32). We use both ABGR and ARGB and I think we should call that out explicitly. (gfxRGBA has a constructor with a format parameter, which is dumb.)

Comment on attachment 538051 [details] [diff] [review] Azure API and D2D backend 5. Drop QuadraticBezierTo for now. There are no users and not all backends natively support them. 6. DrawSurfaceWithShadow is really long, can you split it up. 7. GetRTForOperator and FinalizeRTForOperator could use a summary of what they're doing. 8. Do we expect either of these conditions to ever be true? if (!pat->mSurface || pat->mSurface->Type() != D2D1_BITMAP) { return NULL; } if not perhaps adding a warning would be good. 9. hr = createD3DEffect((void*)g_main, sizeof(g_main), 0, mDevice, NULL, &mPrivateData->mEffect); Let's use a better name than g_main.

I limited my comments to where I disagree or have other things to say :). (In reply to comment #5) > Comment on attachment 538051 [details] [diff] [review] [review] > Azure API and D2D backend > > Review of attachment 538051 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > BasePoint2D/BaseMargin/BaseRect/BaseSize need to be integrated with the > versions that are currently in gfx/src. Maybe the ones in gfx/src need to > move into gfx/2d and you use those. So I'm not going to look at those right > now. Moving the ones in gfxSrc into gfx/2d is certainly my plan. As a matter of fact these BasePoint2D/etc. are exact copies of those. The only reason I didn't move them yet is because when I started this code I had to rip them out of your patch (they hadn't landed yet). > RefPtr and RefCountable should be in mfbt (see bug 661973) (with changes; > e.g. RefCountable should probably be RefCounted<T> for greater efficiency > and naming compatibility with Webkit), and Azure should depend on mfbt. So I > won't review your implementations here. Right now Azure does not depend on our build system. This has major advantages, first of all it can easily be contributed to and used by third parties. Which given the interest people have shown is a great thing, additionally it means it can be rebuilt very quickly and in a better environment on windows when developing, having greatly reduced the time I spent building it. If we cannot maintain these advantages my suggestion would be to do some #ifdef magic that makes them use the mfbt ones in a mozilla build and the current ones without. I'm also not sure RefCounted<T> is better than RefCountable, but I might be convinced of that. One of the issues is you don't want AddRef and Release to ever be inlined. > I think we should probably go with Point/IntPoint/Matrix without the 2D > suffix. The rest of our code assumes 2D and adds a "3D" suffix for the 3D > objects, and we may as well follow that. I don't think that's a good idea, let's be explicit. It's weird to add a suffix in one case and not in the other. > > I think we need a BaseMatrix template that your Matrix and gfxMatrix can > both derive from, with the API of gfxMatrix. I'm sorry if you don't like > gfxMatrix, but I recently spent a lot of time unifying our > Point/Size/Margin/Rect types because the previous set of gfx hackers didn't > like the existing types, and I don't want to go through that again. So I'm > leaving Matrix2D aside for now too. This actually has a performance impact if the layout of gfxMatrix is not compatible with Matrix2D (Matrix2D's layout is meant to map well to OGL/D3D/D2D). I'm not sure if that's true though so provided it's not true I think I'm fine with this. > > We don't have any equivalent to the cairo pattern matrix. You can get most > pattern transforms by setting the user-space transform and adjusting the > path to compensate, but for example it seems there is no way to transform a > pattern independent of the stroke width. Right now I'm not sure if that > matters to us, though, so let's not worry about it. We can add a pattern > transform later (possibly just on SurfacePattern and/or GradientPatterns) if > necessary. Yeah, this is true, I simply didn't add it because we don't need it, Direct2D has support, so does cairo, so I don't see any issues here. > @@ +138,5 @@ > > +{ > > +public: > > + virtual ~GradientStops() {} > > + > > + virtual BackendType GetBackendType() const = 0; > > Wouldn't it be better to store the backend type as a field, so checking it > doesn't require a virtual call? I'm sure we don't care about the extra space. I don't think we care about the virtualization either :). The big advantage this has in my opinion is that when keeping it as a field mistakes lead to annoying, hard to find bugs. I could go either way though, I certainly prefer this style. > > @@ +154,5 @@ > > +{ > > +public: > > + virtual ~Pattern() {} > > + > > + virtual PatternType Type() const = 0; > > As above, I think this should be a field with an inline getter. > > Need to be consistent about whether you use Get as the getter prefix. I > suggest you do. I wholly agree, this caused the inconsistency, our original API proposal had Type() so I used that instead of GetType, except when I was following my own preference. Anyway, regardless I will fix this. > > @@ +163,5 @@ > > + > > +class ColorPattern : public Pattern > > +{ > > +public: > > + ColorPattern() {} > > Why do we want to allow an empty constructor? The reason I did this was so GeneralPattern in the Azure2D context could have all 4 types as members without forcing it to initialize them when it wasn't going to use them. I'm willing to remove this but it does cause a little bit of extra initialization work. > @@ +224,5 @@ > > + * backend type of the draw target this pattern will be used with. > > + */ > > + RadialGradientPattern(const Point2D &aCenter, > > + const Point2D &aOrigin, > > + Float aRadius) > > Why do we only have one radius here? Shouldn't we have two, one for each > circle? > > The comment is inconsistent with the names in the code, but I suggest > changing the names in the code. It's totally unclear what aCenter and > aOrigin mean here. Yes, radial gradients need fixing, it's a hard problem and will require some work (as D2D doesn't allow for the fundamental type of radial gradient we need). > @@ +275,5 @@ > > + virtual ~SourceSurface() {} > > + > > + virtual SurfaceType Type() const = 0; > > + virtual IntSize GetSize() const = 0; > > + virtual SurfaceFormat GetFormat() const = 0; > > I suspect these should all be fields with inline accessors. No, I don't think so, for example in D2D(and also in D3D) we want to get them directly off the texture description in my opinion. Not store everything twice. Type of course is different. > @@ +291,5 @@ > > + /* Get the raw bitmap data of the surface */ > > + virtual unsigned char *GetData() = 0; > > + /* > > + * Stride of the surface, distance in bytes between the start of the image > > + * data belonging to row y and row y+1. > > Can it be negative? Say whether it can be or not. It can, I discussed this with Jeff. I'll include it in the comment but I thought the type would make it obvious. Actually there's a type inconsistency in the class I'll fix. > > @@ +293,5 @@ > > + /* > > + * Stride of the surface, distance in bytes between the start of the image > > + * data belonging to row y and row y+1. > > + */ > > + virtual uint32_t Stride() = 0; > > Fields, I think. Hmm? > > @@ +341,5 @@ > > +{ > > +public: > > + virtual ~Path() {} > > + > > + virtual BackendType GetBackendType() const = 0; > > Field Hmm? > > @@ +346,5 @@ > > + > > + /* This returns a PathBuilder object that contains a copy of the contents of > > + * this path and is still writable. > > + */ > > + virtual RefPtr<PathBuilder> GetPathBuilder() const = 0; > > Call this CopyPath instead? Assuming it always creates a new object, it > certainly shouldn't be Get. Copy is not a very accurate description though, because the result is not a path but a path builder. Maybe CopyToPathBuilder? or something like that? > > @@ +356,5 @@ > > +class PathBuilder : public PathSink > > +{ > > +public: > > + /* Finish writing to the path and return a Path object that can be used for > > + * drawing > > Note that future writes to the PathSink will be ignored and future calls to > Finish() will return null (or if you prefer, return the same path). They actually won't be ignored, they'll cause an exception :). > @@ +365,5 @@ > > +struct Glyph > > +{ > > + uint16_t mIndex; > > + Point2D mRelativePosition; > > + Float mAdvance; > > Why do you need the advance as well as the relative position here? For this > simple glyph buffer, why not just have an mPosition for each glyph? This is how text run stores them and gives them to us. I think this is good because it's the representation every API I've found except cairo uses :). And I'm sure these APIs have good reasons to want the info that way. In any case converting would be needless work in the D2D case. > @@ +402,5 @@ > > + virtual ~DrawTarget() {} > > + > > + virtual BackendType Type() const = 0; > > + virtual RefPtr<SourceSurface> Snapshot() = 0; > > + virtual IntSize GetSize() = 0; > > Probably doesn't need a field, since I don't know who's going to use it I can certainly foresee users of this. Just like there's users for LayerManager's type field. I think we'll at some point be optimizing stuff for this, i.e. if we know a certain thing is very slow in Cairo for some linux X server, etc. > @@ +417,5 @@ > > + * > > + * aSurface Source surface to draw > > + * aDest Destination rectangle that this drawing operation should draw to > > + * aSource Source rectangle in aSurface coordinates, this area of aSurface > > + * will be stretched to the size of aDest. > > Do we guarantee not to sample outside of aSource? I very very much hope the > answer is yes! Yes, because aSource needs to lie on the source surface, if it doesn't, we go into an error state. > > @@ +425,5 @@ > > + virtual void DrawSurface(SourceSurface *aSurface, > > + const Rect &aDest, > > + const Rect &aSource, > > + const DrawOptions &aOptions = DrawOptions(), > > + const DrawSurfaceOptions &aSurfOptions = DrawSurfaceOptions()) = 0; > > Should we make DrawSurfaceOptions inherit from DrawOptions so you only need > to pass one parameter here? > > If not, then DrawSurfaceOptions should come before DrawOptions for > consistency with the stroke APIs. I think the latter is what we want. > @@ +430,5 @@ > > + > > + /* > > + * Blend a surface to the draw target with a shadow. The shadow is drawn as a > > + * gaussian blur using a specified sigma. > > + * NOTE: This function works in device space! > > Why? Can we not make it work in user space to be consistent? There's a lot of reasons why it's very hard, we'd need different, more complex shaders, and in some cases either need an extra intermediate surface or some very complex filter kernel configuration. In any case we never want this to be in user space for the current Canvas implementation. The intermediate surface should have been drawn to in user space, so its contents will be in device space. > > @@ +433,5 @@ > > + * gaussian blur using a specified sigma. > > + * NOTE: This function works in device space! > > + * > > + * aSurface Source surface to draw. > > + * aDest Destination rectangle that this drawing operation should draw to. > > So the entire surface is stretched to fill aDest? Or what? aDest should be a point, this is a mistake in the comment and the type. It's essentially more of an offset. I should reflect that in the naming. > @@ +465,5 @@ > > + */ > > + virtual void StrokeRect(const Rect &aRect, > > + const Pattern &aPattern, > > + const StrokeOptions &aStrokeOptions = StrokeOptions(), > > + const DrawOptions &aOptions = DrawOptions()) = 0; > > We could make StrokeOptions inherit from DrawOptions so only one parameter > is needed. Or are we going to add non-drawing operations like > convert-stroke-to-path that would take a StrokeOptions but not a DrawOptions? The latter, I believe. On the longer run. > > @@ +475,5 @@ > > + * aEnd End point of the line > > + * aPattern Pattern that forms the source of this stroking operation > > + * aOptions Options that are applied to this operation > > + */ > > + virtual void StrokeLine(const Point2D &aStart, > > Who uses StrokeLine? I don't know of any use for it. Some canvas operations are defined as lines in certain degeneracies (0 height/width fillRects for example). It seemed to make sense, in the future it will be useful for borders as well. > @@ +510,5 @@ > > + * Fill a series of clyphs on the draw target with a certain source pattern. > > + */ > > + virtual void FillGlyphs(ScaledFont *aFont, > > + const GlyphBuffer &aBuffer, > > + const Point2D &aBaselineOrigin, > > If we make aBuffer contain absolute positions, we would get rid of this > parameter. I think that's a bad idea, almost all API's work with an API that lets you pass a baseLine, this probably allows for some optimizations inside those backends that aren't possible otherwise too. > @@ +586,5 @@ > > +#ifdef USE_CAIRO > > + static RefPtr<DrawTarget> CreateDrawTargetForCairoSurface(cairo_surface_t* aSurface); > > +#endif > > + > > + static RefPtr<DrawTarget> CreateDrawTarget(BackendType aBackend, const IntSize &aSize, SurfaceFormat aFormat); > > I'm not sure how this would work in general. What does it mean to call this > with aBackend == BACKEND_CAIRO? Would we call > gfxPlatform::CreateOffscreenSurface or something like that? > > Who actually needs this? Anyone who doesn't want to create a texture, but rather leave that all to the DrawTarget type to decide. A common usecase, just not in our very specific current canvas usage. I see no problem with having it though, some of my local testing code uses it for example just because it's less of a hassle (don't need to create textures, etc.) > @@ +599,5 @@ > > + static ID3D10Device1 *mD3D10Device; > > +#endif > > +}; > > + > > +extern Factory sFactory; > > What's this for? I don't think it should be exposed. It's the class everything outside gfx/2d uses for integration right now. Although it probably shouldn't be done in this particular way going forward, but that's another issue. Matt Woodrow had a decent idea on this I believe. > > ::: gfx/2d/Matrix2D.h > @@ +98,5 @@ > > + > > + return *this; > > + } > > + > > + Matrix2D &Translate(Float aX, Float aY) > > Comment as for Scale. Hey! You weren't going to look at this ;) (but you're right). > ::: gfx/2d/Types.h > @@ +34,5 @@ > > + * the terms of any one of the MPL, the GPL or the LGPL. > > + * > > + * ***** END LICENSE BLOCK ***** */ > > + > > +#pragma once > > I think we don't rely on #pragma once, use #ifdefs instead. Yeah, I talked about that with Jeff, using #pragma once and then just doing a script to convert it to #ifdefs before landing is easier though :). Fwiw I much prefer #pragma once (and GCC de-deprecated it), but yeah. > > @@ +79,5 @@ > > +}; > > + > > +enum NativeFontType > > +{ > > + DWRITE_FONT_FACE = 0, > > NATIVE_FONT_DWRITE NATIVE_FONT_DWRITE_FONTFACE probably, but yeah. > > @@ +82,5 @@ > > +{ > > + DWRITE_FONT_FACE = 0, > > +}; > > + > > +enum CompositionOp { OP_OVER = 0, OP_ADD, OP_ATOP, OP_OUT, OP_IN, OP_SOURCE, OP_CLEAR, OP_DEST_IN, OP_DEST_OUT, OP_DEST_OVER, OP_DEST_ATOP, OP_XOR, MAX_COMPOSITIONOP }; > > OP_MAX ... and shouldn't OP_MAX = OP_XOR? This is just as easy ( <= or < doesn't matter), allows correct array initialization without a +1 (in theory, we don't use that). And it won't break if someone adds an OP at the end without changing the =. Naming wise I agree of course. > @@ +109,5 @@ > > + { > > + r = ((aColor >> 0) & 0xff) * (1.0f / 255.0f); > > + g = ((aColor >> 8) & 0xff) * (1.0f / 255.0f); > > + b = ((aColor >> 16) & 0xff) * (1.0f / 255.0f); > > + a = ((aColor >> 24) & 0xff) * (1.0f / 255.0f); > > Instead of having a constructor I would rather have a named static method > that includes the format, e.g. Color::FromABGR(PRUint32). We use both ABGR > and ARGB and I think we should call that out explicitly. (gfxRGBA has a > constructor with a format parameter, which is dumb.) Indeed, I was just trying to mimic gfxRGBA closely for consistency. I most certainly agree.

(In reply to comment #6) > Comment on attachment 538051 [details] [diff] [review] [review] > Azure API and D2D backend > > 5. Drop QuadraticBezierTo for now. There are no users and not all backends > natively support them. Conversion to a cubic bezier is trivial (conversion of the center control point by 2 different offsets to get the 2 control points for a cubic spline). Canvas has a drawing op http://www.w3.org/TR/2010/WD-2dcontext-20100304/#dom-context-2d-quadraticcurveto. I'd prefer to just leave it, and doing conversion in backends that need it. But I don't mind removing it either. > > 6. DrawSurfaceWithShadow is really long, can you split it up. My one attempt at this made it rather hard to keep track of what it was doing :s so I settled for this. I'll give it another shot though. The problem is there's a lot of state to be kept and used throughout the function, so helper functions get large amounts of output parameters etc. > 8. Do we expect either of these conditions to ever be true? > if (!pat->mSurface || pat->mSurface->Type() != D2D1_BITMAP) { > return NULL; > } > if not perhaps adding a warning would be good. I'll have a look. > 9. hr = createD3DEffect((void*)g_main, sizeof(g_main), 0, mDevice, NULL, > &mPrivateData->mEffect); > > Let's use a better name than g_main. I'll need to check if fxc allows specifying a name, it seems overkill to have to run a post-processing script on the generated header from fxc just to get a prettier name :).

(In reply to comment #7) > > RefPtr and RefCountable should be in mfbt (see bug 661973) (with changes; > > e.g. RefCountable should probably be RefCounted<T> for greater efficiency > > and naming compatibility with Webkit), and Azure should depend on mfbt. So I > > won't review your implementations here. > > Right now Azure does not depend on our build system. This has major > advantages, first of all it can easily be contributed to and used by third > parties. Which given the interest people have shown is a great thing, > additionally it means it can be rebuilt very quickly and in a better > environment on windows when developing, having greatly reduced the time I > spent building it. If we cannot maintain these advantages my suggestion > would be to do some #ifdef magic that makes them use the mfbt ones in a > mozilla build and the current ones without. I'm also not sure RefCounted<T> > is better than RefCountable, but I might be convinced of that. One of the > issues is you don't want AddRef and Release to ever be inlined. Azure isn't going have its own RefPtr/RefCounted. I insist very firmly on that. Can't you pull the mfbt files (once they're created!) into the Azure stand-alone build? We're only talking about a few header files. I'm not sure why you want AddRef/Release to *never* be inlined. Surely there could be hot code paths where that would be beneficial. > > I think we should probably go with Point/IntPoint/Matrix without the 2D > > suffix. The rest of our code assumes 2D and adds a "3D" suffix for the 3D > > objects, and we may as well follow that. > > I don't think that's a good idea, let's be explicit. It's weird to add a > suffix in one case and not in the other. It's what we already do, and we aren't going to rename nsPoint to nsPoint2D, probably ever. I don't find it weird; browsers use 2D geometry a lot, 3D much less so, so the default should be the 2D version. Azure is a 2D drawing API after all! > > I think we need a BaseMatrix template that your Matrix and gfxMatrix can > > both derive from, with the API of gfxMatrix. I'm sorry if you don't like > > gfxMatrix, but I recently spent a lot of time unifying our > > Point/Size/Margin/Rect types because the previous set of gfx hackers didn't > > like the existing types, and I don't want to go through that again. So I'm > > leaving Matrix2D aside for now too. > > This actually has a performance impact if the layout of gfxMatrix is not > compatible with Matrix2D (Matrix2D's layout is meant to map well to > OGL/D3D/D2D). I'm not sure if that's true though so provided it's not true I > think I'm fine with this. If the layouts aren't compatible, just change the layout of gfxMatrix to match what you need. (Matt might already have encountered this issue with his work on 3D transforms.) > > @@ +138,5 @@ > > > +{ > > > +public: > > > + virtual ~GradientStops() {} > > > + > > > + virtual BackendType GetBackendType() const = 0; > > > > Wouldn't it be better to store the backend type as a field, so checking it > > doesn't require a virtual call? I'm sure we don't care about the extra space. > > I don't think we care about the virtualization either :). The big advantage > this has in my opinion is that when keeping it as a field mistakes lead to > annoying, hard to find bugs. What sort of bugs? I can't think of why this would be. > I could go either way though, I certainly prefer this style. It's a decision that's easy to change later, so I'll let it slide for now, but at some point I bet we will change it. In any case, in your backend code at least replace "if (blah.Type() == ...) else if (blah.Type() == ...)" etc with a switch so we only have to make one call. > > @@ +163,5 @@ > > > + > > > +class ColorPattern : public Pattern > > > +{ > > > +public: > > > + ColorPattern() {} > > > > Why do we want to allow an empty constructor? > > The reason I did this was so GeneralPattern in the Azure2D context could > have all 4 types as members without forcing it to initialize them when it > wasn't going to use them. I'm willing to remove this but it does cause a > little bit of extra initialization work. Shouldn't GeneralPattern be some kind of union or something using placement new? I can't actually find that in your patch. > > @@ +275,5 @@ > > > + virtual ~SourceSurface() {} > > > + > > > + virtual SurfaceType Type() const = 0; > > > + virtual IntSize GetSize() const = 0; > > > + virtual SurfaceFormat GetFormat() const = 0; > > > > I suspect these should all be fields with inline accessors. > > No, I don't think so, for example in D2D(and also in D3D) we want to get > them directly off the texture description in my opinion. Not store > everything twice. OK. > > @@ +293,5 @@ > > > + /* > > > + * Stride of the surface, distance in bytes between the start of the image > > > + * data belonging to row y and row y+1. > > > + */ > > > + virtual uint32_t Stride() = 0; > > > > Fields, I think. > > Hmm? Never mind, I wanted this in field with an inline Stride() getter, but it looks like we aren't going that way. > > @@ +346,5 @@ > > > + > > > + /* This returns a PathBuilder object that contains a copy of the contents of > > > + * this path and is still writable. > > > + */ > > > + virtual RefPtr<PathBuilder> GetPathBuilder() const = 0; > > > > Call this CopyPath instead? Assuming it always creates a new object, it > > certainly shouldn't be Get. > > Copy is not a very accurate description though, because the result is not a > path but a path builder. Maybe CopyToPathBuilder? or something like that? CopyToBuilder works. > > @@ +356,5 @@ > > > +class PathBuilder : public PathSink > > > +{ > > > +public: > > > + /* Finish writing to the path and return a Path object that can be used for > > > + * drawing > > > > Note that future writes to the PathSink will be ignored and future calls to > > Finish() will return null (or if you prefer, return the same path). > > They actually won't be ignored, they'll cause an exception :). Er, what sort of exception? A crash? > > @@ +365,5 @@ > > > +struct Glyph > > > +{ > > > + uint16_t mIndex; > > > + Point2D mRelativePosition; > > > + Float mAdvance; > > > > Why do you need the advance as well as the relative position here? For this > > simple glyph buffer, why not just have an mPosition for each glyph? > > This is how text run stores them and gives them to us. I think this is good > because it's the representation every API I've found except cairo uses :). > And I'm sure these APIs have good reasons to want the info that way. In any > case converting would be needless work in the D2D case. Let's see ... cairo uses absolute positions and no advances, Quartz uses relative positions from one glyph to the next and no advances, only D2D uses relative positions from a nominal origin as well as advances --- which is weird because it has redundancy. I want to keep the simple GlyphBuffer as simple as possible, and I think it's fine to keep that interface around for whoever wants to use it. Later we'll introduce some kind of PackedGlyphBuffer that is a textrun's internal storage. > > @@ +417,5 @@ > > > + * > > > + * aSurface Source surface to draw > > > + * aDest Destination rectangle that this drawing operation should draw to > > > + * aSource Source rectangle in aSurface coordinates, this area of aSurface > > > + * will be stretched to the size of aDest. > > > > Do we guarantee not to sample outside of aSource? I very very much hope the > > answer is yes! > > Yes, because aSource needs to lie on the source surface, if it doesn't, we > go into an error state. What do you mean exactly "lie on the source surface"? To be clear, what we need is that if the source surface is a black rectangle containing a white square, and aSource is exactly the coordinates of that white square, then whatever transform or filter is used the results will always be fully white, no full or partial black. As you know, what I really want from DrawSurface is for aSource to be allowed to extend outside the source surface and be interpreted in tiled image space ... but that can wait. > > @@ +430,5 @@ > > > + > > > + /* > > > + * Blend a surface to the draw target with a shadow. The shadow is drawn as a > > > + * gaussian blur using a specified sigma. > > > + * NOTE: This function works in device space! > > > > Why? Can we not make it work in user space to be consistent? > > There's a lot of reasons why it's very hard, we'd need different, more > complex shaders, and in some cases either need an extra intermediate surface > or some very complex filter kernel configuration. In any case we never want > this to be in user space for the current Canvas implementation. The > intermediate surface should have been drawn to in user space, so its > contents will be in device space. Can you just use the intermediate surface for all cases where the transform is not identity (or not an integer transform)? I think that would be fine. > > @@ +475,5 @@ > > > + * aEnd End point of the line > > > + * aPattern Pattern that forms the source of this stroking operation > > > + * aOptions Options that are applied to this operation > > > + */ > > > + virtual void StrokeLine(const Point2D &aStart, > > > > Who uses StrokeLine? I don't know of any use for it. > > Some canvas operations are defined as lines in certain degeneracies (0 > height/width fillRects for example). They are? The spec says "The fillRect(x, y, w, h) method must paint the specified rectangular area using the fillStyle. If either height or width are zero, this method has no effect." If we don't have a use for StrokeLine, let's remove it for now. > I think that's a bad idea, almost all API's work with an API that lets you > pass a baseLine, this probably allows for some optimizations inside those > backends that aren't possible otherwise too. Have we found this to be true for any cairo backend? Not as far as I know. > > @@ +586,5 @@ > > > +#ifdef USE_CAIRO > > > + static RefPtr<DrawTarget> CreateDrawTargetForCairoSurface(cairo_surface_t* aSurface); > > > +#endif > > > + > > > + static RefPtr<DrawTarget> CreateDrawTarget(BackendType aBackend, const IntSize &aSize, SurfaceFormat aFormat); > > > > I'm not sure how this would work in general. What does it mean to call this > > with aBackend == BACKEND_CAIRO? Would we call > > gfxPlatform::CreateOffscreenSurface or something like that? > > > > Who actually needs this? > > Anyone who doesn't want to create a texture, but rather leave that all to > the DrawTarget type to decide. A common usecase, just not in our very > specific current canvas usage. I see no problem with having it though, some > of my local testing code uses it for example just because it's less of a > hassle (don't need to create textures, etc.) I can't think of a place we'd want to use this in Gecko. If you want a platform-specific helper for test code, feel free to add that helper to Factory. But on Linux say, you'd have to connect to the default X server or something; I don't think we can generally create an optimal DrawTarget out of thin air. > > @@ +599,5 @@ > > > + static ID3D10Device1 *mD3D10Device; > > > +#endif > > > +}; > > > + > > > +extern Factory sFactory; > > > > What's this for? I don't think it should be exposed. > > It's the class everything outside gfx/2d uses for integration right now. > Although it probably shouldn't be done in this particular way going forward, > but that's another issue. Matt Woodrow had a decent idea on this I believe. The usual way to expose this would be via a static method of Factory. > > @@ +82,5 @@ > > > +{ > > > + DWRITE_FONT_FACE = 0, > > > +}; > > > + > > > +enum CompositionOp { OP_OVER = 0, OP_ADD, OP_ATOP, OP_OUT, OP_IN, OP_SOURCE, OP_CLEAR, OP_DEST_IN, OP_DEST_OUT, OP_DEST_OVER, OP_DEST_ATOP, OP_XOR, MAX_COMPOSITIONOP }; > > > > OP_MAX ... and shouldn't OP_MAX = OP_XOR? > > This is just as easy ( <= or < doesn't matter), allows correct array > initialization without a +1 (in theory, we don't use that). And it won't > break if someone adds an OP at the end without changing the =. Naming wise I > agree of course. It's also incorrect since MAX should be, well, the maximum, not one more than the maximum. If you want to keep this meaning, call it OP_COUNT instead. (Not to mention that OP_MAX could conceivably be a useful composition operator!)

(In reply to comment #9) > (In reply to comment #7) > > > RefPtr and RefCountable should be in mfbt (see bug 661973) (with changes; > > > e.g. RefCountable should probably be RefCounted<T> for greater efficiency > > > and naming compatibility with Webkit), and Azure should depend on mfbt. So I > > > won't review your implementations here. > > > > Right now Azure does not depend on our build system. This has major > > advantages, first of all it can easily be contributed to and used by third > > parties. Which given the interest people have shown is a great thing, > > additionally it means it can be rebuilt very quickly and in a better > > environment on windows when developing, having greatly reduced the time I > > spent building it. If we cannot maintain these advantages my suggestion > > would be to do some #ifdef magic that makes them use the mfbt ones in a > > mozilla build and the current ones without. I'm also not sure RefCounted<T> > > is better than RefCountable, but I might be convinced of that. One of the > > issues is you don't want AddRef and Release to ever be inlined. > > Azure isn't going have its own RefPtr/RefCounted. I insist very firmly on > that. > > Can't you pull the mfbt files (once they're created!) into the Azure > stand-alone build? We're only talking about a few header files. Sure, we could check them in there and use them, no objection to that. When will they be created? > > I'm not sure why you want AddRef/Release to *never* be inlined. Surely there > could be hot code paths where that would be beneficial. I believe in those codepaths we should be using weak references. The problem is you never want a release to be executed from outside the component where allocation was executed. Since at least in a default windows static CRT build this would need to allocation and freeing happening on different heaps, and -very- nasty memory corruption bugs occurring which are hard to track down and easy to miss. In a libxul build this problem shouldn't occur quickly, but in the future it might. Additional reasons are that you would have to ensure 'delete' wherever it is inlined would resolve to the same 'delete' as the component where it was allocated, or again it could resolve to different implementations and the delete could occur on the wrong heap. I've actually already seen this problem in cairo once somewhere where in one compile unit free() would resolve to our mozalloc free, and in another to the normal free(). Inlining release is a bad idea, at least on windows, I don't know enough about other platforms to speak for them. > > > > I think we should probably go with Point/IntPoint/Matrix without the 2D > > > suffix. The rest of our code assumes 2D and adds a "3D" suffix for the 3D > > > objects, and we may as well follow that. > > > > I don't think that's a good idea, let's be explicit. It's weird to add a > > suffix in one case and not in the other. > > It's what we already do, and we aren't going to rename nsPoint to nsPoint2D, > probably ever. I don't find it weird; browsers use 2D geometry a lot, 3D > much less so, so the default should be the 2D version. Azure is a 2D drawing > API after all! I think right now the inconsistency is a nuisance already, I'm not saying we should rename the existing once since I can understand that being a pain. But I'd prefer to do the new ones right. It is a 2D drawing API, but it'll internally deal with a lot of 3D stuff for several backends. Additionally 3D becomes more important going forward, just look at stuff like phone interfaces and such, I don't think 2D is implicit for a browser. > > > I think we need a BaseMatrix template that your Matrix and gfxMatrix can > > > both derive from, with the API of gfxMatrix. I'm sorry if you don't like > > > gfxMatrix, but I recently spent a lot of time unifying our > > > Point/Size/Margin/Rect types because the previous set of gfx hackers didn't > > > like the existing types, and I don't want to go through that again. So I'm > > > leaving Matrix2D aside for now too. > > > > This actually has a performance impact if the layout of gfxMatrix is not > > compatible with Matrix2D (Matrix2D's layout is meant to map well to > > OGL/D3D/D2D). I'm not sure if that's true though so provided it's not true I > > think I'm fine with this. > > If the layouts aren't compatible, just change the layout of gfxMatrix to > match what you need. (Matt might already have encountered this issue with > his work on 3D transforms.) Sounds fine. > > I don't think we care about the virtualization either :). The big advantage > > this has in my opinion is that when keeping it as a field mistakes lead to > > annoying, hard to find bugs. > > What sort of bugs? I can't think of why this would be. Creating a new class and not putting the type in the right place, memory corruption leading to the type actually being changed. I'm also not even sure if reading the value at runtime from memory is faster than getting a constant value through a virtual function call. > > > I could go either way though, I certainly prefer this style. > > It's a decision that's easy to change later, so I'll let it slide for now, > but at some point I bet we will change it. In any case, in your backend code > at least replace "if (blah.Type() == ...) else if (blah.Type() == ...)" etc > with a switch so we only have to make one call. Ok, sure :) I dislike the looks of switch statements and the compiler should > > The reason I did this was so GeneralPattern in the Azure2D context could > > have all 4 types as members without forcing it to initialize them when it > > wasn't going to use them. I'm willing to remove this but it does cause a > > little bit of extra initialization work. > > Shouldn't GeneralPattern be some kind of union or something using placement > new? I can't actually find that in your patch. It's not in my patch, it's inside the nsCanvasRenderingContext2DAzure. It could be made a union, fwiw. But you'd still be stuck with needing default constructors, and for best perf do it on the stack with no initialization. (See http://hg.mozilla.org/projects/graphics/file/d133cfd2bb24/content/canvas/src/nsCanvasRenderingContext2DAzure.cpp#l776) > > > @@ +346,5 @@ > > > > + > > > > + /* This returns a PathBuilder object that contains a copy of the contents of > > > > + * this path and is still writable. > > > > + */ > > > > + virtual RefPtr<PathBuilder> GetPathBuilder() const = 0; > > > > > > Call this CopyPath instead? Assuming it always creates a new object, it > > > certainly shouldn't be Get. > > > > Copy is not a very accurate description though, because the result is not a > > path but a path builder. Maybe CopyToPathBuilder? or something like that? > > CopyToBuilder works. Sounds good. > > > Note that future writes to the PathSink will be ignored and future calls to > > > Finish() will return null (or if you prefer, return the same path). > > > > They actually won't be ignored, they'll cause an exception :). > > Er, what sort of exception? A crash? Null pointer dereference. Since I clear the D2D sink. > > > > @@ +365,5 @@ > > > > +struct Glyph > > > > +{ > > > > + uint16_t mIndex; > > > > + Point2D mRelativePosition; > > > > + Float mAdvance; > > > > > > Why do you need the advance as well as the relative position here? For this > > > simple glyph buffer, why not just have an mPosition for each glyph? > > > > This is how text run stores them and gives them to us. I think this is good > > because it's the representation every API I've found except cairo uses :). > > And I'm sure these APIs have good reasons to want the info that way. In any > > case converting would be needless work in the D2D case. > > Let's see ... cairo uses absolute positions and no advances, Quartz uses > relative positions from one glyph to the next and no advances, only D2D uses > relative positions from a nominal origin as well as advances --- which is > weird because it has redundancy. I want to keep the simple GlyphBuffer as > simple as possible, and I think it's fine to keep that interface around for > whoever wants to use it. Later we'll introduce some kind of > PackedGlyphBuffer that is a textrun's internal storage. I'd like to check with the font guys if there's any reason to do one or the other. Otherwise I'm open to changing this although it'll have a small perf impact from requiring some additional arithmetic but I don't think that'll matter. > > > > @@ +417,5 @@ > > > > + * > > > > + * aSurface Source surface to draw > > > > + * aDest Destination rectangle that this drawing operation should draw to > > > > + * aSource Source rectangle in aSurface coordinates, this area of aSurface > > > > + * will be stretched to the size of aDest. > > > > > > Do we guarantee not to sample outside of aSource? I very very much hope the > > > answer is yes! > > > > Yes, because aSource needs to lie on the source surface, if it doesn't, we > > go into an error state. > > What do you mean exactly "lie on the source surface"? The aSource rect is required to be bound by the rect of the source surface. > > To be clear, what we need is that if the source surface is a black rectangle > containing a white square, and aSource is exactly the coordinates of that > white square, then whatever transform or filter is used the results will > always be fully white, no full or partial black. This will happen. > > > > > > Who uses StrokeLine? I don't know of any use for it. > > > > Some canvas operations are defined as lines in certain degeneracies (0 > > height/width fillRects for example). > > They are? The spec says "The fillRect(x, y, w, h) method must paint the > specified rectangular area using the fillStyle. If either height or width > are zero, this method has no effect." strokeRect, sorry. > > If we don't have a use for StrokeLine, let's remove it for now. So shall we leave that? > > > I think that's a bad idea, almost all API's work with an API that lets you > > pass a baseLine, this probably allows for some optimizations inside those > > backends that aren't possible otherwise too. > > Have we found this to be true for any cairo backend? Not as far as I know. It's how at least the DirectWrite API works, and GDI essentially since it works with a rectangle iirc. I wouldn't be surprised if the temp surface sizes it allocates for text go up in size a lot when not doing that, although it might do work to optimize that. I don't know of any API that uses absolute glyph positions except Cairo. Also it requires even more arithmetic to get things right in the canvas backend when converting from our textrun code to the Azure API. > > Anyone who doesn't want to create a texture, but rather leave that all to > > the DrawTarget type to decide. A common usecase, just not in our very > > specific current canvas usage. I see no problem with having it though, some > > of my local testing code uses it for example just because it's less of a > > hassle (don't need to create textures, etc.) > > I can't think of a place we'd want to use this in Gecko. If you want a > platform-specific helper for test code, feel free to add that helper to > Factory. But on Linux say, you'd have to connect to the default X server or > something; I don't think we can generally create an optimal DrawTarget out > of thin air. I'll think about this. I need to fix up integration code and when I revisit it I can kill it off if we don't need it. > > It's the class everything outside gfx/2d uses for integration right now. > > Although it probably shouldn't be done in this particular way going forward, > > but that's another issue. Matt Woodrow had a decent idea on this I believe. > > The usual way to expose this would be via a static method of Factory. Sure, I don't really care either way :). > > This is just as easy ( <= or < doesn't matter), allows correct array > > initialization without a +1 (in theory, we don't use that). And it won't > > break if someone adds an OP at the end without changing the =. Naming wise I > > agree of course. > > It's also incorrect since MAX should be, well, the maximum, not one more > than the maximum. If you want to keep this meaning, call it OP_COUNT > instead. (Not to mention that OP_MAX could conceivably be a useful > composition operator!) OP_COUNT it is!

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #7) > > Inlining release is a bad idea, at least on windows, I don't know enough > about other platforms to speak for them. I should be more specific! Inlining release for things that have ownership passed across API boundaries is a bad idea.

(In reply to comment #10) > Sure, we could check them in there and use them, no objection to that. When > will they be created? Not sure, but we'll make it happen this week one way or another. > > I'm not sure why you want AddRef/Release to *never* be inlined. Surely there > > could be hot code paths where that would be beneficial. > > I believe in those codepaths we should be using weak references. The problem > is you never want a release to be executed from outside the component where > allocation was executed. Since at least in a default windows static CRT > build this would need to allocation and freeing happening on different > heaps, and -very- nasty memory corruption bugs occurring which are hard to > track down and easy to miss. In a libxul build this problem shouldn't occur > quickly, but in the future it might. So someone could add an option to force non-inline Release() (AddRef doesn't matter) when that becomes needed, but we won't need it for classes declared in libxul as far as I can tell, and in fact we shouldn't use it in libxul. > > > > I think we should probably go with Point/IntPoint/Matrix without the 2D > > > > suffix. The rest of our code assumes 2D and adds a "3D" suffix for the 3D > > > > objects, and we may as well follow that. > > > > > > I don't think that's a good idea, let's be explicit. It's weird to add a > > > suffix in one case and not in the other. > > > > It's what we already do, and we aren't going to rename nsPoint to nsPoint2D, > > probably ever. I don't find it weird; browsers use 2D geometry a lot, 3D > > much less so, so the default should be the 2D version. Azure is a 2D drawing > > API after all! > > I think right now the inconsistency is a nuisance already, I'm not saying we > should rename the existing once since I can understand that being a pain. > But I'd prefer to do the new ones right. It is a 2D drawing API, but it'll > internally deal with a lot of 3D stuff for several backends. Internal issues should not affect our public names. > Additionally 3D > becomes more important going forward, just look at stuff like phone > interfaces and such, I don't think 2D is implicit for a browser. There is a mountain of 2D layout features (which is growing) and just one 3D layout feature --- CSS 3D transforms --- with no others currently planned. And we deliberately try to keep things in 2D as much as possible and leave 3D up to the layer system and a few other places where we work with gfx3DMatrix --- for performance and complexity reasons as well as convenience. I can't forsee that it would ever make sense to rename nsPoint to nsPoint2D etc. So let's not try to be consistent with a future that will probably never happen. It really is important that Azure be consistent with the rest of our code when possible. > > If the layouts aren't compatible, just change the layout of gfxMatrix to > > match what you need. (Matt might already have encountered this issue with > > his work on 3D transforms.) > > Sounds fine. Matt pointed out that gfxMatrix currently needs to be layout-compatible with cairo_matrix_t. So there may be a problem here; let's see. > > > I don't think we care about the virtualization either :). The big advantage > > > this has in my opinion is that when keeping it as a field mistakes lead to > > > annoying, hard to find bugs. > > > > What sort of bugs? I can't think of why this would be. > > Creating a new class and not putting the type in the right place, Make it a parameter to the superclass constructor. > memory corruption leading to the type actually being changed. Come on, memory corruption means you're doomed no matter what. > I'm also not even sure if reading the value at runtime from memory is faster > than getting a constant value through a virtual function call. It is much faster; the virtual function call requires a read of the vtable pointer (basically the same cost as reading the type value), followed by an extra read to get the function pointer, followed by a function call, and if the destination address is mispredicted by the branch target buffer (which it very often will be in a large application), you also get a full pipeline flush. Plus the compiler can't optimize around the virtual call since it doesn't know what the callee will do --- you lose scheduling, CSE involving non-local-variables, etc. There may be register spills as well depending on the calling convention. Virtual calls are amazingly bad. > > > I could go either way though, I certainly prefer this style. > > > > It's a decision that's easy to change later, so I'll let it slide for now, > > but at some point I bet we will change it. In any case, in your backend code > > at least replace "if (blah.Type() == ...) else if (blah.Type() == ...)" etc > > with a switch so we only have to make one call. > > Ok, sure :) I dislike the looks of switch statements and the compiler should Should what? It can't optimize those virtual calls down to a single one since they might have side effects. It has to execute every single one. Sorry if you don't like the look of switch statements, but that's C/C++ for you :-). > > > The reason I did this was so GeneralPattern in the Azure2D context could > > > have all 4 types as members without forcing it to initialize them when it > > > wasn't going to use them. I'm willing to remove this but it does cause a > > > little bit of extra initialization work. > > > > Shouldn't GeneralPattern be some kind of union or something using placement > > new? I can't actually find that in your patch. > > It's not in my patch, it's inside the nsCanvasRenderingContext2DAzure. It > could be made a union, fwiw. But you'd still be stuck with needing default > constructors, and for best perf do it on the stack with no initialization. > (See > http://hg.mozilla.org/projects/graphics/file/d133cfd2bb24/content/canvas/src/ > nsCanvasRenderingContext2DAzure.cpp#l776) OK, I would have GeneralPattern use an internal array-of-char for storage and placement-new the right type of Pattern. That's sure to be more efficient and doesn't require zero-param constructors. > > > > Who uses StrokeLine? I don't know of any use for it. > > > > > > Some canvas operations are defined as lines in certain degeneracies (0 > > > height/width fillRects for example). > > > > They are? The spec says "The fillRect(x, y, w, h) method must paint the > > specified rectangular area using the fillStyle. If either height or width > > are zero, this method has no effect." > > strokeRect, sorry. Aaaah, let's leave StrokeLine in, you're absolutely right. > > > I think that's a bad idea, almost all API's work with an API that lets you > > > pass a baseLine, this probably allows for some optimizations inside those > > > backends that aren't possible otherwise too. > > > > Have we found this to be true for any cairo backend? Not as far as I know. > > It's how at least the DirectWrite API works, and GDI essentially since it > works with a rectangle iirc. I wouldn't be surprised if the temp surface > sizes it allocates for text go up in size a lot when not doing that, > although it might do work to optimize that. I don't know of any API that > uses absolute glyph positions except Cairo. Quartz does not use a baseline when drawing positioned glyphs. > Also it requires even more > arithmetic to get things right in the canvas backend when converting from > our textrun code to the Azure API. I don't think it's more code in nsCanvasBidiProcessorAzure::DrawText, just keep a running "current absolute position" starting at the start of the baseline and you're all set.

(In reply to comment #12) > (In reply to comment #10) > > > I'm not sure why you want AddRef/Release to *never* be inlined. Surely there > > > could be hot code paths where that would be beneficial. > > > > I believe in those codepaths we should be using weak references. The problem > > is you never want a release to be executed from outside the component where > > allocation was executed. Since at least in a default windows static CRT > > build this would need to allocation and freeing happening on different > > heaps, and -very- nasty memory corruption bugs occurring which are hard to > > track down and easy to miss. In a libxul build this problem shouldn't occur > > quickly, but in the future it might. > > So someone could add an option to force non-inline Release() (AddRef doesn't > matter) when that becomes needed, but we won't need it for classes declared > in libxul as far as I can tell, and in fact we shouldn't use it in libxul. If we can guarantee every compile unit in libxul resolves to the same 'new' and 'delete' call :). Which wasn't true for inside cairo at least, but that might've just been a bug. > There is a mountain of 2D layout features (which is growing) and just one 3D > layout feature --- CSS 3D transforms --- with no others currently planned. > And we deliberately try to keep things in 2D as much as possible and leave > 3D up to the layer system and a few other places where we work with > gfx3DMatrix --- for performance and complexity reasons as well as > convenience. I can't forsee that it would ever make sense to rename nsPoint > to nsPoint2D etc. So let's not try to be consistent with a future that will > probably never happen. It really is important that Azure be consistent with > the rest of our code when possible. Meh, I can live with this, I think it's annoying, but hey :). > > > If the layouts aren't compatible, just change the layout of gfxMatrix to > > > match what you need. (Matt might already have encountered this issue with > > > his work on 3D transforms.) > > > > Sounds fine. > > Matt pointed out that gfxMatrix currently needs to be layout-compatible with > cairo_matrix_t. So there may be a problem here; let's see. I think they might be layout compatible anyway, but indeed, let's see. > > memory corruption leading to the type actually being changed. > > Come on, memory corruption means you're doomed no matter what. Right, but in one case the problem is easier to diagnose than the other. > > > I'm also not even sure if reading the value at runtime from memory is faster > > than getting a constant value through a virtual function call. > > It is much faster; the virtual function call requires a read of the vtable > pointer (basically the same cost as reading the type value), followed by an > extra read to get the function pointer, followed by a function call, and if > the destination address is mispredicted by the branch target buffer (which > it very often will be in a large application), you also get a full pipeline > flush. Plus the compiler can't optimize around the virtual call since it > doesn't know what the callee will do --- you lose scheduling, CSE involving > non-local-variables, etc. There may be register spills as well depending on > the calling convention. Virtual calls are amazingly bad. You're assuming here it's just Type and no other call to a virtual function. In many cases where type is used other virtual function calls will be made as well. Meaning the vtable has already been read and the contents of the vtable is in the cache (obviously the value of 'type' is likely to be in the cache then as well, to be fair). Additionally the register situation becomes a bit different. I'll believe the virtual function call is a little slower. But I don't think it's a relevant difference. I seem to recall a research paper that suggested changing every single function in a large piece of software to a virtual function only costs you about ~10% perf. > > Ok, sure :) I dislike the looks of switch statements and the compiler should > > Should what? It can't optimize those virtual calls down to a single one > since they might have side effects. It has to execute every single one. Type() is a member of blah and is marked 'const'. So it should be allowed to optimize that. But then again, I guess the function could return and touch some global static, so that screws up that assumption there. LTCG will probably fix that though. But yeah, it doesn't matter too much. Let's do switch :). > > > > > The reason I did this was so GeneralPattern in the Azure2D context could > > > > have all 4 types as members without forcing it to initialize them when it > > > > wasn't going to use them. I'm willing to remove this but it does cause a > > > > little bit of extra initialization work. > > > > > > Shouldn't GeneralPattern be some kind of union or something using placement > > > new? I can't actually find that in your patch. > > > > It's not in my patch, it's inside the nsCanvasRenderingContext2DAzure. It > > could be made a union, fwiw. But you'd still be stuck with needing default > > constructors, and for best perf do it on the stack with no initialization. > > (See > > http://hg.mozilla.org/projects/graphics/file/d133cfd2bb24/content/canvas/src/ > > nsCanvasRenderingContext2DAzure.cpp#l776) > > OK, I would have GeneralPattern use an internal array-of-char for storage > and placement-new the right type of Pattern. That's sure to be more > efficient and doesn't require zero-param constructors. I think that's a little more obscure. If the size of patterns change it also means you'll get nasty write past bounds if you forget to enlarge the char array :) But if you prefer that I'm willing to do that? > > It's how at least the DirectWrite API works, and GDI essentially since it > > works with a rectangle iirc. I wouldn't be surprised if the temp surface > > sizes it allocates for text go up in size a lot when not doing that, > > although it might do work to optimize that. I don't know of any API that > > uses absolute glyph positions except Cairo. > > Quartz does not use a baseline when drawing positioned glyphs. It uses relative to eachother though, right? > > > Also it requires even more > > arithmetic to get things right in the canvas backend when converting from > > our textrun code to the Azure API. > > I don't think it's more code in nsCanvasBidiProcessorAzure::DrawText, just > keep a running "current absolute position" starting at the start of the > baseline and you're all set. What do you mean? Rather than just moving the values I'll have to do. glyph.positionX = advanceSum + textrun.glyph.offsetX; glyph.positionY = baseline.Y + textrun.glyph.offsetY; right? i.e. I get two extra additions per glyph. Currently I can do. glyph.advance = textrun.glyph.advance; etc. i.e. I just move values around.

(In reply to comment #12) > (In reply to comment #10) > It is much faster; the virtual function call requires a read of the vtable > pointer (basically the same cost as reading the type value), followed by an > extra read to get the function pointer, followed by a function call, and if > the destination address is mispredicted by the branch target buffer (which > it very often will be in a large application), you also get a full pipeline > flush. Plus the compiler can't optimize around the virtual call since it > doesn't know what the callee will do --- you lose scheduling, CSE involving > non-local-variables, etc. There may be register spills as well depending on > the calling convention. Virtual calls are amazingly bad. So I decided to do some testing here, it was pretty hard to get usable results but after creating some code structures it seems I got the assembly to look like what I expected it to after optimization. This wasn't in a complex program obviously so there was a prediction advantage. The answer is it kind of depends on how often you call Type() during something's lifetime. The constructor was 6 cycles more expensive for the field approach. The getter was on average 2 cycles cheaper than the virtual function call.

(In reply to comment #13) > If we can guarantee every compile unit in libxul resolves to the same 'new' > and 'delete' call :). Which wasn't true for inside cairo at least, but that > might've just been a bug. It might not have been true when we could build non-libxul, but I'm confident it's true now. > > > memory corruption leading to the type actually being changed. > > > > Come on, memory corruption means you're doomed no matter what. > > Right, but in one case the problem is easier to diagnose than the other. Actually I think a bogus type tag (where the corrupt value is probably obviously invalid) would be much easier to diagnose than a corrupted integer value (which is quite likely to appear valid). > You're assuming here it's just Type and no other call to a virtual function. > In many cases where type is used other virtual function calls will be made > as well. Meaning the vtable has already been read and the contents of the > vtable is in the cache (obviously the value of 'type' is likely to be in the > cache then as well, to be fair). I didn't mention cache effects, they're not really relevant. > Additionally the register situation becomes > a bit different. I'll believe the virtual function call is a little slower. > But I don't think it's a relevant difference. I seem to recall a research > paper that suggested changing every single function in a large piece of > software to a virtual function only costs you about ~10% perf. I don't believe that would be true for us. Back in 2000 or 2001 Dave Hyatt changed some key functions in nsIFrame (GetRect etc) from virtual to non-virtual and Tp got 10% faster. It depends on call patterns, the code locality of the application, and what optimizations the compiler might be doing that have been inhibited by virtual calls. It's tricky because the effects of BTB pressure are complex; if you reduce virtual calls in one area, virtual calls in other parts of the code get faster because they're more likely to have BTB hits (and the converse is true too if you add virtual calls!). Making significant design changes to minimize virtual calls would be premature optimization --- but this is a tiny change. However, I did agree above to let you continue with the virtual Type() calls for now, so we don't need to over-discuss this. > > > Ok, sure :) I dislike the looks of switch statements and the compiler should > > > > Should what? It can't optimize those virtual calls down to a single one > > since they might have side effects. It has to execute every single one. > > Type() is a member of blah and is marked 'const'. So it should be allowed to > optimize that. Not at all. "const" doesn't mean there are no side effects. > But then again, I guess the function could return and touch > some global static, so that screws up that assumption there. Yeah :-) > LTCG will probably fix that though. Unlikely, since C++ compilers usually cannot prove that there are no extra subclasses of the base class present at run-time, so it's always possible there is an override of Type() they haven't seen that could do bad things. > I think that's a little more obscure. If the size of patterns change it also > means you'll get nasty write past bounds if you forget to enlarge the char > array :) But if you prefer that I'm willing to do that? You can avoid that problem: union { AlignedStorage2<ColorPattern> mColor; AlignedStorage2<SurfacePattern> mSurface; AlignedStorage2<GradientPattern> mGradient; } mPattern; > > Quartz does not use a baseline when drawing positioned glyphs. > > It uses relative to eachother though, right? Yeah, it's a series of offsets from one glyph origin to the next. > > > Also it requires even more > > > arithmetic to get things right in the canvas backend when converting from > > > our textrun code to the Azure API. > > > > I don't think it's more code in nsCanvasBidiProcessorAzure::DrawText, just > > keep a running "current absolute position" starting at the start of the > > baseline and you're all set. > > What do you mean? Rather than just moving the values I'll have to do. > > glyph.positionX = advanceSum + textrun.glyph.offsetX; > glyph.positionY = baseline.Y + textrun.glyph.offsetY; > > right? i.e. I get two extra additions per glyph. Currently I can do. > > glyph.advance = textrun.glyph.advance; etc. i.e. I just move values around. OK. It still doesn't matter.

(In reply to comment #15) > (In reply to comment #13) > You can avoid that problem: > union { > AlignedStorage2<ColorPattern> mColor; > AlignedStorage2<SurfacePattern> mSurface; > AlignedStorage2<GradientPattern> mGradient; > } mPattern; Good idea! Done.

I don't really like this kind of stuff: if (!mDSPathBuilder) { mPathBuilder->MoveTo(Point2D(x, y)); } else { mDSPathBuilder->MoveTo(mTarget->GetTransform() * Point2D(x, y)); } First, it took me a long time to figure out that DS stood for device space. Second, it's not clear to me what the motivation for having both a mPathBuilder and a mDSPathBuidler is.

In order to facilitate solid integration of Azure we need to be able to get an optimal DrawTarget from the layer manager.

Comment on attachment 540328 [details] [diff] [review] Add API for creating an optimal DrawTarget. Review of attachment 540328 [details] [diff] [review]: ----------------------------------------------------------------- Let's call it CreateDrawTarget. No reason to create a suboptimal one here :-). I think Layers.h should be including "mozilla/RefPtr.h", then you don't need mozilla::gfx:: prefix on RefPtr. Also you should be able to write gfxIntSize for mozilla::gfx::IntSize, since the former should be typedefed to the latter.

Comment on attachment 540329 [details] [diff] [review] Implement creating an optimal DrawTarget for D3D10 Review of attachment 540329 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/d3d10/LayerManagerD3D10.cpp @@ +441,5 @@ > + nsRefPtr<ID3D10Texture2D> texture; > + > + CD3D10_TEXTURE2D_DESC desc(DXGI_FORMAT_B8G8R8A8_UNORM, aSize.width, aSize.height, 1, 1); > + desc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE; > + desc.MiscFlags = D3D10_RESOURCE_MISC_GDI_COMPATIBLE; Why do you want GDI compatibility?

Comment on attachment 538051 [details] [diff] [review] Azure API and D2D backend Review of attachment 538051 [details] [diff] [review]: ----------------------------------------------------------------- Just making clear a new patch is needed here.

I realized that something's bugging me here about the API. We use DrawTargets in two rather different ways: 1) create a DrawTarget passing in a native object to draw into, draw, call Flush and then use the native object 2) create a DrawTarget without passing in anything specific to draw into, call Snapshot to get a SourceSurface you can use I'm not 100% sure we can always implement Snapshot correctly for DrawTargets created by method #1. For example, on Mac you can't read back from a CGContext that's drawing directly into a window. And calling Flush() on DrawTargets created by method #2 is meaningless. This is a bit ugly. The original design had DrawTarget::CreateIntermediateTarget return an object which exposed both a DrawTarget for the intermediate surface and the Snapshot method to get a SourceSurface. So not all DrawTargets supported Snapshot, only ones that specifically represented intermediate surfaces. I don't feel super-strongly that we should change this though. Adding a new kind of object just to expose a single optionally-supported method may be overkill. But we probably want to document that Snapshot can fail, and document some of the DrawTarget factory methods to indicate that the returned DrawTargets are guaranteed to support Snapshot.

Which brings me to the question I should have asked before: what is LayerManager::CreateDrawTarget actually going to be used for? You don't seem to need it for canvas since you're currently using the gfxASurface conversion API. I think it would be better style to create DrawTargets that draw into a specific destination whenever that's possible.

(In reply to comment #24) > Which brings me to the question I should have asked before: what is > LayerManager::CreateDrawTarget actually going to be used for? You don't seem > to need it for canvas since you're currently using the gfxASurface > conversion API. I think it would be better style to create DrawTargets that > draw into a specific destination whenever that's possible. I'm actually about to push code that does not use the conversion API's. In the general case it actually still uses CreateForD3D10Texture. But if that fails it will use the other API to create a 1x1 D2D draw target. (much like the current canvas will create a 1x1 ImageSurface in that case) So we started using this. :) (In reply to comment #21) > Comment on attachment 540329 [details] [diff] [review] [review] > Implement creating an optimal DrawTarget for D3D10 > > Review of attachment 540329 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/d3d10/LayerManagerD3D10.cpp > @@ +441,5 @@ > > + nsRefPtr<ID3D10Texture2D> texture; > > + > > + CD3D10_TEXTURE2D_DESC desc(DXGI_FORMAT_B8G8R8A8_UNORM, aSize.width, aSize.height, 1, 1); > > + desc.BindFlags = D3D10_BIND_RENDER_TARGET | D3D10_BIND_SHADER_RESOURCE; > > + desc.MiscFlags = D3D10_RESOURCE_MISC_GDI_COMPATIBLE; > > Why do you want GDI compatibility? Not really thanks for catching that. (In reply to comment #23) > > I don't feel super-strongly that we should change this though. Adding a new > kind of object just to expose a single optionally-supported method may be > overkill. But we probably want to document that Snapshot can fail, and > document some of the DrawTarget factory methods to indicate that the > returned DrawTargets are guaranteed to support Snapshot. I think that's a good idea.

(In reply to comment #25) > (In reply to comment #24) > > Which brings me to the question I should have asked before: what is > > LayerManager::CreateDrawTarget actually going to be used for? You don't seem > > to need it for canvas since you're currently using the gfxASurface > > conversion API. I think it would be better style to create DrawTargets that > > draw into a specific destination whenever that's possible. > > I'm actually about to push code that does not use the conversion API's. In > the general case it actually still uses CreateForD3D10Texture. But if that > fails it will use the other API to create a 1x1 D2D draw target. (much like > the current canvas will create a 1x1 ImageSurface in that case) I'd like to see that other code and understand when and why you want to create a 1x1 D2D draw target.

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > Which brings me to the question I should have asked before: what is > > > LayerManager::CreateDrawTarget actually going to be used for? You don't seem > > > to need it for canvas since you're currently using the gfxASurface > > > conversion API. I think it would be better style to create DrawTargets that > > > draw into a specific destination whenever that's possible. > > > > I'm actually about to push code that does not use the conversion API's. In > > the general case it actually still uses CreateForD3D10Texture. But if that > > fails it will use the other API to create a 1x1 D2D draw target. (much like > > the current canvas will create a 1x1 ImageSurface in that case) > > I'd like to see that other code and understand when and why you want to > create a 1x1 D2D draw target. Yeah, I'm not sure whether there's a point to it but since the old canvas code did it I figured I should do the same. It creates a valid surface but marks mValid false. I think in the Azure case we can actually do without it but am not 100% certain. I'll push the code in a couple of minutes.

It seems to me the new code calls the layer manager CreateOptimalDrawTarget in all situations. I think I'd be fine with this if we called the new method CreateCanvasDrawTarget. You'll need to document that it creates a snapshottable DrawTarget.

By the way, I'd like to review nsCanvasRenderingContext2DAzure.

(In reply to comment #29) > By the way, I'd like to review nsCanvasRenderingContext2DAzure. Please do! I'd like to land it in 2 days! Could you review it on the project branch? Generating a patch is a bit of a nuisance and I'd like to go through that just once when we're pretty confident that it's good for r+?

I'd find it easier to review using Splinter. Do you want the review in this bug or a new bug?

(In reply to comment #31) > I'd find it easier to review using Splinter. Do you want the review in this > bug or a new bug? This bug is fine.

Please attach the patch, then. It's much easier to review that way, for me.

Or I'll generate it myself if you haven't by tomorrow :-)

(In reply to comment #34) > Or I'll generate it myself if you haven't by tomorrow :-) My tree is in a bad state right now (resolving conflicts between mozilla::RefCounted and the CCNodeType enumeration and such things), as I'm trying to get mozilla::RefPtr and mozilla::RefCounted to work for Azure. As soon as I get that all figured out (no guarantee as to the timeframe :() I'll try and generate the patch, of course if you could do it that would be fantastic.

I just realized at the moment mozTextToPath, mozDrawText and mozTextAlongPath aren't implemented in the new Azure canvas. Do we think they're important? Robarnold believed noone was using them (he created them) and said they predated the standard by a year. (none of our test seem to cover them) Possibly we could just deprecate them. I could also implement them, it isn't too hard, but possibly removing them is actually the easier solution, even for longer term.

Let's try dropping them. They're already deprecated on MDN. We'll have a shake-down period for Azure anyway, so this can be part of that.

Roc, do you know a good way to generate a cross-repository diff? Generating this patch right on the project branch is hard.

Can't you just diff tip against changeset 2c1c975dd6db, the most recent changeset from mozilla-central?

Hey, it looks like that actually worked, I didn't think it would since I'd merged in m-c.

Updated the patch, this doesn't include change to Point2D and update to use mozilla::refptr as I'm still waiting on feedback/review on some refptr changes.

Comment on attachment 540665 [details] [diff] [review] Add Azure Canvas and adjust tests for it Review of attachment 540665 [details] [diff] [review]: ----------------------------------------------------------------- More to come... ::: content/canvas/public/nsICanvasRenderingContextInternal.h @@ +102,5 @@ > NS_IMETHOD GetThebesSurface(gfxASurface **surface) = 0; > + > + // This gets an Azure SourceSurface for the canvas, this will be a snapshot > + // of the canvas at the time it was called. > + virtual mozilla::gfx::RefPtr<mozilla::gfx::SourceSurface> GetSurfaceSnapshot() = 0; So this returns null for non-Azure canvases? Please document. I assume RefPtr will be in the mozilla:: namespace? But shouldn't this be TemporaryRef? ::: content/canvas/src/WebGLContext.h @@ +342,5 @@ > const PRUnichar* aEncoderOptions, > nsIInputStream **aStream); > NS_IMETHOD GetThebesSurface(gfxASurface **surface); > + mozilla::gfx::RefPtr<mozilla::gfx::SourceSurface> GetSurfaceSnapshot() > + { return NULL; } nsnull instead of NULL, throughout! ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +128,5 @@ > + > +// windows.h (included by chromium code) defines this, in its infinite wisdom > +#undef DrawText > + > +using namespace mozilla::ipc; Put this down after the others. @@ +190,5 @@ > + GetCanvasAzureMemoryUsed, > + NULL) > + > +static void > +CopyContext(gfxContext* dest, gfxContext* src) Unused, remove. @@ +253,5 @@ > + mOrigin = ((mCenter * aBeginRadius) - (aBeginOrigin * mRadius)) / > + (aBeginRadius - mRadius); > + } > + > + RefPtr<GradientStops> GetGradientStopsForTarget(DrawTarget *aRT) I suspect we should be returning TemporaryRef here. @@ +285,5 @@ > + > + if (!mRawStops.Length() && mOffsetStart > 0) { > + newStop.offset = mOffsetStart; > + newStop.color = Color::FromABGR(color); > + mRawStops.AppendElement(newStop); Why do we need to append this extra stop? @@ +308,5 @@ > + Point2D mBegin; > + Point2D mEnd; > + Point2D mCenter; > + Float mRadius; > + Point2D mOrigin; These need to be documented. What exactly is mOrigin? @@ +311,5 @@ > + Float mRadius; > + Point2D mOrigin; > + > + Float mOffsetStart; > + Float mOffsetRatio; These need to be documented too. Can we not have separate subclasses for linear vs radial? @@ +450,5 @@ > + NS_IMETHOD GetInputStream(const char* aMimeType, > + const PRUnichar* aEncoderOptions, > + nsIInputStream **aStream); > + NS_IMETHOD GetThebesSurface(gfxASurface **surface); > + mozilla::gfx::RefPtr<mozilla::gfx::SourceSurface> GetSurfaceSnapshot() Drop mozilla::gfx:: @@ +461,5 @@ > + LayerManager *aManager); > + void MarkContextClean(); > + NS_IMETHOD SetIsIPC(PRBool isIPC); > + // this rect is in canvas device space > + void Redraw(const mozilla::gfx::Rect &r); drop mozilla::gfx:: @@ +465,5 @@ > + void Redraw(const mozilla::gfx::Rect &r); > + NS_IMETHOD Redraw(const gfxRect &r) { Redraw(ToRect(r)); return NS_OK; } > + > + // this rect is in mThebes's current user space > + NS_IMETHOD RedrawUser(const gfxRect &r); Fix mThebes content. Also, this doesn't need to be virtual. @@ +483,5 @@ > + STYLE_MAX > + }; > + > +protected: > + NS_IMETHOD InitializeWithTarget(nsIDocShell *shell, DrawTarget *surface, PRInt32 width, PRInt32 height); why is this virtual? should probably be nonvirtual @@ +502,5 @@ > + */ > + static PRUint8 (*sPremultiplyTable)[256]; > + > + // Some helpers. Doesn't modify acolor on failure. > + nsresult SetStyleFromStringOrInterface(const nsAString& aStr, nsISupports *aInterface, Style aWhichStyle); acolor? fix comment. @@ +507,5 @@ > + nsresult GetStyleAsStringOrInterface(nsAString& aStr, nsISupports **aInterface, PRInt32 *aType, Style aWhichStyle); > + > + void StyleColorToString(const nscolor& aColor, nsAString& aStr); > + > + void DirtyAllStyles(); the mDirtyStyle flags are not used, so this should be removed. @@ +519,5 @@ > + * Creates the premultiply lookup table, if it doesn't exist. > + */ > + void EnsurePremultiplyTable(); > + > + void EnsureWritablePath(); Document what this does. @@ +522,5 @@ > + > + void EnsureWritablePath(); > + > + // Ensures a path in UserSpace is available. > + void EnsureUSPath(); EnsureUserSpacePath @@ +535,5 @@ > + > + // Member vars > + PRInt32 mWidth, mHeight; > + PRPackedBool mValid; > + PRPackedBool mZero; Document mValid and mZero @@ +538,5 @@ > + PRPackedBool mValid; > + PRPackedBool mZero; > + PRPackedBool mOpaque; > + PRPackedBool mResetLayer; > + PRPackedBool mIPC; These two as well @@ +542,5 @@ > + PRPackedBool mIPC; > + > + // the canvas element we're a context of > + nsCOMPtr<nsIDOMHTMLCanvasElement> mCanvasElement; > + nsHTMLCanvasElement *HTMLCanvasElement() { Please keep the data members and the methods separate. @@ +552,5 @@ > + > + // our drawing surfaces, contexts, and layers > + RefPtr<DrawTarget> mTarget; > + > + PRUint32 mSaveCount; Why not just make this a method that returns mStyleStack.Length() - 1? @@ +567,5 @@ > + */ > + PRPackedBool mPredictManyRedrawCalls; > + > + /** > + * This is set whenever there's a nonempty path set by the API user. Really? Don't we lazily set this when the current path is rendered via fill() or stroke()? @@ +575,5 @@ > + /** > + * We also have a device space pathbuilder. The reason for this is as > + * follows, when a path is being built, but the transform changes, we > + * can no longer keep a single path in userspace, considering there's > + * several 'user spaces' now. We therefor transform the current path therefore @@ +592,5 @@ > + RefPtr<Path> mDSPath; > + RefPtr<PathBuilder> mDSPathBuilder; > + RefPtr<PathBuilder> mPathBuilder; > + bool mPathTransformUpdated; > + Matrix2D mPathToDS; You should explain in more detail what these mean, especially the invariants that hold. @@ +605,5 @@ > + * Returns true iff the the given operator should affect areas of the > + * destination where the source is transparent. Among other things, this > + * implies that a fully transparent source would still affect the canvas. > + */ > + PRBool OperatorAffectsUncoveredAreas(gfxContext::GraphicsOperator op) const Not used, remove. @@ +625,5 @@ > + // The spec says we should not draw shadows when the alpha value is 0, > + // regardless of the operator being used. > + return state.op == OP_OVER && state.StyleIsColor(STYLE_SHADOW) && > + NS_GET_A(state.colorStyles[STYLE_SHADOW]) > 0 && > + (state.shadowOffset != Point2D(0, 0) || state.shadowBlur != 0); This is obsolete now. We should draw shadows regardless of the offset/blur. This should have caused a test failure... @@ +632,5 @@ > + /** > + * Draws the current path in the given style. Takes care of > + * any shadow drawing and will use intermediate surfaces as needed. > + * > + * If dirtyRect is given, it will contain the user-space dirty You mean it will be set to the user-space dirty rect by this method? @@ +690,5 @@ > + float x, > + float y, > + float maxWidth, > + TextDrawOperation op, > + float* aWidth); This is all going to have to be fixed up to use parameter names starting with 'a' (and other style issues fixed), but I guess you inherited these from nsCanvasRenderingContext2D.cpp so let it go for now... @@ +698,5 @@ > + * The previous set style. Is equal to STYLE_MAX when there is no valid > + * previous style. > + */ > + Style mLastStyle; > + PRPackedBool mDirtyStyle[STYLE_MAX]; Remove these, you're not using them (and they're confusing!) @@ +760,5 @@ > + > + /** > + * returns true iff the given style is a solid color. > + */ > + inline PRBool StyleIsColor(Style whichStyle) const I wouldn't bother with all the "inline" declarations myself. The compiler will probably do whatever it wants regardless. @@ +768,5 @@ > + } > + > + Point2D shadowOffset; > + float globalAlpha; > + float shadowBlur; Float @@ +777,5 @@ > + TextBaseline textBaseline; > + > + nscolor colorStyles[STYLE_MAX]; > + nsCOMPtr<nsCanvasGradientAzure> gradientStyles[STYLE_MAX]; > + nsCOMPtr<nsCanvasPatternAzure> patternStyles[STYLE_MAX]; nsRefPtrs. But shouldn't we actually have an array of structs here? Or even a struct which is internally a union? Would be faster to copy since it's smaller. And why not just store the shadow color as a separate thing instead of as a peer of fillStyle/strokeStyle? @@ +779,5 @@ > + nscolor colorStyles[STYLE_MAX]; > + nsCOMPtr<nsCanvasGradientAzure> gradientStyles[STYLE_MAX]; > + nsCOMPtr<nsCanvasPatternAzure> patternStyles[STYLE_MAX]; > + > + Matrix2D transform; This "2D" suffix is going away right? :-) @@ +782,5 @@ > + > + Matrix2D transform; > + CompositionOp op; > + > + PRPackedBool imageSmoothingEnabled; We should organize this so it packs nicely. I think that means all the pointers first, followed by the floats, Point and Matrix, followed by enums, followed by the PRPackedBool. @@ +787,5 @@ > + > + Float lineWidth; > + CapStyle lineCap; > + JoinStyle lineJoin; > + float miterLimit; Float @@ +795,5 @@ > + > + class GeneralPattern > + { > + public: > + GeneralPattern() : mPattern(NULL) {} nsnull @@ +811,5 @@ > + // This should only be called once or the mPattern destructor will > + // not be executed. > + NS_ASSERTION(!mPattern, "ForStyle() should only be called once on GeneralPattern!"); > + > + nsCanvasRenderingContext2DAzure::ContextState state = const &! You're making a copy here, that's bad! @@ +828,5 @@ > + mPattern = new (mRadialGradientPattern.addr()) > + RadialGradientPattern(state.gradientStyles[aStyle]->mCenter, > + state.gradientStyles[aStyle]->mOrigin, > + state.gradientStyles[aStyle]->mRadius, > + state.gradientStyles[aStyle]->GetGradientStopsForTarget(aRT)); What's the story about radial gradients with different circle centers? Are you going to add that to Azure after this lands? @@ +840,5 @@ > + ExtendMode mode; > + if (state.patternStyles[aStyle]->mRepeat == nsCanvasPatternAzure::NOREPEAT) { > + mode = EXTEND_CLAMP; > + } else { > + mode = EXTEND_WRAP; When are we going to make repeating work properly? I hope that's done by remove SurfacePattern::mExtendMode and using a tile-space rect instead. @@ +858,5 @@ > + }; > + Pattern *mPattern; > + }; > + > + class AdjustedTarget Document this! @@ +862,5 @@ > + class AdjustedTarget > + { > + public: > + AdjustedTarget(nsCanvasRenderingContext2DAzure *ctx, > + const mozilla::gfx::Rect &aBounds = mozilla::gfx::Rect()) Lose mozilla::gfx::, and document aBounds. If it's an optional parameter, why not make it a pointer so that null can be passed in? But it looks like no-one passes a value here, so I suggest taking it out altogether, especially since it appears the non-default path is not implemented. @@ +863,5 @@ > + { > + public: > + AdjustedTarget(nsCanvasRenderingContext2DAzure *ctx, > + const mozilla::gfx::Rect &aBounds = mozilla::gfx::Rect()) > + : mCtx(NULL) nsnull @@ +889,5 @@ > + mSurfOffset.x = -ctx->CurrentState().shadowOffset.x; > + transform._31 += ctx->CurrentState().shadowOffset.x; > + } else { > + mTempSize.width -= ctx->CurrentState().shadowOffset.x; > + } I think this code might be clearer if you worked with rects, and took the union of the shadow and drawing rects here. That would also extend more cleanly to having more precise bounds passed in. @@ +918,5 @@ > + > + if (!mTarget) { > + // XXX - Deal with the situation where our temp size is too big to > + // fit in a texture. > + mTarget = ctx->mTarget; Should clear mCtx here. @@ +948,5 @@ > + IntSize mTempSize; > + Point2D mSurfOffset; > + }; > + > + nsTArray<ContextState> mStyleStack; This means that every save() and restore() that's not nested is going to do an alloc and free, that's bad. (I see that you construct with an initial capacity, but the first RemoveElementAt will reduce the capacity.) We should either do an nsAutoTArray<ContextState,2> or something like that, or else make Save() and Restore() not shrink the array (e.g. have Save() do EnsureCapacity instead of SetCapacity, and have Restore() just clear the state instead of removing the array element). @@ +1078,5 @@ > + > +nsresult > +nsCanvasRenderingContext2DAzure::SetStyleFromStringOrInterface(const nsAString& aStr, > + nsISupports *aInterface, > + Style aWhichStyle) Fix indent @@ +1135,5 @@ > +nsresult > +nsCanvasRenderingContext2DAzure::GetStyleAsStringOrInterface(nsAString& aStr, > + nsISupports **aInterface, > + PRInt32 *aType, > + Style aWhichStyle) Fix indent @@ +1204,5 @@ > + return NS_OK; > +} > + > +void > +nsCanvasRenderingContext2DAzure::Redraw(const mozilla::gfx::Rect &r) Lose mozilla::gfx:: @@ +1238,5 @@ > + ++mInvalidateCount; > + return NS_OK; > + } > + > + mozilla::gfx::Rect newr = mTarget->GetTransform() * ToRect(r); Lose mozilla::gfx:: I didn't notice you support Matrix * Rect. I think that's not so good since this doesn't just transform, it also expands to the axis-aligned bounding rect. So I'd prefer an explicit TransformBounds like we have elsewhere. @@ +1278,5 @@ > + > + if (layerManager) { > + target = layerManager->CreateDrawTarget(size, format); > + } else { > + target = Factory::CreateDrawTarget(BACKEND_DIRECT2D, size, format); Ick. I'm not sure what to do here, but probably a cairo image surface backend is the safest bet once we support that. @@ +1311,5 @@ > + > + mResetLayer = PR_TRUE; > + > + /* Create dummy surfaces here */ > + if (target == nsnull) if (!target) { Somewhere something needs to document when target could be null @@ +1313,5 @@ > + > + /* Create dummy surfaces here */ > + if (target == nsnull) > + { > + mTarget = Factory::CreateDrawTarget(BACKEND_DIRECT2D, IntSize(1, 1), FORMAT_B8G8R8A8); This seems wrong, we won't want this when Azure has more than one backend. Maybe we would choose a cairo image surface backend or something like that? @@ +1334,5 @@ > + state->colorStyles[STYLE_STROKE] = NS_RGB(0,0,0); > + state->colorStyles[STYLE_SHADOW] = NS_RGBA(0,0,0,0); > + DirtyAllStyles(); > + > + mTarget->FillRect(mozilla::gfx::Rect(Point2D(0, 0), Size(mWidth, mHeight)), lose mozilla::gfx:: @@ +1396,5 @@ > + if (NS_FAILED(GetThebesSurface(getter_AddRefs(surface)))) { > + return NS_ERROR_FAILURE; > + } > + > + mTarget->Flush(); The Flush() should be inside GetThebesSurface() --- if it's needed. @@ +1422,5 @@ > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::GetInputStream(const char *aMimeType, > + const PRUnichar *aEncoderOptions, > + nsIInputStream **aStream) Fix indent. @@ +1428,5 @@ > + if (!mValid || !mTarget) { > + return NS_ERROR_FAILURE; > + } > + > + mTarget->Flush(); See above @@ +1461,5 @@ > + nsRefPtr<gfxImageSurface> imgsurf = > + new gfxImageSurface(imageBuffer.get(), > + gfxIntSize(mWidth, mHeight), > + mWidth * 4, > + gfxASurface::ImageFormatARGB32); Instead of doing all this, we should just call mTarget->Snapshot()->GetDataSurface(), shouldn't we? @@ +1494,5 @@ > + > + if (mOpaque) > + format = FORMAT_B8G8R8X8; > + > + return format; return mOpaque ? ... : ...; @@ +1564,5 @@ > +{ > + if (!FloatValidate(x,y)) > + return NS_OK; > + > + TransformUpdated(); It makes more sense to call this "TransformWillUpdate" since the transform hasn't been updated yet. @@ +1676,5 @@ > + > + rv = aValue->GetAsAString(str); > + NS_ENSURE_SUCCESS(rv, rv); > + > + return SetStrokeStyle_multi(str, nsnull); I wonder if there's a better way to do this, by splitting up SetStrokeSTyle_multi into separate functions for interface vs string. @@ +1770,5 @@ > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::SetMozFillRule(const nsAString& aString) > +{ > + return NS_OK; Are we going to fix this before landing? @@ +1809,5 @@ > +// gradients and patterns > +// > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::CreateLinearGradient(float x0, float y0, float x1, float y1, > + nsIDOMCanvasGradient **_retval) fix indent @@ +1823,5 @@ > +} > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::CreateRadialGradient(float x0, float y0, float r0, float x1, float y1, float r1, > + nsIDOMCanvasGradient **_retval) fix indent @@ +1841,5 @@ > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::CreatePattern(nsIDOMHTMLElement *image, > + const nsAString& repeat, > + nsIDOMCanvasPattern **_retval) fix indent @@ +1878,5 @@ > + if (canvas && node) { > + if (canvas->CountContexts() == 1) { > + nsICanvasRenderingContextInternal *srcCanvas = canvas->GetContextAtIndex(0); > + > + RefPtr<SourceSurface> srcSurf = srcCanvas->GetSurfaceSnapshot(); Don't we need to null check here in case srcCanvas is not an Azure canvas? ::: content/canvas/test/test_canvas.html @@ +6086,5 @@ > ctx.fillRect(0, 0, 100, 50); > +ctx.translate(-50, 0); > +ctx.fillRect(50, 0, 100, 50); > + > +if (!IsAzureEnabled()) { It's easier to read without the unnecessary "not" operator, flipping around the clauses. @@ +6398,5 @@ > g.addColorStop(1, '#f00'); > ctx.fillStyle = g; > +ctx.fillRect(0, 0, 100, 50); > + > +if (!IsAzureEnabled()) { Can you add comments about the tests you don't pass? Preferably, with bug numbers where you'll fix them. @@ +11146,5 @@ > > } catch (e) { > _thrown_outer = true; > } > +ok(!_thrown_outer, ctx.canvas.id + ' should not throw exception'); Can you move these to their own patch so they don't clutter up the diff? I suggest having a function checkNoException(_thrown_outer, ctx). @@ +13206,5 @@ > +ok(ctx.isPointInPath(0, 10) === true, "ctx.isPointInPath(0, 10) === true"); > +ok(ctx.isPointInPath(10, -0.01) === false, "ctx.isPointInPath(10, -0.01) === false"); > +ok(ctx.isPointInPath(10, 20.01) === false, "ctx.isPointInPath(10, 20.01) === false"); > +ok(ctx.isPointInPath(-0.01, 10) === false, "ctx.isPointInPath(-0.01, 10) === false"); > +ok(ctx.isPointInPath(20.01, 10) === false, "ctx.isPointInPath(20.01, 10) === false"); I don't see what you changed here. @@ +15025,5 @@ > + _thrown_outer = true; > +} > + > +if (!IsAzureEnabled()) { > + todo(_thrown_outer, ctx.canvas.is + ' should throw exception INVALID_STATE_ERR'); ctx.canvas.id I assume this doesn't throw currently ... why should it?

In general a question on this, there's a bunch of situations where these review comments would apply to the current nsCanvasRenderingContext2D as well... If I address them it means the inner workings on this context starts differing more strongly from the original. Do we want that? Or do we maybe want me to address them in the old canvas context as well? (In reply to comment #43) > Comment on attachment 540665 [details] [diff] [review] [review] > > @@ +1770,5 @@ > > + > > +NS_IMETHODIMP > > +nsCanvasRenderingContext2DAzure::SetMozFillRule(const nsAString& aString) > > +{ > > + return NS_OK; > > Are we going to fix this before landing? Yes > > I don't see what you changed here. > > @@ +15025,5 @@ > > + _thrown_outer = true; > > +} > > + > > +if (!IsAzureEnabled()) { > > + todo(_thrown_outer, ctx.canvas.is + ' should throw exception INVALID_STATE_ERR'); > > ctx.canvas.id > > I assume this doesn't throw currently ... why should it?

(In reply to comment #44) > > > > I don't see what you changed here. > > > > @@ +15025,5 @@ > > > + _thrown_outer = true; > > > +} > > > + > > > +if (!IsAzureEnabled()) { > > > + todo(_thrown_outer, ctx.canvas.is + ' should throw exception INVALID_STATE_ERR'); > > > > ctx.canvas.id > > > > I assume this doesn't throw currently ... why should it? What I meant to say here was the spec changed(I think, at least the spec says it should throw and we currently don't) and it now says that :). Since I implemented from the spec and not the old canvas implementation, it behaves differently.

Comment on attachment 540666 [details] [diff] [review] Azure API and D2D backend >+struct GlyphBuffer >+{ >+ GlyphBuffer() : mIsRTL(false) {} >+ >+ std::vector<Glyph> mGlyphs; >+ bool mIsRTL; >+}; I'm not a huge fan of the use of vector here: 1. it requires us to include <vector> in the header which is causing build pain 2. it's probably slower than just passing in a const Glyph*. A simple test case I put together showed filling a vector with 100 glyphs to be 10x slower than allocating the array and populating it. I hope that counting the number of glyphs wouldn't take a lot more time than filling the vector. Also, I don't think we should have mIsRTL.

(In reply to comment #44) > In general a question on this, there's a bunch of situations where these > review comments would apply to the current nsCanvasRenderingContext2D as > well... If I address them it means the inner workings on this context starts > differing more strongly from the original. Do we want that? Yes. > Or do we maybe > want me to address them in the old canvas context as well? No. (In reply to comment #45) > What I meant to say here was the spec changed(I think, at least the spec > says it should throw and we currently don't) and it now says that :). Since > I implemented from the spec and not the old canvas implementation, it > behaves differently. OK.

Comment on attachment 540665 [details] [diff] [review] Add Azure Canvas and adjust tests for it Review of attachment 540665 [details] [diff] [review]: ----------------------------------------------------------------- You should document the path scheme, probably where the member variables are. It seems to be: keep the path either in user space or device space but never both; when drawing with the path, make sure it's in user space; when starting a new path, make it in user space; when changing the transform while a nonempty user-space path exists, switch the path to device space and keep it there until one of the other conditions kicks in. I think that's pretty much optimal, so good work! ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +592,5 @@ > + RefPtr<Path> mDSPath; > + RefPtr<PathBuilder> mDSPathBuilder; > + RefPtr<PathBuilder> mPathBuilder; > + bool mPathTransformUpdated; > + Matrix2D mPathToDS; It looks like mPathBuilder and mDSPathbuilder are never used at the same time. Maybe also for mPath/mDSPath? Maybe it would make more sense to just have mPath/mPathBuilder and a boolean to indicate whether they're in device or user space. @@ +1992,5 @@ > + return NS_OK; > +} > + > +nsresult > +nsCanvasRenderingContext2DAzure::DrawPath(Style style, gfxRect *dirtyRect) This should return void. @@ +2006,5 @@ > + Stroke(mPath, GeneralPattern().ForStyle(this, style, mTarget), > + StrokeOptions(CurrentState().lineWidth, CurrentState().lineJoin, > + CurrentState().lineCap, CurrentState().miterLimit), > + DrawOptions(CurrentState().globalAlpha, CurrentState().op)); > + return NS_OK; Fix indent @@ +2028,5 @@ > +{ > + if (!FloatValidate(x,y,w,h)) > + return NS_OK; > + > + mTarget->FillRect(mozilla::gfx::Rect(x, y, w, h), ColorPattern(Color(0, 0, 0)), DrawOptions(1.0f, OP_CLEAR)); Lose mozilla::gfx:: ... everywhere!!! I'm not going to comment on it anymore :-) @@ +2041,5 @@ > + return NS_OK; > + > + bool doDrawShadow = NeedToDrawShadow(); > + > + RefPtr<DrawTarget> target = mTarget; Not used? Remove. @@ +2062,5 @@ > + } else if (CurrentState().patternStyles[STYLE_FILL]->mRepeat == > + nsCanvasPatternAzure::REPEAT) { > + limitx = false; > + limity = false; > + } Would be simpler as ... repeat = CurrentState().patternStyles[STYLE_FILL]->mRepeat; bool limitx = repeat == nsCanvasPatternAzure::NOREPEAT || repeat == nsCanvasPatternAzure::REPEATY; bool limity = repeat == nsCanvasPatternAzure::NOREPEAT || repeat == nsCanvasPatternAzure::REPEATX; @@ +2065,5 @@ > + limity = false; > + } > + > + IntSize patternSize = > + CurrentState().patternStyles[STYLE_FILL]->mSurface->GetSize(); It might actually be worth factoring out const ContextState& state = CurrentState(); into all the functions that use CurrentState() more than once. I suspect that would save a significant amount of generated code as well as being a bit shorter to write. @@ +2083,5 @@ > + w = patternSize.width - x; > + if (w < 0) { > + w = 0; > + } > + } Simpler as float x2 = NS_MIN(patternSize.width, x + w); x = NS_MAX(0, x); w = x2 - x; But does this handle negative w/h correctly? The spec seems unclear. @@ +2100,5 @@ > + if (h < 0) { > + h = 0; > + } > + } > + } As above. @@ +2226,5 @@ > + return NS_OK; > + > + EnsureWritablePath(); > + > + if (!mDSPathBuilder) { Lose unnecessary negation: if (mPathBuilder) { (and similar below) @@ +2365,5 @@ > + anticlockwise = (dir < 0); > + cx = x3 + any*radius*(anticlockwise ? 1 : -1); > + cy = y3 - anx*radius*(anticlockwise ? 1 : -1); > + angle0 = atan2((y3-cy), (x3-cx)); > + angle1 = atan2((y4-cy), (x4-cx)); I'm not checking the math here. @@ +2464,5 @@ > + mTarget->GetTransform() * currentEndPoint); > + } > + > + arcSweepLeft -= M_PI / 2.0f; > + currentStartAngle = currentEndAngle; I'm not checking the math here. @@ +2480,5 @@ > + } > + > + EnsureWritablePath(); > + > + mPathBuilder->MoveTo(Point2D(x, y)); Don't have you have to handle the mDSPathBuilder case here? Perhaps we need more tests? :-) @@ +2503,5 @@ > + */ > +static nsresult > +CreateFontStyleRule(const nsAString& aFont, > + nsINode* aNode, > + css::StyleRule** aResult) Should just return already_AddRefed<StyleRule> instead of an out parameter. I guess you copied this code though. @@ +2505,5 @@ > +CreateFontStyleRule(const nsAString& aFont, > + nsINode* aNode, > + css::StyleRule** aResult) > +{ > + nsRefPtr<css::StyleRule> rule; Put "using namespace mozilla::css;" somewhere @@ +2903,5 @@ > + gfxPlatform::GetPlatform()->GetScaledFontForFont(font); > + > + GlyphBuffer buffer; > + > + buffer.mIsRTL = mTextRun->IsRightToLeft(); This should go away. @@ +2994,5 @@ > +}; > + > +nsresult > +nsCanvasRenderingContext2DAzure::DrawOrMeasureText(const nsAString& aRawText, > + float aX, Fix indent @@ +3050,5 @@ > + isRTL = GET_BIDI_OPTION_DIRECTION(document->GetBidiOptions()) == IBMBIDI_TEXTDIRECTION_RTL; > + } > + > + // don't need to take care of these with stroke since Stroke() does that > + PRBool doDrawShadow = aOp == TEXT_DRAW_OPERATION_FILL && NeedToDrawShadow(); nsCanvasBidiProcessorAzure calls DrawTarget::Stroke, not nsCanvasRenderingContext2D::Stroke, so I don't see how shadows are drawn for stroked glyphs. @@ +3202,5 @@ > + > +gfxFontGroup *nsCanvasRenderingContext2DAzure::GetCurrentFontStyle() > +{ > + // use lazy initilization for the font group since it's rather expensive > + if(!CurrentState().fontGroup) { if ( @@ +3216,5 @@ > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::MozDrawText(const nsAString& textToDraw) > +{ > +#if 0 Just remove (and below) @@ +3256,5 @@ > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::MozMeasureText(const nsAString& textToMeasure, float *retVal) > +{ > + nsCOMPtr<nsIDOMTextMetrics> metrics; Might as well remove this too. @@ +3450,5 @@ > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::GetLineCap(nsAString& capstyle) > +{ > + if (CurrentState().lineCap == CAP_BUTT) switch! @@ +3485,5 @@ > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::GetLineJoin(nsAString& joinstyle) > +{ > + if (CurrentState().lineJoin == JOIN_ROUND) Switch! @@ +3577,5 @@ > + nsHTMLCanvasElement* canvas = nsHTMLCanvasElement::FromContent(content); > + if (canvas) { > + nsIntSize size = canvas->GetSize(); > + if (size.width == 0 || size.height == 0) { > + return NS_ERROR_DOM_INVALID_STATE_ERR; Why are we doing this? Surely if the source canvas is empty we should silently do nothing. @@ +3607,5 @@ > + } > + > + if (srcSurf) { > + IntSize size = srcSurf->GetSize(); > + imgSize = gfxIntSize(size.width, size.height); So, for Azure canvas sources you never populate the image cache, so you do a security check on every draw, so things actually won't be very fast if you draw the same canvas a lot. Can you just not special-case Azure canvas sources? @@ +3670,5 @@ > + } > + > + if (sw == 0.0 || sh == 0.0) { > + // zero-sized source -- failure !? > + return NS_ERROR_DOM_INDEX_SIZE_ERR; Should not be a failure, should just return OK. @@ +3683,5 @@ > + // The following check might do the validation of the float arguments: > + // (!FloatValidate(sx, sy, sw, sh) || !FloatValidate(dx, dy, dw, dh)) > + // but we would also need to validate some sums for overflow (e.g. sx + sw). > + if (!FloatValidate(sx + sw, sy + sh, dx + dw, dy + dh)) { > + return NS_ERROR_DOM_SYNTAX_ERR; should be returning OK @@ +4101,5 @@ > + } > +} > + > +void > +nsCanvasRenderingContext2DAzure::EnsureWritablePath() { { on new line Seems like the path helpers shouldn't be between getimagedata/putimagedata. @@ +4152,5 @@ > + p1 = inverse * p1; > + if (!inverse.Invert()) { > + return; > + } > + p1 = inverse * p1; What's p1 for? Seems like it should be removed? @@ +4189,5 @@ > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::PutImageData_explicit(PRInt32 x, PRInt32 y, PRUint32 w, PRUint32 h, > + unsigned char *aData, PRUint32 aDataLen, > + PRBool hasDirtyRect, PRInt32 dirtyX, PRInt32 dirtyY, > + PRInt32 dirtyWidth, PRInt32 dirtyHeight) Fix indent @@ +4292,5 @@ > + Matrix2D oldTransform = mTarget->GetTransform(); > + mTarget->SetTransform(Matrix2D()); > + mTarget->DrawSurface(sourceSurface, dirtyRect, > + mozilla::gfx::Rect(dirtyRect.x - x, dirtyRect.y - y, > + dirtyRect.width, dirtyRect.height)); Why not just draw with OP_SOURCE instead of clearing first?

Your API patch needs to use hg rename to move the files so we don't have new copies and keep the history.

Comment on attachment 540972 [details] [diff] [review] Remove or modify tests depending on mozDrawText Review of attachment 540972 [details] [diff] [review]: -----------------------------------------------------------------

This should be what we need, my proposal is to land this soon so we can land azure integration in separate patches, reducing the work if a backout is needed. It uses the new mfbt refptrs and unifies the Base* classes as well as renaming Point2D and Matrix2D to Point and Matrix respectively.

Corrected line ending style in Rect.h.

(In reply to comment #48) > Comment on attachment 540665 [details] [diff] [review] [review] > Add Azure Canvas and adjust tests for it > > Review of attachment 540665 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp > @@ +592,5 @@ > > + RefPtr<Path> mDSPath; > > + RefPtr<PathBuilder> mDSPathBuilder; > > + RefPtr<PathBuilder> mPathBuilder; > > + bool mPathTransformUpdated; > > + Matrix2D mPathToDS; > > It looks like mPathBuilder and mDSPathbuilder are never used at the same > time. Maybe also for mPath/mDSPath? Maybe it would make more sense to just > have mPath/mPathBuilder and a boolean to indicate whether they're in device > or user space. I played with this, mDSPath could go away. mDSPathBuilder and mPathBuilder I left in place, otherwise you'd get things like 'if (mPathBuilder && pathIsInUserSpace)' instead of 'if (mPathBuilder)' and such. > @@ +1992,5 @@ > > + return NS_OK; > > +} > > + > > +nsresult > > +nsCanvasRenderingContext2DAzure::DrawPath(Style style, gfxRect *dirtyRect) > > This should return void. Actually, it has no value anyway, I made it go away. > @@ +2028,5 @@ > > +{ > > + if (!FloatValidate(x,y,w,h)) > > + return NS_OK; > > + > > + mTarget->FillRect(mozilla::gfx::Rect(x, y, w, h), ColorPattern(Color(0, 0, 0)), DrawOptions(1.0f, OP_CLEAR)); > > Lose mozilla::gfx:: ... everywhere!!! I'm not going to comment on it anymore > :-) Note the correlation here! It's always mozilla::gfx::Rect - Rect conflicts with the Rect function in nsICanvasRenderingContext :(. > > Simpler as > float x2 = NS_MIN(patternSize.width, x + w); > x = NS_MAX(0, x); > w = x2 - x; > > But does this handle negative w/h correctly? The spec seems unclear. I think so, the tests pass, although that doesn't say everything! > @@ +2503,5 @@ > > + */ > > +static nsresult > > +CreateFontStyleRule(const nsAString& aFont, > > + nsINode* aNode, > > + css::StyleRule** aResult) > > Should just return already_AddRefed<StyleRule> instead of an out parameter. > I guess you copied this code though. I could do that, but I'd rather not, this code is just copied and I'd prefer to leave the tested code and address this in a safe follow-up. > @@ +3050,5 @@ > > + isRTL = GET_BIDI_OPTION_DIRECTION(document->GetBidiOptions()) == IBMBIDI_TEXTDIRECTION_RTL; > > + } > > + > > + // don't need to take care of these with stroke since Stroke() does that > > + PRBool doDrawShadow = aOp == TEXT_DRAW_OPERATION_FILL && NeedToDrawShadow(); > > nsCanvasBidiProcessorAzure calls DrawTarget::Stroke, not > nsCanvasRenderingContext2D::Stroke, so I don't see how shadows are drawn for > stroked glyphs. This comment is bogus, we draw them through the adjustedTarget, this is only to determine if the bounding box can be easily determined. > @@ +3577,5 @@ > > + nsHTMLCanvasElement* canvas = nsHTMLCanvasElement::FromContent(content); > > + if (canvas) { > > + nsIntSize size = canvas->GetSize(); > > + if (size.width == 0 || size.height == 0) { > > + return NS_ERROR_DOM_INVALID_STATE_ERR; > > Why are we doing this? Surely if the source canvas is empty we should > silently do nothing. http://dev.w3.org/html5/2dcontext/#dom-context-2d-drawimage 'If the image argument is an HTMLCanvasElement object with either a horizontal dimension or a vertical dimension equal to zero, then the implementation must raise an INVALID_STATE_ERR exception.' I didn't write this thing :-) > > @@ +3607,5 @@ > > + } > > + > > + if (srcSurf) { > > + IntSize size = srcSurf->GetSize(); > > + imgSize = gfxIntSize(size.width, size.height); > > So, for Azure canvas sources you never populate the image cache, so you do a > security check on every draw, so things actually won't be very fast if you > draw the same canvas a lot. Can you just not special-case Azure canvas > sources? Mixed canvas sources will be super super rare.. does it matter? > > @@ +3670,5 @@ > > + } > > + > > + if (sw == 0.0 || sh == 0.0) { > > + // zero-sized source -- failure !? > > + return NS_ERROR_DOM_INDEX_SIZE_ERR; > > Should not be a failure, should just return OK. Again: 'If one of the sw or sh arguments is zero, the implementation must raise an INDEX_SIZE_ERR exception.' > > @@ +3683,5 @@ > > + // The following check might do the validation of the float arguments: > > + // (!FloatValidate(sx, sy, sw, sh) || !FloatValidate(dx, dy, dw, dh)) > > + // but we would also need to validate some sums for overflow (e.g. sx + sw). > > + if (!FloatValidate(sx + sw, sy + sh, dx + dw, dy + dh)) { > > + return NS_ERROR_DOM_SYNTAX_ERR; > > should be returning OK It actually did, I validate higher up now, removed this. > @@ +4292,5 @@ > > + Matrix2D oldTransform = mTarget->GetTransform(); > > + mTarget->SetTransform(Matrix2D()); > > + mTarget->DrawSurface(sourceSurface, dirtyRect, > > + mozilla::gfx::Rect(dirtyRect.x - x, dirtyRect.y - y, > > + dirtyRect.width, dirtyRect.height)); > > Why not just draw with OP_SOURCE instead of clearing first? Azure's OP_SOURCE is not bound by the mask (like canvas op source), but only by the clip, OP_SOURCE with clip currently doesn't perform too awesome in the D2D azure backend. We can change this later and add a specialized path in the D2D backend that's optimized if needed. (In reply to comment #43) > Comment on attachment 540665 [details] [diff] [review] [review] > Add Azure Canvas and adjust tests for it > > Review of attachment 540665 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > @@ +285,5 @@ > > + > > + if (!mRawStops.Length() && mOffsetStart > 0) { > > + newStop.offset = mOffsetStart; > > + newStop.color = Color::FromABGR(color); > > + mRawStops.AppendElement(newStop); > > Why do we need to append this extra stop? > > @@ +308,5 @@ > > + Point2D mBegin; > > + Point2D mEnd; > > + Point2D mCenter; > > + Float mRadius; > > + Point2D mOrigin; > > These need to be documented. What exactly is mOrigin? I documented these with the bug that will make them go away :). > @@ +552,5 @@ > > + > > + // our drawing surfaces, contexts, and layers > > + RefPtr<DrawTarget> mTarget; > > + > > + PRUint32 mSaveCount; > > Why not just make this a method that returns mStyleStack.Length() - 1? I just replaced the couple of places that used it with mStyleStack.Length() - 1. > @@ +625,5 @@ > > + // The spec says we should not draw shadows when the alpha value is 0, > > + // regardless of the operator being used. > > + return state.op == OP_OVER && state.StyleIsColor(STYLE_SHADOW) && > > + NS_GET_A(state.colorStyles[STYLE_SHADOW]) > 0 && > > + (state.shadowOffset != Point2D(0, 0) || state.shadowBlur != 0); > > This is obsolete now. We should draw shadows regardless of the offset/blur. > This should have caused a test failure... Latest spec says (Working Draft May 24th): 'Shadows are only drawn if the opacity component of the alpha component of the color of shadowColor is non-zero and either the shadowBlur is non-zero, or the shadowOffsetX is non-zero, or the shadowOffsetY is non-zero.' Also, what's the point of drawing a shadow that's completely under an object? That could just do some weird color blending. Which then should really be done by adjusting the color value. > > @@ +777,5 @@ > > + TextBaseline textBaseline; > > + > > + nscolor colorStyles[STYLE_MAX]; > > + nsCOMPtr<nsCanvasGradientAzure> gradientStyles[STYLE_MAX]; > > + nsCOMPtr<nsCanvasPatternAzure> patternStyles[STYLE_MAX]; > > nsRefPtrs. > > But shouldn't we actually have an array of structs here? Or even a struct > which is internally a union? Would be faster to copy since it's smaller. > > And why not just store the shadow color as a separate thing instead of as a > peer of fillStyle/strokeStyle? This is what Canvas does. I have no idea why that decision was made, but I didn't feel like messing with these kind of internals at this point. If we change this in my opinion we should change it in both Azure and non-Azure, to make sure one impl doesn't have a bug the other one doesn't. > @@ +779,5 @@ > > + nscolor colorStyles[STYLE_MAX]; > > + nsCOMPtr<nsCanvasGradientAzure> gradientStyles[STYLE_MAX]; > > + nsCOMPtr<nsCanvasPatternAzure> patternStyles[STYLE_MAX]; > > + > > + Matrix2D transform; > > This "2D" suffix is going away right? :-) It has gone! :) > @@ +828,5 @@ > > + mPattern = new (mRadialGradientPattern.addr()) > > + RadialGradientPattern(state.gradientStyles[aStyle]->mCenter, > > + state.gradientStyles[aStyle]->mOrigin, > > + state.gradientStyles[aStyle]->mRadius, > > + state.gradientStyles[aStyle]->GetGradientStopsForTarget(aRT)); > > What's the story about radial gradients with different circle centers? Are > you going to add that to Azure after this lands? Indeed, documented with the bug number. See bug 666097. > > @@ +840,5 @@ > > + ExtendMode mode; > > + if (state.patternStyles[aStyle]->mRepeat == nsCanvasPatternAzure::NOREPEAT) { > > + mode = EXTEND_CLAMP; > > + } else { > > + mode = EXTEND_WRAP; > > When are we going to make repeating work properly? I hope that's done by > remove SurfacePattern::mExtendMode and using a tile-space rect instead. Repeating works right for fillRect, I'm going to make it work right for all other operations next week. But not by that method but simply by clipping to a rect of the height of the image infinitely wide or the width of the image infinitely high :). So no Azure changes for the initial implementation. We can discuss tile-space rects in-depth and with some more thought later I think :). > @@ +862,5 @@ > > + class AdjustedTarget > > + { > > + public: > > + AdjustedTarget(nsCanvasRenderingContext2DAzure *ctx, > > + const mozilla::gfx::Rect &aBounds = mozilla::gfx::Rect()) > > Lose mozilla::gfx::, and document aBounds. If it's an optional parameter, > why not make it a pointer so that null can be passed in? But it looks like > no-one passes a value here, so I suggest taking it out altogether, > especially since it appears the non-default path is not implemented. It will be used soon (before aurora) see bug 666452, added a comment for that. > @@ +889,5 @@ > > + mSurfOffset.x = -ctx->CurrentState().shadowOffset.x; > > + transform._31 += ctx->CurrentState().shadowOffset.x; > > + } else { > > + mTempSize.width -= ctx->CurrentState().shadowOffset.x; > > + } > > I think this code might be clearer if you worked with rects, and took the > union of the shadow and drawing rects here. That would also extend more > cleanly to having more precise bounds passed in. If it's okay I'll do that as part of bug 666452, which requires me to refactor that code a bit anyway. > @@ +948,5 @@ > > + IntSize mTempSize; > > + Point2D mSurfOffset; > > + }; > > + > > + nsTArray<ContextState> mStyleStack; > > This means that every save() and restore() that's not nested is going to do > an alloc and free, that's bad. (I see that you construct with an initial > capacity, but the first RemoveElementAt will reduce the capacity.) We should > either do an nsAutoTArray<ContextState,2> or something like that, or else > make Save() and Restore() not shrink the array (e.g. have Save() do > EnsureCapacity instead of SetCapacity, and have Restore() just clear the > state instead of removing the array element). Changed this to an nsAutoTArray, if this is important we should move this change to nsCanvasRenderingContext as well probably. I just copied it from there. > @@ +1238,5 @@ > > + ++mInvalidateCount; > > + return NS_OK; > > + } > > + > > + mozilla::gfx::Rect newr = mTarget->GetTransform() * ToRect(r); > > Lose mozilla::gfx:: > > I didn't notice you support Matrix * Rect. I think that's not so good since > this doesn't just transform, it also expands to the axis-aligned bounding > rect. So I'd prefer an explicit TransformBounds like we have elsewhere. That operator was bogus, shouldn't be there and didn't even do the right thing! I removed it and implemented TransformBounds properly. > @@ +1278,5 @@ > > + > > + if (layerManager) { > > + target = layerManager->CreateDrawTarget(size, format); > > + } else { > > + target = Factory::CreateDrawTarget(BACKEND_DIRECT2D, size, format); > > Ick. I'm not sure what to do here, but probably a cairo image surface > backend is the safest bet once we support that. That was my idea longer term, BACKEND_<whatever is the software backend> > @@ +1396,5 @@ > > + if (NS_FAILED(GetThebesSurface(getter_AddRefs(surface)))) { > > + return NS_ERROR_FAILURE; > > + } > > + > > + mTarget->Flush(); > > The Flush() should be inside GetThebesSurface() --- if it's needed. It is in GetThebesSurface, this was a waste. > @@ +1461,5 @@ > > + nsRefPtr<gfxImageSurface> imgsurf = > > + new gfxImageSurface(imageBuffer.get(), > > + gfxIntSize(mWidth, mHeight), > > + mWidth * 4, > > + gfxASurface::ImageFormatARGB32); > > Instead of doing all this, we should just call > mTarget->Snapshot()->GetDataSurface(), shouldn't we? This seems used rarely and seems hard to test. Since this data might go into a decoder it also seemed like an interesting attack vector, I decided to play this safe for now and use the old codepath. So we could address it later in more detail and with more testing. I can change that if you prefer? > @@ +1676,5 @@ > > + > > + rv = aValue->GetAsAString(str); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + return SetStrokeStyle_multi(str, nsnull); > > I wonder if there's a better way to do this, by splitting up > SetStrokeSTyle_multi into separate functions for interface vs string. This function wasn't really my expertise, again I'd suggest if we change it we do it in a separate patch for both Azure and normal canvas. > @@ +1770,5 @@ > > + > > +NS_IMETHODIMP > > +nsCanvasRenderingContext2DAzure::SetMozFillRule(const nsAString& aString) > > +{ > > + return NS_OK; > > Are we going to fix this before landing? It's fixed. > @@ +1878,5 @@ > > + if (canvas && node) { > > + if (canvas->CountContexts() == 1) { > > + nsICanvasRenderingContextInternal *srcCanvas = canvas->GetContextAtIndex(0); > > + > > + RefPtr<SourceSurface> srcSurf = srcCanvas->GetSurfaceSnapshot(); > > Don't we need to null check here in case srcCanvas is not an Azure canvas? Good catch, should be rare but seems like a good idea indeed.

Comment on attachment 541263 [details] [diff] [review] Add glue code in gfx for thebes-azure interop Talked on IRC about ripping the USE_CAIRO stuff out for now.

Updated to not allow cairo surface drawtarget creation.

Comment on attachment 541264 [details] [diff] [review] Part 2: Add glue code in gfx for thebes-azure interop Carying on r+

This is the code that allows us to use a pref to create an Azure CanvasRenderingContext2D.

This addresses most of roc's comments.

This updates the license header and fixes an indent issue I missed.

Comment on attachment 541265 [details] [diff] [review] Part 7: Add code that allows choosing the Azure Canvas Backend Review of attachment 541265 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLCanvasElement.cpp @@ +733,5 @@ > + nsCOMPtr<nsIPrefBranch> prefService = do_GetService(NS_PREFSERVICE_CONTRACTID); > + NS_ENSURE_TRUE(prefService != nsnull, NS_ERROR_FAILURE); > + > + PRBool azure = PR_FALSE; > + prefService->GetBoolPref("gfx.canvas.azure.enabled", &azure); Use Preferences::GetBool()

(In reply to comment #54) > Note the correlation here! It's always mozilla::gfx::Rect - Rect conflicts > with the Rect function in nsICanvasRenderingContext :(. OK, do namespace gfx = mozilla::gfx; And then at least you can write gfx:: instead of mozilla::gfx:: > > @@ +3607,5 @@ > > > + } > > > + > > > + if (srcSurf) { > > > + IntSize size = srcSurf->GetSize(); > > > + imgSize = gfxIntSize(size.width, size.height); > > > > So, for Azure canvas sources you never populate the image cache, so you do a > > security check on every draw, so things actually won't be very fast if you > > draw the same canvas a lot. Can you just not special-case Azure canvas > > sources? > > Mixed canvas sources will be super super rare.. does it matter? It seems to me that painting an Azure canvas to an Azure destination will be slow because you bypass the cache and have to do a security check every time. Are you saying you think that's rare? > > @@ +4292,5 @@ > > > + Matrix2D oldTransform = mTarget->GetTransform(); > > > + mTarget->SetTransform(Matrix2D()); > > > + mTarget->DrawSurface(sourceSurface, dirtyRect, > > > + mozilla::gfx::Rect(dirtyRect.x - x, dirtyRect.y - y, > > > + dirtyRect.width, dirtyRect.height)); > > > > Why not just draw with OP_SOURCE instead of clearing first? > > Azure's OP_SOURCE is not bound by the mask (like canvas op source), but only > by the clip, OP_SOURCE with clip currently doesn't perform too awesome in > the D2D azure backend. We can change this later and add a specialized path > in the D2D backend that's optimized if needed. We certainly should! Please add an XXX comment to that effect. For a simple CPU-based renderer, drawing to a cliprect with OP_SOURCE should be faster than Clear followed by a separate DrawSurface. But hang on, why does nsCanvasRenderingContext2DAzure::ClearRect work then? Shouldn't the OP_CLEAR there clear the entire clip region? > > @@ +625,5 @@ > > > + // The spec says we should not draw shadows when the alpha value is 0, > > > + // regardless of the operator being used. > > > + return state.op == OP_OVER && state.StyleIsColor(STYLE_SHADOW) && > > > + NS_GET_A(state.colorStyles[STYLE_SHADOW]) > 0 && > > > + (state.shadowOffset != Point2D(0, 0) || state.shadowBlur != 0); > > > > This is obsolete now. We should draw shadows regardless of the offset/blur. > > This should have caused a test failure... > > Latest spec says (Working Draft May 24th): > > 'Shadows are only drawn if the opacity component of the alpha component of > the color of shadowColor is non-zero and either the shadowBlur is non-zero, > or the shadowOffsetX is non-zero, or the shadowOffsetY is non-zero.' Yes, but we've changed our behavior deliberately to "draw shadows if operator is OVER, regardless of shadow offset or blur." See the note here: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#shadows "It is likely that this will change: browser vendors have indicated an interest in changing the processing model for shadows such that they only draw when the composition operator is "source-over" (the default)." Note: always refer to the WHATWG draft, it's always at least as up-to-date and accurate as the W3C draft, and usually more so. > Also, what's the point of drawing a shadow that's completely under an > object? That could just do some weird color blending. Which then should > really be done by adjusting the color value. It means that you don't get weird edge-case behavior if you're animating the offset or blur through zero. > This is what Canvas does. I have no idea why that decision was made, but I > didn't feel like messing with these kind of internals at this point. If we > change this in my opinion we should change it in both Azure and non-Azure, > to make sure one impl doesn't have a bug the other one doesn't. I don't think that's a concern. I can live the existing code but I'd like to simplify it where possible. > This seems used rarely and seems hard to test. Since this data might go into > a decoder it also seemed like an interesting attack vector, I decided to > play this safe for now and use the old codepath. So we could address it > later in more detail and with more testing. I can change that if you prefer? You can address it later.

(In reply to comment #63) > (In reply to comment #54) > > Note the correlation here! It's always mozilla::gfx::Rect - Rect conflicts > > with the Rect function in nsICanvasRenderingContext :(. > > OK, do > namespace gfx = mozilla::gfx; > And then at least you can write gfx:: instead of mozilla::gfx:: Ack. > > > > @@ +3607,5 @@ > > > > + } > > > > + > > > > + if (srcSurf) { > > > > + IntSize size = srcSurf->GetSize(); > > > > + imgSize = gfxIntSize(size.width, size.height); > > > > > > So, for Azure canvas sources you never populate the image cache, so you do a > > > security check on every draw, so things actually won't be very fast if you > > > draw the same canvas a lot. Can you just not special-case Azure canvas > > > sources? > > > > Mixed canvas sources will be super super rare.. does it matter? > > It seems to me that painting an Azure canvas to an Azure destination will be > slow because you bypass the cache and have to do a security check every > time. Are you saying you think that's rare? Sorry, I thought you meant special case azure canvas sources so that only azure canvases don't go through the cache. How should I make azure canvases go through the cache? Or should this be address this separately too? > > > @@ +4292,5 @@ > > > > + Matrix2D oldTransform = mTarget->GetTransform(); > > > > + mTarget->SetTransform(Matrix2D()); > > > > + mTarget->DrawSurface(sourceSurface, dirtyRect, > > > > + mozilla::gfx::Rect(dirtyRect.x - x, dirtyRect.y - y, > > > > + dirtyRect.width, dirtyRect.height)); > > > > > > Why not just draw with OP_SOURCE instead of clearing first? > > > > Azure's OP_SOURCE is not bound by the mask (like canvas op source), but only > > by the clip, OP_SOURCE with clip currently doesn't perform too awesome in > > the D2D azure backend. We can change this later and add a specialized path > > in the D2D backend that's optimized if needed. > > We certainly should! Please add an XXX comment to that effect. For a simple > CPU-based renderer, drawing to a cliprect with OP_SOURCE should be faster > than Clear followed by a separate DrawSurface. Yep, I definitely realize that, memcpy and all :). I'm still thinking about how to deal with this. For now this seemed like obviously the fastest approach though. Will add the comment. > > But hang on, why does nsCanvasRenderingContext2DAzure::ClearRect work then? > Shouldn't the OP_CLEAR there clear the entire clip region? > Yeah, I did clear differently since canvas doesn't actually have a clear OP.

Addressed review comment.

(In reply to comment #63) > (In reply to comment #54) > > Note the correlation here! It's always mozilla::gfx::Rect - Rect conflicts > > with the Rect function in nsICanvasRenderingContext :(. > > OK, do > namespace gfx = mozilla::gfx; > And then at least you can write gfx:: instead of mozilla::gfx:: That syntax isn't allowed, I think because gfx is already a namespace in that context. I did namespace mgfx = mozilla::gfx;

Updated some more based on bug traffic.

Carrying r+ from jrmuizel, some small fixes for build issues and conflicts found on different platforms.

(In reply to comment #64) > > It seems to me that painting an Azure canvas to an Azure destination will be > > slow because you bypass the cache and have to do a security check every > > time. Are you saying you think that's rare? > > Sorry, I thought you meant special case azure canvas sources so that only > azure canvases don't go through the cache. How should I make azure canvases > go through the cache? Or should this be address this separately too? What if you don't have a special path for Azure canvases? Things will still work, right? nsLayoutUtils::SurfaceFromElement will end up calling GetThebesSurfaceForDrawTarget and we'll get back a gfxD2DSurface which can be cached and drawn efficiently. So I think you should just take out the special Azure path for now. It's probably a net lose. > > We certainly should! Please add an XXX comment to that effect. For a simple > > CPU-based renderer, drawing to a cliprect with OP_SOURCE should be faster > > than Clear followed by a separate DrawSurface. > > Yep, I definitely realize that, memcpy and all :). I'm still thinking about > how to deal with this. For now this seemed like obviously the fastest > approach though. Will add the comment. Actually I think you should do the right thing (clip + OP_SOURCE) for now and make it fast(er) in D2D as the followup. > > But hang on, why does nsCanvasRenderingContext2DAzure::ClearRect work then? > > Shouldn't the OP_CLEAR there clear the entire clip region? > > Yeah, I did clear differently since canvas doesn't actually have a clear OP. We should make CLEAR the same as the other ops and have ClearRect clip.

(In reply to comment #72) > (In reply to comment #64) > > > It seems to me that painting an Azure canvas to an Azure destination will be > > > slow because you bypass the cache and have to do a security check every > > > time. Are you saying you think that's rare? > > > > Sorry, I thought you meant special case azure canvas sources so that only > > azure canvases don't go through the cache. How should I make azure canvases > > go through the cache? Or should this be address this separately too? > > What if you don't have a special path for Azure canvases? Things will still > work, right? nsLayoutUtils::SurfaceFromElement will end up calling > GetThebesSurfaceForDrawTarget and we'll get back a gfxD2DSurface which can > be cached and drawn efficiently. > > So I think you should just take out the special Azure path for now. It's > probably a net lose. It is in the benchmarks, that's the reason I added this path :). but I can live with that, it's not that big a win. > > > > We certainly should! Please add an XXX comment to that effect. For a simple > > > CPU-based renderer, drawing to a cliprect with OP_SOURCE should be faster > > > than Clear followed by a separate DrawSurface. > > > > Yep, I definitely realize that, memcpy and all :). I'm still thinking about > > how to deal with this. For now this seemed like obviously the fastest > > approach though. Will add the comment. > > Actually I think you should do the right thing (clip + OP_SOURCE) for now > and make it fast(er) in D2D as the followup. Just so we're clear on this, this will make Azure over 2-3x slower on anything that uses putImageData, I don't particularly object, just saying. > > > > But hang on, why does nsCanvasRenderingContext2DAzure::ClearRect work then? > > > Shouldn't the OP_CLEAR there clear the entire clip region? > > > > Yeah, I did clear differently since canvas doesn't actually have a clear OP. > > We should make CLEAR the same as the other ops and have ClearRect clip. In that case let's remove the clear operator altogether and add a clearRect call to Azure. It's all canvas needs for now, we can add a clearPath or something later.

(In reply to comment #73) > (In reply to comment #72) > > (In reply to comment #64) > > > > We certainly should! Please add an XXX comment to that effect. For a simple > > > > CPU-based renderer, drawing to a cliprect with OP_SOURCE should be faster > > > > than Clear followed by a separate DrawSurface. > > > > > > Yep, I definitely realize that, memcpy and all :). I'm still thinking about > > > how to deal with this. For now this seemed like obviously the fastest > > > approach though. Will add the comment. > > > > Actually I think you should do the right thing (clip + OP_SOURCE) for now > > and make it fast(er) in D2D as the followup. > > Just so we're clear on this, this will make Azure over 2-3x slower on > anything that uses putImageData, I don't particularly object, just saying. > Another suggestion, let's add a 'CopySurface' API, similar to DrawSurface but that overwrites the surface. Let's not forget that all this business with clipping and then doing neat tricks with operators to do simple drawing operations is part of what made the Cairo D2D backend slow and terribly hard and messy to optimize.

(In reply to comment #73) > (In reply to comment #72) > > (In reply to comment #64) > > > > > > But hang on, why does nsCanvasRenderingContext2DAzure::ClearRect work then? > > > > Shouldn't the OP_CLEAR there clear the entire clip region? > > > > > > Yeah, I did clear differently since canvas doesn't actually have a clear OP. > > > > We should make CLEAR the same as the other ops and have ClearRect clip. > > In that case let's remove the clear operator altogether and add a clearRect > call to Azure. It's all canvas needs for now, we can add a clearPath or > something later. So, just to elaborate again, the reason I'm saying this is because having an unbounded OP_CLEAR makes no sense. Fill, stroke, which pattern, it all wouldn't matter. Every call would do exactly the same with this 'OP_CLEAR' only the clip would matter, hence a clearRect call actually makes more sense. It maps well onto the canvas API, would be bound by clip and would allow clearing the entire surface.

(In reply to comment #73) > > Actually I think you should do the right thing (clip + OP_SOURCE) for now > > and make it fast(er) in D2D as the followup. > > Just so we're clear on this, this will make Azure over 2-3x slower on > anything that uses putImageData, I don't particularly object, just saying. Only until you fix the D2D backend, which you will do before we make Azure on by default :-). > > > > But hang on, why does nsCanvasRenderingContext2DAzure::ClearRect work then? > > > > Shouldn't the OP_CLEAR there clear the entire clip region? > > > > > > Yeah, I did clear differently since canvas doesn't actually have a clear OP. > > > > We should make CLEAR the same as the other ops and have ClearRect clip. > > In that case let's remove the clear operator altogether and add a clearRect > call to Azure. It's all canvas needs for now, we can add a clearPath or > something later. Why not just use OP_CLEAR instead of new API?

We decided to add ClearRect and CopySurface Azure APIs to handle these issues, and remove OP_CLEAR. The rationale is that for Azure we want to express statically-known special cases directly as Azure API instead of relying on backends to recognize and optimize special cases, at least for anything performance-sensitive. CopySurface will ignore the current clip and transform.

Updated to new API's that were agreed on.

Updated as per review comments.

This updates the patch to mark one test failing on Mac OSX and links the failure to the created bug. Carrying r+ from roc.

Updated to apply and compile cleanly on mozilla-central by itself. Also updated to report BaseRect/etc. as renames properly and only report modifications.

Turns out I left a cairo function and a makefile modification out of the patch accidentally, added that.

Comment on attachment 541572 [details] [diff] [review] Part 2: Add glue code in gfx for thebes-azure interop v2 Review of attachment 541572 [details] [diff] [review]: -----------------------------------------------------------------

This just updates the patch to apply by itself, moving a big from Part 7 into Part 6.

Updated as per aforementioned shuffle.

Comment on attachment 541285 [details] [diff] [review] Part 8: Update canvas tests to deal with Azure. Review of attachment 541285 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/test/test_canvas.html @@ +16743,5 @@ > +ctx.fillRect(-100, 0, 200, 50); > + > +if (!IsAzureEnabled()) { > + // This is going away as only OVER will have shadows. > + isPixel(ctx, 50, 25, 0, 255, 0, 255, 2); Instead of disabling the test for Azure (effectively), change this to check for 255,255,0,255, which is the expected rendering when only OVER has shadows. I'm disturbed that this is currently passing on trunk, actually. Maybe we don't implement XOR correctly on trunk? Better make it todo for non-Azure. @@ +16773,5 @@ > +ctx.fillRect(-10, -10, 120, 70); > + > +if (!IsAzureEnabled()) { > + // This is going away as only OVER will have shadows. > + isPixel(ctx, 50, 25, 0, 255, 0, 255, 2); Same as above.

This corrects a wrong negate for the fails-if.

Correct accidentally left out hunk.

Updated to deal with comments.

Comment on attachment 541587 [details] [diff] [review] Part 9: Update canvas tests to deal with Azure. v2 Review of attachment 541587 [details] [diff] [review]: -----------------------------------------------------------------

Comment on attachment 541570 [details] [diff] [review] Part 1: Azure API and D2D backend v6 Review of attachment 541570 [details] [diff] [review]: ----------------------------------------------------------------- Matrix looks OK. The only compatibility issue between it and gfxMatrix is that gfxMatrix::Invert returns the matrix whereas Matrix::Invert returns a bool, but the latter is much better so that's OK. However, we do need to unify those classes into a single BaseMatrix template soonish (separate patch to reduce risk). ::: gfx/2d/2D.h @@ +92,5 @@ > + , mAntialiasMode(aAntialiasMode) > + , mSnapping(aSnapping) > + {} > + > + float mAlpha; Float @@ +155,5 @@ > + GradientStops() {} > +}; > + > +/* > + * This is the base class for 'patterns' patterns describe the pixels used as "'patterns'. Patterns" @@ +161,5 @@ > + * drawing commands. These objects are not backend specific, however for > + * example the gradient stops on a gradient pattern can be backend specific. > + */ > +class Pattern > +{ { on same line as Pattern. Same goes for most of your classes (but not your structs, curiously). @@ +184,5 @@ > + Color mColor; > +}; > + > +/* > + * This stack-based class is used for Linear Gradient Patterns, the gradient You didn't address my previous comment: > I don't think we need to describe these classes as "stack-based". We need to say they can be used on the stack, but they don't have > to be. @@ +264,5 @@ > + Filter mFilter; > +}; > + > +/* > + * This base class is implemented by SourceSurfaces. These objects are surfaces My previous comment: "This is the base class for source surfaces. These objects are surfaces" @@ +324,5 @@ > + */ > + virtual void Close() = 0; > + /* Add an arc to the current figure */ > + virtual void Arc(const Point &aOrigin, float aRadius, float aStartAngle, > + float aEndAngle, bool aAntiClockwise = false) = 0; Previous comment "Shouldn't these be Float instead of float?" @@ +369,5 @@ > +class PathBuilder : public PathSink > +{ > +public: > + /* Finish writing to the path and return a Path object that can be used for > + * drawing Previous comment "Note that future writes to the PathSink will be ignored and future calls to Finish() will return null (or if you prefer, return the same path)." @@ +387,5 @@ > + */ > +struct GlyphBuffer > +{ > + // A pointer to a buffer of glyphs. Managed by the caller. > + Glyph *mGlyphs; const Glyph * @@ +392,5 @@ > + // Number of glyphs mGlyphs points to. > + uint32_t mNumGlyphs; > +}; > + > +/* This class is an abstraction of a backend/platform specific font object. ... at a particular size. @@ +405,5 @@ > + virtual FontType GetType() const = 0; > + > + /* This allows getting a path that describes the outline of a set of glyphs. > + * A target is passed in so that font backends may generate the path for a > + * compatible drawtarget in an optimized manner. It's not just that. A Path only works with a particular backend, so we guarantee that the path is created for the DrawTarget's backend and can be used with any DrawTarget for that backend. @@ +441,5 @@ > + * > + * aSurface Source surface to draw > + * aDest Destination rectangle that this drawing operation should draw to > + * aSource Source rectangle in aSurface coordinates, this area of aSurface > + * will be stretched to the size of aDest. Mention that we don't sample outside of aSource. @@ +470,5 @@ > + Float aSigma) = 0; > + > + /* > + * Clear a rectangle on the draw target to transparent black. This will > + * respect the clipping region. ... and transform. ::: gfx/2d/DrawTargetCG.cpp @@ +59,5 @@ > + mode = kCGBlendModeCopy; > + break; > + case OP_CLEAR: > + mode = kCGBlendModeClear; > + break; This won't build now that OP_CLEAR is gone.

Comment on attachment 541580 [details] [diff] [review] Part 7: Add code that allows choosing the Azure Canvas Backend v3 Review of attachment 541580 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that ::: content/html/content/src/nsHTMLCanvasElement.cpp @@ +730,5 @@ > +nsresult > +NS_NewCanvasRenderingContext2D(nsIDOMCanvasRenderingContext2D** aResult) > +{ > + PRBool azure = PR_FALSE; > + nsresult rv = mozilla::Preferences::GetBool("gfx.canvas.azure.enabled", &azure); using namespace mozilla;

Comment on attachment 541585 [details] [diff] [review] Part 10: Remove or modify tests depending on mozDrawText v3 Review of attachment 541585 [details] [diff] [review]: -----------------------------------------------------------------

Adjusted to actually have the code!

Address review-comments. Carrying review from jrmuizel.

Address an issue with self-copy.

Comment on attachment 541591 [details] [diff] [review] Part 6: Add Azure Canvas v5 Review of attachment 541591 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/public/nsICanvasRenderingContextInternal.h @@ +102,5 @@ > NS_IMETHOD GetThebesSurface(gfxASurface **surface) = 0; > + > + // This gets an Azure SourceSurface for the canvas, this will be a snapshot > + // of the canvas at the time it was called. > + virtual mozilla::TemporaryRef<mozilla::gfx::SourceSurface> GetSurfaceSnapshot() = 0; previous comment: "So this returns null for non-Azure canvases? Please document." ::: content/canvas/src/WebGLContext.h @@ +342,5 @@ > const PRUnichar* aEncoderOptions, > nsIInputStream **aStream); > NS_IMETHOD GetThebesSurface(gfxASurface **surface); > + mozilla::TemporaryRef<mozilla::gfx::SourceSurface> GetSurfaceSnapshot() > + { return NULL; } nsnull ::: content/canvas/src/nsCanvasRenderingContext2D.cpp @@ +373,5 @@ > const PRUnichar* aEncoderOptions, > nsIInputStream **aStream); > NS_IMETHOD GetThebesSurface(gfxASurface **surface); > + mozilla::TemporaryRef<mozilla::gfx::SourceSurface> GetSurfaceSnapshot() > + { return NULL; } nsnull ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +214,5 @@ > + return mType; > + } > + > + > + RefPtr<GradientStops> GetGradientStopsForTarget(DrawTarget *aRT) previous comment: "I suspect we should be returning TemporaryRef here." @@ +255,5 @@ > + > + > + /* nsIDOMCanvasGradient */ > + NS_IMETHOD AddColorStop (float offset, > + const nsAString& colorstr) Shouldn't this just be in the base class? It seems to be the same as the linear gradient version. @@ +568,5 @@ > + // Member vars > + PRInt32 mWidth, mHeight; > + > + // This is true when the canvas is valid, false otherwise. If the canvas > + // is invalid certain behavior is expected. Please document when the canvas is "invalid". @@ +625,5 @@ > + * builder present at the same time. There is also never a path and a > + * path builder present at the same time. When writing proceeds on an > + * existing path the Path is cleared and a new builder is created. > + */ > + RefPtr<Path> mPath; This is always a user-space path, right? Add a comment to say so. @@ +647,5 @@ > + ContextState& state = CurrentState(); > + > + // The spec says we should not draw shadows when the alpha value is 0, > + // or the operator isn't OVER. > + return state.op == OP_OVER && (state.shadowColor & 0xFF000000); NS_GET_A(state.shadowColor) != 0 was clearer, please restore it. @@ +657,5 @@ > + * > + * If dirtyRect is given, it will set this to the user-space dirty > + * rectangle of the draw operation. > + */ > + nsresult DrawPath(Style style, gfxRect *dirtyRect = nsnull); Remove this! @@ +791,5 @@ > + > + nscolor colorStyles[STYLE_MAX]; > + nsRefPtr<nsCanvasGradientAzure> gradientStyles[STYLE_MAX]; > + nsRefPtr<nsCanvasPatternAzure> patternStyles[STYLE_MAX]; > + nscolor shadowColor; Previous comment: "We should organize this so it packs nicely. I think that means all the pointers first, followed by the floats, Point and Matrix, followed by enums, followed by the PRPackedBool." @@ +828,5 @@ > + // This should only be called once or the mPattern destructor will > + // not be executed. > + NS_ASSERTION(!mPattern, "ForStyle() should only be called once on GeneralPattern!"); > + > + const nsCanvasRenderingContext2DAzure::ContextState state = aCtx->CurrentState(); You forgot the &, so there's still a bad copy here. @@ +1327,5 @@ > + > + mResetLayer = PR_TRUE; > + > + /* Create dummy surfaces here */ > + if (target == nsnull) previous comment: "if (!target) { Somewhere something needs to document when target could be null" @@ +2053,5 @@ > + bool doDrawShadow = NeedToDrawShadow(); > + > + const ContextState &state = CurrentState(); > + > + RefPtr<DrawTarget> target = mTarget; Previous comment: "Not used? Remove." @@ +2059,5 @@ > + if (state.patternStyles[STYLE_FILL]) { > + nsCanvasPatternAzure::RepeatMode repeat = > + state.patternStyles[STYLE_FILL]->mRepeat; > + // In the FillRect case repeat modes are easy to deal with. > + bool limitx = repeat == nsCanvasPatternAzure::NOREPEAT | repeat == nsCanvasPatternAzure::REPEATY; || @@ +2080,5 @@ > + if (x + w > patternSize.width) { > + w = patternSize.width - x; > + if (w < 0) { > + w = 0; > + } You didn't simplify it the way I suggested? @@ +3368,5 @@ > + > +NS_IMETHODIMP > +nsCanvasRenderingContext2DAzure::MozDrawText(const nsAString& textToDraw) > +{ > + return NS_OK; Actually, let's make these obsolete Moz methods throw NOT_SUPPORTED for better developer friendliness. @@ +4217,5 @@ > + *surface = tmpSurf.forget().get(); > + return NS_OK; > + } > + > + mTarget->Flush(); Actually the flush should be inside GetThebesSurfaceForDrawTarget, right?

Comment on attachment 541602 [details] [diff] [review] Part 1: Azure API and D2D backend v7 Review of attachment 541602 [details] [diff] [review]: ----------------------------------------------------------------- sr+ with that ::: gfx/2d/2D.h @@ +247,5 @@ > + RefPtr<GradientStops> mStops; > +}; > + > +/* > + * This stack-based class is used for Surface Patterns, they wrap a surface and Kill this last use of "stack-based" :-) @@ +266,5 @@ > + Filter mFilter; > +}; > + > +/* > + * This is the base class for SourceSurfaces. These objects are surfaces /SourceSurfaces/source surfaces/

Comment on attachment 541610 [details] [diff] [review] Part 6: Add Azure Canvas v7 Review of attachment 541610 [details] [diff] [review]: ----------------------------------------------------------------- Wildly optimistic r+ in the hope that you'll reorder the ContextState fields this time :-) ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +649,5 @@ > + { > + const ContextState& state = CurrentState(); > + > + // The spec says we should not draw shadows when the alpha value is 0, > + // or the operator isn't OVER. // The spec says we should not draw shadows if the operator isn't OVER. // If it's OVER and the alpha value is zero, we don't need to draw the shadow. @@ +785,5 @@ > + > + nscolor colorStyles[STYLE_MAX]; > + nsRefPtr<nsCanvasGradientAzure> gradientStyles[STYLE_MAX]; > + nsRefPtr<nsCanvasPatternAzure> patternStyles[STYLE_MAX]; > + nscolor shadowColor; You still haven't done this! Previous comment: "We should organize this so it packs nicely. I think that means all the pointers first, followed by the floats, Point and Matrix, followed by enums, followed by the PRPackedBool."

Benoit, let me know if this looks ok to you for the pref change :).

Landed on mozilla-inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/4ede291d2e4c http://hg.mozilla.org/integration/mozilla-inbound/rev/95039cfae6a3 http://hg.mozilla.org/integration/mozilla-inbound/rev/0f674a8c14a3 http://hg.mozilla.org/integration/mozilla-inbound/rev/8223367a233f http://hg.mozilla.org/integration/mozilla-inbound/rev/f1834d6ef78a http://hg.mozilla.org/integration/mozilla-inbound/rev/a8171812fe05 http://hg.mozilla.org/integration/mozilla-inbound/rev/fd95d3906e9c http://hg.mozilla.org/integration/mozilla-inbound/rev/308914e5f486 http://hg.mozilla.org/integration/mozilla-inbound/rev/c64155b1ece5 http://hg.mozilla.org/integration/mozilla-inbound/rev/7b236a5035a5 http://hg.mozilla.org/integration/mozilla-inbound/rev/46c3d095bc75

Follow-up fix to a typo pushed to mozilla-inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/aede8422cdc4

Latest m-inbound win32 build with this patch is crash city on IE demos speedreading fish-tank gave up after that. since its likely to not have symbols the crash-report will not be of use. http://crash-stats.mozilla.com/report/index/bp-cff1af3a-2a39-4fdd-a8b9-2109b2110624

(In reply to comment #104) > Latest m-inbound win32 build with this patch is crash city on IE demos > > speedreading > fish-tank > > gave up after that. > > since its likely to not have symbols the crash-report will not be of use. > > http://crash-stats.mozilla.com/report/index/bp-cff1af3a-2a39-4fdd-a8b9- > 2109b2110624 Likely due to the bug I pushed the followup for - verifying.

Confirmed this, builds from after my follow-up will have the issue fixed.

http://hg.mozilla.org/mozilla-central/rev/4ede291d2e4c http://hg.mozilla.org/mozilla-central/rev/95039cfae6a3 http://hg.mozilla.org/mozilla-central/rev/0f674a8c14a3 http://hg.mozilla.org/mozilla-central/rev/8223367a233f http://hg.mozilla.org/mozilla-central/rev/f1834d6ef78a http://hg.mozilla.org/mozilla-central/rev/a8171812fe05 http://hg.mozilla.org/mozilla-central/rev/fd95d3906e9c http://hg.mozilla.org/mozilla-central/rev/308914e5f486 http://hg.mozilla.org/mozilla-central/rev/c64155b1ece5 http://hg.mozilla.org/mozilla-central/rev/7b236a5035a5 http://hg.mozilla.org/mozilla-central/rev/46c3d095bc75

and the follow-up http://hg.mozilla.org/mozilla-central/rev/aede8422cdc4

Please file and fix followups for * removing gfx_{min,max} * adding license headers you missed (at least gfx/thebes/gfx2DGlue.h) * the Canvas 2D APIs that return NS_ERROR_DOM_NOT_SUPPORTED_ERR * AsyncDrawXULElement, which stopped throwing for untrusted callers * the broken handling of nsresults in NS_NewCanvasRenderingContext2D * the test_canvas.html failures that don't have a bug number annotation

(In reply to comment #109) > * the Canvas 2D APIs that return NS_ERROR_DOM_NOT_SUPPORTED_ERR We decided to stop supporting the Moz-only text APIs with Azure. As far as we know, no-one uses them.

I don't care if they're implemented or removed entirely. What landed makes those calls throws on one platform, and breaks feature detection. Whatever the decision (I assume the latter), it needs to be executed in full.

(In reply to comment #109) > Please file and fix followups for > > * removing gfx_{min,max} This is problematic for the reason listed there. I see no hurry to it. There's a bug filed for the reason that they're there (bug 666609). I'll add a reference in the comment. > * adding license headers you missed (at least gfx/thebes/gfx2DGlue.h) Good catch, should be easy to fix with a comment-only change patch. > * the Canvas 2D APIs that return NS_ERROR_DOM_NOT_SUPPORTED_ERR I'll leave to others to decide what to do with those. I'll execute whatever is decided. > * AsyncDrawXULElement, which stopped throwing for untrusted callers I need to figure out what uses that so I can just make it work, it isn't hard. But without any way to test it I wasn't going to just put code in there :). Filed 667192. > * the broken handling of nsresults in NS_NewCanvasRenderingContext2D You mean not checking the result for GetBool? The current documentation of getbool states 'The value is never modified when these methods fail' that means checking the result for failure is redundant (since the same behavior would occur as when !azure, which will be true in this case). > * the test_canvas.html failures that don't have a bug number annotation There's none (new ones at least) afaik, other than the radial gradient bug which has a bug, there were a bunch of passes added. See bug 666097.

In any case, having nsresult rv = Preferences::GetBool("gfx.canvas.azure.enabled", &azure); and then ignoring rv isn't the way to go. Please make it use PRBool azure = Preferences::GetBool("gfx.canvas.azure.enabled", PR_FALSE); instead, if you don't want to check for failure.

(In reply to comment #113) > In any case, having > > nsresult rv = Preferences::GetBool("gfx.canvas.azure.enabled", &azure); > > and then ignoring rv isn't the way to go. Please make it use > > PRBool azure = Preferences::GetBool("gfx.canvas.azure.enabled", PR_FALSE); > > instead, if you don't want to check for failure. Sounds good! I didn't know about that function signature existing. I'll adjust.

A testcase, please, for comparing canvas drawing performance before and after this bug landed.

(In reply to comment #112) > (In reply to comment #109) > > * AsyncDrawXULElement, which stopped throwing for untrusted callers > > I need to figure out what uses that so I can just make it work, it isn't > hard. But without any way to test it I wasn't going to just put code in > there :). Filed 667192. We should at least make it do if (!nsContentUtils::IsCallerTrustedForRead()) { // not permitted to use DrawWindow // XXX ERRMSG we need to report an error to developers here! (bug 329026) return NS_ERROR_DOM_SECURITY_ERR; } in the short term.

(In reply to comment #111) > I don't care if they're implemented or removed entirely. What landed makes > those calls throws on one platform, and breaks feature detection. Whatever > the decision (I assume the latter), it needs to be executed in full. You're right, we should remove them entirely, and should probably do it now.

Comment on attachment 541572 [details] [diff] [review] Part 2: Add glue code in gfx for thebes-azure interop v2 >+RefPtr<ScaledFont> >+gfxWindowsPlatform::GetScaledFontForFont(gfxFont *aFont) >+{ >+ if(mUseDirectWrite) { >+ gfxDWriteFont *font = static_cast<gfxDWriteFont*>(aFont); >+ >+ NativeFont nativeFont; >+ nativeFont.mType = NATIVE_FONT_DWRITE_FONT_FACE; >+ nativeFont.mFont = font->GetFontFace(); >+ RefPtr<ScaledFont> scaledFont = >+ mozilla::gfx::Factory::CreateScaledFontForNativeFont(nativeFont, font->GetAdjustedSize()); >+ >+ return scaledFont; >+ } >+} Not only does this fail to return anything if mUseDirectWrite is unset, it also doesn't compile with the Vista SDK...

(In reply to comment #118) > Comment on attachment 541572 [details] [diff] [review] [review] > Part 2: Add glue code in gfx for thebes-azure interop v2 > > >+RefPtr<ScaledFont> > >+gfxWindowsPlatform::GetScaledFontForFont(gfxFont *aFont) > >+{ > >+ if(mUseDirectWrite) { > >+ gfxDWriteFont *font = static_cast<gfxDWriteFont*>(aFont); > >+ > >+ NativeFont nativeFont; > >+ nativeFont.mType = NATIVE_FONT_DWRITE_FONT_FACE; > >+ nativeFont.mFont = font->GetFontFace(); > >+ RefPtr<ScaledFont> scaledFont = > >+ mozilla::gfx::Factory::CreateScaledFontForNativeFont(nativeFont, font->GetAdjustedSize()); > >+ > >+ return scaledFont; > >+ } > >+} > Not only does this fail to return anything if mUseDirectWrite is unset, it > also doesn't compile with the Vista SDK... Yeah, that should probably return NULL. And I should change the return type to TemporaryRef<T>. I didn't think we still supported the Vista SDK? Aren't we supposed to use the Win7 SDK these days?

I'm also getting some unresolved symbols: IID_IDXGISurface IID_ID2D1SimplifiedGeometrySink gfxD2DSurface::~gfxD2DSurface() gfxD2DSurface::GetTexture()

*_D2D_SURFACE are only defined if you are using the Windows 7 SDK, so they can replace the uses of XP_WIN. The only place I had to move things around was gfxWindowsPlatform, which I suspect is only compiled on Windows anyway...

Comment on attachment 543379 [details] [diff] [review] Fix compilation with Vista SDK [Moved to bug 717499] Review of attachment 543379 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/Factory.cpp @@ +40,5 @@ > #ifdef USE_CAIRO > #include "DrawTargetCairo.h" > #endif > > +#ifdef CAIRO_HAS_D2D_SURFACE This is a bit of an issue since this isn't present in the Azure stand-alone build (and shouldn't be named that way). We should rename this #define to USE_DIRECT2D or something and then define that before including this file in gfxPlatform inside #ifdef CAIRO_HAS_D2D_SURFACE.

Landed http://hg.mozilla.org/mozilla-central/rev/df68fdf2f727 to address my comments in time for Firefox 7, with pending-r=roc based on your comments.

(In reply to comment #123) > Landed > http://hg.mozilla.org/mozilla-central/rev/df68fdf2f727 > to address my comments in time for Firefox 7, with pending-r=roc based on > your comments. Better commit messages help a lot when looking back at changes.

Those changes look good, thanks.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a2) Gecko/20110706 Firefox/7.0a2 The results of the implementation of the Azure API are visible on the latest Aurora 7 merged from Nightly. The landing of this bug is detailed mainly by the changesets in comment 107

Christoph is going to take a run at fuzzing this with his canvas fuzzer.

https://developer.mozilla.org/en/Firefox_7_for_developers#Canvas https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D