Joe Drew (not getting mail) Reporter
	Description • 13 years ago

We need a Cairo backend for the new 2D API. It's currently in development in the cairo branch of the graphics branch: http://hg.mozilla.org/projects/graphics

	Joe Drew (not getting mail) Reporter
	Comment 1 • 13 years ago

This implements significantly more of the API. It's probably ready for review.

	Jeff Muizelaar [:jrmuizel]
	Comment 2 • 13 years ago

Comment on attachment 579173 [details] [diff] [review]
implement significantly more of the cairo backend

Review of attachment 579173 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly seems reasonable.

::: ../../../mozilla-central/gfx/2d/DrawTargetCairo.cpp
@@ +247,5 @@
>  }
>  
> +cairo_pattern_t*
> +GfxPatternToCairoPattern(const Pattern& aPattern, Float aAlpha)
> +{

I'd rather this function not return NULL. That keeps the caller from needing to check for NULL

@@ +265,5 @@
> +      const SurfacePattern& pattern = static_cast<const SurfacePattern&>(aPattern);
> +      cairo_surface_t* surf = NULL;
> +
> +      if (pattern.mSurface->GetType() == SURFACE_CAIRO) {
> +        const SourceSurfaceCairo* sourcesurf = static_cast<const SourceSurfaceCairo*>(pattern.mSurface.get());

sourcesurf is not a great name.

@@ +267,5 @@
> +
> +      if (pattern.mSurface->GetType() == SURFACE_CAIRO) {
> +        const SourceSurfaceCairo* sourcesurf = static_cast<const SourceSurfaceCairo*>(pattern.mSurface.get());
> +        surf = sourcesurf->GetSurface();
> +        cairo_surface_reference(surf);

might be worth adding a comment that we're referencing this to match up with image_surface_create()

@@ +279,5 @@
> +        surf = cairo_image_surface_create_for_data(sourcesurf->GetData(),
> +                                                   GfxFormatToCairoFormat(sourcesurf->GetFormat()),
> +                                                   sourcesurf->GetSize().width,
> +                                                   sourcesurf->GetSize().height,
> +                                                   sourcesurf->Stride());

It looks like this cairo_image_surface is holding an implicit reference to the DataSourceSurface. I feel like it would be better if this was explicit, or at least documented.

@@ +294,5 @@
> +    case PATTERN_LINEAR_GRADIENT:
> +    {
> +      const LinearGradientPattern& pattern = static_cast<const LinearGradientPattern&>(aPattern);
> +      RefPtr<GradientStops> stops = pattern.mStops;
> +      if (stops->GetBackendType() == BACKEND_CAIRO) {

We should perhaps assert this condition instead of silently ignoring non cairo ones.

@@ +302,5 @@
> +        const std::vector<GradientStop>& stops =
> +          static_cast<GradientStopsCairo*>(pattern.mStops.get())->GetStops();
> +        for (std::vector<GradientStop>::const_iterator i = stops.begin();
> +             i != stops.end();
> +             ++i) {

This loop would probably easier to read if you just used an index instead of an iterator. afaik that's what the other backends do.

@@ +462,5 @@
> +
> +  SourceSurfaceCairo* sourcesurf = static_cast<SourceSurfaceCairo*>(aSurface);
> +
> +  Float width = aSurface->GetSize().width,
> +        height = aSurface->GetSize().height;

not having Float here surprised me.

@@ +508,5 @@
> +
> +  cairo_identity_matrix(mContext);
> +  cairo_translate(mContext, aDest.x, aDest.y);
> +
> +  cairo_mask_surface(mContext, blursurf, aOffset.x, aOffset.y);

blursurf is leaked

@@ +540,5 @@
> +    std::vector<double> dashes(aStrokeOptions.mDashLength);
> +    for (size_t i = 0; i < aStrokeOptions.mDashLength; ++i) {
> +      dashes[i] = aStrokeOptions.mDashPattern[i];
> +    }
> +    cairo_set_dash(aCtx, &dashes[0], aStrokeOptions.mDashLength,

other backends use &dashes.begin() or something like that. Both are crappy.

@@ +565,5 @@
> +  if (needIntermediate) {
> +    cairo_push_group_with_content(mContext, CAIRO_CONTENT_COLOR_ALPHA);
> +
> +    // Don't want operators to be applied twice
> +    cairo_set_operator(mContext, CAIRO_OPERATOR_SOURCE);

It might be better to use CAIRO_OPERATOR_OVER here for improved performance.

@@ +588,5 @@
> +        cairo_fill(mContext);
> +      } else {
> +        cairo_clip(mContext);
> +        cairo_paint(mContext);
> +      }

Aren't fill() and clip()/paint() the same?

@@ +618,5 @@
> +DrawTargetCairo::CopySurface(SourceSurface *aSurface,
> +                             const IntRect &aSourceRect,
> +                             const IntPoint &aDestination)
> +{
> +  PrepareForDrawing(mContext);

This needs an implementation

@@ +677,2 @@
>    }
>  

It doesn't seem like this handles global alpha properly.

::: ../../../mozilla-central/gfx/2d/SourceSurfaceCairo.cpp
@@ +136,5 @@
> +    cairo_t* ctx = cairo_create(imageSurf);
> +    cairo_pattern_t* pat = cairo_pattern_create_for_surface(mSurface);
> +    cairo_set_source(ctx, pat);
> +    cairo_paint(ctx);
> +    cairo_destroy(ctx);

you leak the pattern and could probably just use cairo_set_source_surface()

	Joe Drew (not getting mail) Reporter
	Comment 3 • 13 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)

> @@ +677,2 @@
> >    }
> >  
> 
> It doesn't seem like this handles global alpha properly.

This *should* be handled properly after the path patch, which simplifies the handling of patterns significantly.

	Joe Drew (not getting mail) Reporter
	Comment 4 • 13 years ago

I'm morphing this into a metabug.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 12 years ago

Joe, what work remains to be done here?

	Joe Drew (not getting mail) Reporter
	Comment 6 • 12 years ago

The rest of the gfx::DrawTarget API needs to be implemented (not much left), and then we need to fix all reftest failures. Probably ~2-3 weeks of work.