[Azure] [Meta] Cairo backend for new 2D API

NEW
Unassigned

Status

()

Core
Graphics
P3
normal
6 years ago
a month ago

People

(Reporter: Joe Drew (not getting mail), Unassigned)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

6 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
(Reporter)

Comment 1

6 years ago
Created attachment 579173 [details] [diff] [review]
implement significantly more of the cairo backend

This implements significantly more of the API. It's probably ready for review.
Attachment #579173 - Flags: review?(jmuizelaar)
(Reporter)

Updated

6 years ago
Depends on: 707848
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()
Attachment #579173 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

6 years ago
Depends on: 715513
(Reporter)

Comment 3

6 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.
(Reporter)

Comment 4

6 years ago
I'm morphing this into a metabug.
Summary: [Azure] Cairo backend for new 2D API → [Azure] [Meta] Cairo backend for new 2D API
(Reporter)

Updated

6 years ago
Attachment #579173 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Depends on: 715652
(Reporter)

Updated

6 years ago
Depends on: 719627
(Reporter)

Updated

6 years ago
Depends on: 719631
(Reporter)

Updated

6 years ago
Depends on: 722217
Blocks: 734668
Joe, what work remains to be done here?
(Reporter)

Comment 6

6 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.

Updated

6 years ago
Depends on: 747822

Updated

6 years ago
Blocks: 748116

Updated

6 years ago
Depends on: 753237

Updated

6 years ago
Depends on: 760349
Depends on: 761895

Updated

5 years ago
Blocks: 764108

Updated

5 years ago
Depends on: 764125
(Reporter)

Updated

4 years ago
Assignee: joe → nobody
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.