716802 - [Skia] Implement new Azure API functions in Skia backend

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Implementations

Azure content rendering added some new 2D API functions, this implements them for Skia.

Feel free to defer the review to jeff, he loves reviewing my patches.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Implementations v2

Included missing parts so that it actually compiles.

Comment 2

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 587818 [details] [diff] [review]
Implementations v2


>+  if (aPattern.GetType() == PATTERN_COLOR) {
>+  } else if (aPattern.GetType() == PATTERN_LINEAR_GRADIENT) {
>+  } else if (aPattern.GetType() == PATTERN_RADIAL_GRADIENT) {
>+  } else {
>+  }

I think this entire if-elseif-else block would be better as a switch statement.

>+  SkLayerRasterizer *raster = new SkLayerRasterizer();
>+  raster->addLayer(maskPaint, 0, 0);

I don't think you need to call the addLayer that explicitly sets dx or dy here, as the addLayer(SkPaint) function should just do the right thing.

>+  SkSafeUnref(paint.mPaint.setRasterizer(raster));
>+
>+  // Skia only uses the mask rasterizer when we are drawing a path/rect.
>+  // Take our destination bounds and convert them into user space to use
>+  // as the path to draw.
>+  SkPath path;
>+  Rect rect(0, 0, mSize.width, mSize.height);
>+  path.addRect(RectToSkRect(rect));

Why do you need to create a Rect then convert it to an SkRect? Can you just create an SkRect and use that? I think SkIRect::MakeXYWH would do what you want.

>   const SkBitmap& bitmap = static_cast<SourceSurfaceSkia*>(aSurface)->GetBitmap();
>+  SkBitmap temp;
>+  SkIRect source = IntRectToSkIRect(aSourceRect);
>+  bitmap.extractSubset(&temp, source);

What pixel format do we use here? Are we definitely in all cases not doing an additional deep copy here? If not then maybe a comment stating so would be good as the API method name doesn't make this obvious :)

Worse yet, is this going to cause us to do a read pixels when we eventually move towards GPU-backed Skia?

Apart from that, LGTM.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Implementations v3

Fixed review comments

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to George Wright (:gw280) from comment #2)
> I think this entire if-elseif-else block would be better as a switch
> statement.

> 
> I don't think you need to call the addLayer that explicitly sets dx or dy
> here, as the addLayer(SkPaint) function should just do the right thing.
> 

> Why do you need to create a Rect then convert it to an SkRect? Can you just
> create an SkRect and use that? I think SkIRect::MakeXYWH would do what you
> want.

Done.

> What pixel format do we use here? Are we definitely in all cases not doing
> an additional deep copy here? If not then maybe a comment stating so would
> be good as the API method name doesn't make this obvious :)
> 
> Worse yet, is this going to cause us to do a read pixels when we eventually
> move towards GPU-backed Skia?
> 
> Apart from that, LGTM.

Turns out that this actually caused a bug (drawSprite doesn't ignore the current clip), so I've removed this hunk entirely.

Comment 5

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e190ab5f2240

Comment 6

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e190ab5f2240