[Skia] Implement new Azure API functions in Skia backend

RESOLVED FIXED in mozilla12

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 587245 [details] [diff] [review]
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.
Attachment #587245 - Flags: review?(gwright)
(Assignee)

Comment 1

6 years ago
Created attachment 587818 [details] [diff] [review]
Implementations v2

Included missing parts so that it actually compiles.
Attachment #587245 - Attachment is obsolete: true
Attachment #587245 - Flags: review?(gwright)
Attachment #587818 - Flags: review?(gwright)
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.
(Assignee)

Comment 3

6 years ago
Created attachment 589630 [details] [diff] [review]
Implementations v3

Fixed review comments
Attachment #587818 - Attachment is obsolete: true
Attachment #587818 - Flags: review?(gwright)
Attachment #589630 - Flags: review?(gwright)
(Assignee)

Comment 4

6 years ago
(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.
Attachment #589630 - Flags: review?(gwright) → review+
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e190ab5f2240
https://hg.mozilla.org/mozilla-central/rev/e190ab5f2240
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.