298880 - thebes API updates/suggestions

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I've attached a patch that has some suggestions for the public Thebes APIs; the
main changes/points are:

* Consistent ref/ptr usage.  I think we should always use pointers to contexts,
surfaces, patterns, and other similar "heavy" classes; converseley, we should
always use refs for lightweight types (points, sizes, rects, etc.).  Const refs
should be used where appropriate instead of passing points/etc. by value.

* gfxMatrix suggestion (unimplemented): Should use 'double' directly and call it
gfxDoubleMatrix, and then typedef it to gfxMatrix in the same place we typedef
double to gfxFloat.  That way if someone wants to use a different gfxFloat, they
can implement a new matrix class which may be more natural for that other type.

* changed gfxRect to contain a gfxPoint and a gfxSize instead of 4 gfxFloats;
this will lead to more natural expressions involving rects and their components
(e.g. someRect.pos += somePoint; instead of someRect.x += somePoint.x;
someRect.y += somePoint.y;).

* Added some more Context APIs:

- Suggestion of a SaveMatrix()/RestoreMatrix() API that does the same as
Save()/Restre(), but just with the CTM.  The stack can be owned by the context.
 This can fix the XXX for needing a fast way to save/restore the current
transformation as the frame hierarchy is traversed mentioned above Save().

- Defined Stroke()/Fill()/Clip() to not consume the current path (cairo's
stroke_preserve()/fill_preserve()/clip_preserve() methods).  Calling NewPath()
is a lot clearer than ending up with an implicit new path.

- Changed SetColor and SetPattern to a single SetSource taking a color, pattern,
or surface.  This more closely indicates that you can only have one source set;
e.g. calling SetColor and then calling SetPattern cancels the color.

- Added Paint() method for painting surfaces

- Added PixelSnappedRectangle() method

- Added DeviceToUser/UserToDevice for points and sizes

Thoughts?

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: suggested patch to thebes API

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated revision

Updated after discussion with pav; got rid of my SaveMatrix/RestoreMatrix idea,
because cairo's Save/Restore is pretty fast.  Changed overloaded SetSource()
back to SetColor and SetPattern, added SetSource for surfaces.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> cairo's Save/Restore is pretty fast

Are you sure?

-private:
     cairo_surface_t* mSurface;

Is this really wise?

+    void Paint(gfxASurface *surface, gfxFloat alpha = 1.0);

Painting is not a path operation (right?) so it should be moved. Why is the
surface parameter here?

+    void Polygon(const gfxPoint *points, unsigned long numPoints);

Ditch Polygon, I reckon.

+    void DeviceToUser(gfxPoint& point) const;
+    void DeviceToUser(gfxSize& size) const;
+    void UserToDevice(gfxPoint& point) const;
+    void UserToDevice(gfxSize& size) const;

Why not make these functions that return a result?

+    void SetSource(gfxASurface* surface, const gfxPoint& origin);

What does the origin parameter do?

