Last Comment Bug 716802 - [Skia] Implement new Azure API functions in Skia backend
: [Skia] Implement new Azure API functions in Skia backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on:
Blocks: 687187
  Show dependency treegraph
 
Reported: 2012-01-09 21:55 PST by Matt Woodrow (:mattwoodrow)
Modified: 2012-01-19 10:49 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implementations (6.66 KB, patch)
2012-01-09 21:55 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review
Implementations v2 (15.75 KB, patch)
2012-01-11 14:11 PST, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review
Implementations v3 (14.49 KB, patch)
2012-01-18 13:46 PST, Matt Woodrow (:mattwoodrow)
gwright: review+
Details | Diff | Review

Description Matt Woodrow (:mattwoodrow) 2012-01-09 21:55:02 PST
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.
Comment 1 Matt Woodrow (:mattwoodrow) 2012-01-11 14:11:06 PST
Created attachment 587818 [details] [diff] [review]
Implementations v2

Included missing parts so that it actually compiles.
Comment 2 George Wright (:gw280) (:gwright) 2012-01-18 00:36:38 PST
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) 2012-01-18 13:46:50 PST
Created attachment 589630 [details] [diff] [review]
Implementations v3

Fixed review comments
Comment 4 Matt Woodrow (:mattwoodrow) 2012-01-18 13:48:19 PST
(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) 2012-01-18 20:50:24 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e190ab5f2240
Comment 6 Matt Brubeck (:mbrubeck) 2012-01-19 10:49:05 PST
https://hg.mozilla.org/mozilla-central/rev/e190ab5f2240

Note You need to log in before you can comment on or make changes to this bug.