(In reply to comment #0)
> * Consistent ref/ptr usage.  I think we should always use pointers to
> contexts, surfaces, patterns, and other similar "heavy" classes; converseley,
> we should always use refs for lightweight types (points, sizes, rects, etc.).

Good

>  Const refs should be used where appropriate instead of passing points/etc. by
> value.

It may be more efficient to pass points by value, especially on 64-bit machines
using SSE2. But it doesn't matter too much because we can change that without
changing callers.

> * gfxMatrix suggestion (unimplemented): Should use 'double' directly and call
> it gfxDoubleMatrix, and then typedef it to gfxMatrix in the same place we
> typedef double to gfxFloat.  That way if someone wants to use a different
> gfxFloat, they can implement a new matrix class which may be more natural for
> that other type.

I'm not sure why you'd want to do that...

> * changed gfxRect to contain a gfxPoint and a gfxSize instead of 4 gfxFloats;
> this will lead to more natural expressions involving rects and their
> components (e.g. someRect.pos += somePoint; instead of someRect.x +=
> somePoint.x; someRect.y += somePoint.y;).

I'm not sold on this but OK.

> - Defined Stroke()/Fill()/Clip() to not consume the current path (cairo's
> stroke_preserve()/fill_preserve()/clip_preserve() methods).  Calling NewPath()
> is a lot clearer than ending up with an implicit new path.
> 
> - Added Paint() method for painting surfaces

All good

> - Added PixelSnappedRectangle() method

It's been replaced by a parameter now, I guess, which is fine. BUT you need to
define what it does when the transformed rectangle is not aligned with the axes.
In my current snapping code, I then decline to snap the rectangle at all. Does
that sound like the right thing?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #3)
> > cairo's Save/Restore is pretty fast
> 
> Are you sure?

From looking at the code, it should be; I was thinking of a lightweight matrix
stack that we could use instead, but pav had me look at the code.  It just
allocates a constant-size gstate, does a memcpy, and then refs the existing
surfaces/patterns/etc.  If for some reason it's a bottleneck, we can have it
cache unused gstates, but I can't imagine that it will be.

> -private:
>      cairo_surface_t* mSurface;
> 
> Is this really wise?

I think so; it makes it protected instead of private, so that concrete surface
classes can get at mSurface directly instead of needing to go through
CairoSurface().

> +    void Paint(gfxASurface *surface, gfxFloat alpha = 1.0);
> 
> Painting is not a path operation (right?) so it should be moved. Why is the
> surface parameter here?

Yeah, I wasn't sure where to stick it; I guess it's really its own beast. 
Ignore the surface parameter, not sure what I was thinking.

> +    void DeviceToUser(gfxPoint& point) const;
> +    void DeviceToUser(gfxSize& size) const;
> +    void UserToDevice(gfxPoint& point) const;
> +    void UserToDevice(gfxSize& size) const;
> 
> Why not make these functions that return a result?

Will do.

> +    void SetSource(gfxASurface* surface, const gfxPoint& origin);
> 
> What does the origin parameter do?

It sets the origin inside the surface which should be drawn at the current
origin.  (In cairo's set_source_surface, it sets up a transform of -origin on
the pattern matrix.)

> >  Const refs should be used where appropriate instead of passing points/etc. by
> > value.
> 
> It may be more efficient to pass points by value, especially on 64-bit machines
> using SSE2. But it doesn't matter too much because we can change that without
> changing callers.

Good point; I picked refs for consistency with other usage, but as you say we
can just change everything over to passing by value.

> > * gfxMatrix suggestion (unimplemented): Should use 'double' directly and call
> > it gfxDoubleMatrix, and then typedef it to gfxMatrix in the same place we
> > typedef double to gfxFloat.  That way if someone wants to use a different
> > gfxFloat, they can implement a new matrix class which may be more natural for
> > that other type.
> 
> I'm not sure why you'd want to do that...

Well, it's a bit of a moot point since we're not likely to do much with matrices
other than interact with cairo.  My thinking was that if someone wanted to use
'float' or even some fixed point type for gfxFloat, they would want to
reimplement gfxMatrix to be the most efficient for their particular type
(instead of using the cairo matrix internally, which is doubles).

> 
> > - Added PixelSnappedRectangle() method
> 
> It's been replaced by a parameter now, I guess, which is fine. BUT you need to
> define what it does when the transformed rectangle is not aligned with the axes.
> In my current snapping code, I then decline to snap the rectangle at all. Does
> that sound like the right thing?

I think that sounds right; I'm a little unsure right now about when we want to
use a snapped rectangle and when not... I think with most rendering right now we
want to, but that may give us strange results when there's a scaling factor in
the transformation matrix.  Maybe we also only want to snap only if we have an
x/y scale of 1.0?

New patch coming shortly.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v3 patch

Updated with comments

Comment 6

[Stuart Parmenter]

Attachment: All thebes changes.

This patch merges roc's changes from 298637 with vlad's patches from this bug
along with my own patches.

I've added refcounting to gfxContext and gfxASurface as well as gone back to
passing by value for gfxPoint,gfxSize,gfxRect.	I've removed some comments that
no longer apply and added a few new ones.

Fixed a few problems that didn't build on windows.

I'm going to go ahead and check this in so that we aren't all passing around
megapatches anymore.

Comment 7

[Stuart Parmenter]

Comment on attachment 187495 [details] [diff] [review]
All thebes changes.

I checked this in along with a followup patch to make refcounting a little less
silly than whats in this patch.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> It sets the origin inside the surface which should be drawn at the current
> origin.  (In cairo's set_source_surface, it sets up a transform of -origin on
> the pattern matrix.)

Why not provide a general way to set the pattern matrix? We should allow the
pattern matrix to be non-affine.

All the other comments sound good.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #8)
> > It sets the origin inside the surface which should be drawn at the current
> > origin.  (In cairo's set_source_surface, it sets up a transform of -origin on
> > the pattern matrix.)
> 
> Why not provide a general way to set the pattern matrix? We should allow the
> pattern matrix to be non-affine.

We do, through gfxPattern::SetMatrix.  SetSource() is really just a convenience
wrapper around cairo_set_source_surface(), which is just a wrapper for
pattern_create_for_surface().  I think the most common use will be SetSource
with an origin of 0,0; the next common use will probably be one with an offset;
and the for the fully general case, you'd have to go through SetPattern.  Does
that sound sane?  We can rename SetSource() to make it more clear that it's just
a convenience wrapper around SetPattern/set_source_pattern..

Comment 10

[Stuart Parmenter]

closing out old bug