Last Comment Bug 688367 - [Azure] Implement Skia backend for Azure
: [Azure] Implement Skia backend for Azure
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 2 votes (vote)
: mozilla10
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on:
Blocks: 687187
  Show dependency treegraph
 
Reported: 2011-09-21 22:30 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2013-01-11 11:43 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement Skia backend (65.04 KB, patch)
2011-09-21 22:30 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Implement Skia backend v2 (78.02 KB, patch)
2011-09-25 16:16 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Implement Skia backend v3 (78.02 KB, patch)
2011-09-25 17:00 PDT, Matt Woodrow (:mattwoodrow)
jmuizelaar: review+
Details | Diff | Splinter Review
Add Skia backend to Azure (76.71 KB, patch)
2011-10-31 20:13 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2011-09-21 22:30:22 PDT
Created attachment 561658 [details] [diff] [review]
Implement Skia backend

The main patch for adding Skia support to Azure.

This probably needs a bit more work before we can land it, asking for review now to try parallelize the process a little.
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-09-23 15:08:13 PDT
Comment on attachment 561658 [details] [diff] [review]
Implement Skia backend


>+bool 
>+SourceSurfaceSkia::InitFromData(unsigned char* aData,
>+                                const IntSize &aSize,
>+                                int32_t aStride,
>+                                SurfaceFormat aFormat)
>+{
>+  mBitmap.setConfig(GfxFormatToSkiaConfig(aFormat), aSize.width, aSize.height, aStride);
>+  if (!mBitmap.allocPixels()) {
>+    return false;
>+  }
>+  //TODO: Is this a Skia bug? The docs says param2 is a height (in pixels), but the
>+  // implementation is using it as a size
>+  if (!mBitmap.copyPixelsFrom(aData, aSize.height*aStride, aStride)) {

we can use mBitmap.getSafeSize() or mBitmap.getSize() instead of aSize.height*aStride.

Also, it might be worth looking where if anywhere we check for overflow.
Comment 2 Jeff Muizelaar [:jrmuizel] 2011-09-23 17:43:03 PDT
Comment on attachment 561658 [details] [diff] [review]
Implement Skia backend


>+namespace mozilla {
>+namespace gfx {
>+
>+SkColor ColorToSkColor(const Color &color, Float aAlpha)
>+{
>+  //XXX: do a better job converting to int
>+  //XXXmattwoodrow: The r & b components are swapped because SkPostConfig.h is wrong. We need to fix this

I assume you want to fix this before landing?


>+class GradientStopsSkia : public GradientStops
>+{
>+public:
>+  GradientStopsSkia(const std::vector<GradientStop>& aStops, uint32_t aNumStops)
>+    : mCount(aNumStops)
>+  {
>+    if (mCount == 0) {
>+      return;
>+    }

There's some trickyness going on this function that I haven't put the energy to figure out. Please add a comment about why.

>+    uint32_t shift = 0;
>+    if (aStops[0].offset != 0) {
>+      mCount++;
>+      shift = 1;
>+    }
>+    if (aStops[aNumStops-1].offset != 1) {
>+      mCount++;
>+    }
>+    mColors.resize(mCount);
>+    mPositions.resize(mCount);
>+    if (aStops[0].offset != 0) {
>+      mColors[0] = ColorToSkColor(aStops[0].color, 1.0);
>+      mPositions[0] = 0;
>+    }
>+    for (uint32_t i = 0; i < aNumStops; i++) {
>+      mColors[i + shift] = ColorToSkColor(aStops[i].color, 1.0);
>+      mPositions[i + shift] = aStops[i].offset;
>+    }
>+    if (aStops[aNumStops-1].offset != 1) {
>+      mColors[mCount-1] = ColorToSkColor(aStops[aNumStops-1].color, 1.0);
>+      mPositions[mCount-1] = 1;

SK_Scalar1 instead of 1. Maybe also use SkFloatToScalar to convert from floats.

We should figure out if Android is using float or fixed scalars. To know how much we should care about this.
Comment 3 Matt Woodrow (:mattwoodrow) 2011-09-25 16:16:17 PDT
Created attachment 562335 [details] [diff] [review]
Implement Skia backend v2

Fixed initial review comments, added android support
Comment 4 Matt Woodrow (:mattwoodrow) 2011-09-25 17:00:22 PDT
Created attachment 562340 [details] [diff] [review]
Implement Skia backend v3

Fixed a large collection of memory leaks.
Comment 5 Jeff Muizelaar [:jrmuizel] 2011-09-29 21:36:46 PDT
Comment on attachment 562340 [details] [diff] [review]
Implement Skia backend v3


>+
>+Rect
>+PathSkia::GetStrokedBounds(const StrokeOptions &aStrokeOptions,
>+                           const Matrix &aTransform) const
>+{
>+  //TODO: No idea.

Webkit uses getFillPath for this kind of thing. It looks like you could do that or use an SkStroker directly.
Comment 6 Jeff Muizelaar [:jrmuizel] 2011-10-03 14:58:47 PDT
Comment on attachment 562340 [details] [diff] [review]
Implement Skia backend v3

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

I didn't do a super thorough review because it will be easy to iterate on this in tree. Also, it might be worth converting some of the XXX/TODO's into asserts so that we don't end up having to rediscover intentional bugs.

::: gfx/2d/DrawTargetSkia.cpp
@@ +255,5 @@
> +  {
> +    mPaint.setXfermodeMode(GfxOpToSkiaOp(aOptions.mCompositionOp));
> +    mCanvas = aCanvas;
> +
> +    //TODO: Can we set greyscale somehow?

What does this comment mean?

@@ +384,5 @@
> +  if (aSource.IsEmpty()) {
> +    return;
> +  }
> +
> +  //TODO: DrawSurfaceOptions!

This is needed for -moz-crisp-edges. Do you pass those tests?

::: gfx/2d/Logging.h
@@ +91,1 @@
>    }

This change slipped in

::: gfx/2d/PathSkia.cpp
@@ +117,5 @@
> +void
> +PathBuilderSkia::Arc(const Point &aOrigin, float aRadius, float aStartAngle,
> +                     float aEndAngle, bool aAntiClockwise)
> +{ 
> +  //TODO: Taken directly from azure canvas, we should share this somewhere

Yes please.

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +84,5 @@
> +    return false;
> +  }
> +  //TODO: Is this a Skia bug? The docs says param2 is a height (in pixels), but the
> +  // implementation is using it as a size
> +  if (!mBitmap.copyPixelsFrom(aData, mBitmap.getSafeSize(), aStride)) {

It is a doc bug
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-10-05 19:23:48 PDT
Comment on attachment 562340 [details] [diff] [review]
Implement Skia backend v3


>+
>+void
>+PathBuilderSkia::LineTo(const Point &aPoint)
>+{
>+  if (!mPath.countPoints()) {
>+    MoveTo(aPoint);
>+  }
>+  mPath.lineTo(SkFloatToScalar(aPoint.x), SkFloatToScalar(aPoint.y));
>+}

there should be an else before the lineTo, otherwise you're making a needless degenerate segment.
Comment 8 Jeff Muizelaar [:jrmuizel] 2011-10-06 21:14:51 PDT
Comment on attachment 562340 [details] [diff] [review]
Implement Skia backend v3


>+
>+TemporaryRef<DataSourceSurface>
>+SourceSurfaceSkia::GetDataSurface()
>+{
>+  RefPtr<DataSourceSurface> surf;
>+  surf = this;
>+  return surf;
>+}

The default implementation of GetDataSurface() should take care of this.


>+  bool InitFromData(unsigned char* aData,
>+                    const IntSize &aSize,
>+                    int32_t aStride,
>+                    SurfaceFormat aFormat);
>+
>+  bool InitWithBitmap(SkCanvas* aBitmap,
>+                      SurfaceFormat aFormat);
>+
>+
>+  unsigned char *GetData();
>+
>+  int32_t Stride() { return mStride; }

GetData and Stride should have the 'virtual' keyword infront
Comment 9 Matt Woodrow (:mattwoodrow) 2011-10-30 17:32:06 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)

> > +
> > +    //TODO: Can we set greyscale somehow?
> 
> What does this comment mean?

The 2D.h API differentiates between AA_GREY and AA_SUBPIXEL, and skia is just using a boolean.

> 
> @@ +384,5 @@
> > +  if (aSource.IsEmpty()) {
> > +    return;
> > +  }
> > +
> > +  //TODO: DrawSurfaceOptions!
> 
> This is needed for -moz-crisp-edges. Do you pass those tests?

Unsure, will check.

> 
> ::: gfx/2d/PathSkia.cpp
> @@ +117,5 @@
> > +void
> > +PathBuilderSkia::Arc(const Point &aOrigin, float aRadius, float aStartAngle,
> > +                     float aEndAngle, bool aAntiClockwise)
> > +{ 
> > +  //TODO: Taken directly from azure canvas, we should share this somewhere
> 
> Yes please.

I'll do this in a separate bug.
Comment 10 Matt Woodrow (:mattwoodrow) 2011-10-31 20:13:08 PDT
Created attachment 570915 [details] [diff] [review]
Add Skia backend to Azure
Comment 11 Matt Woodrow (:mattwoodrow) 2011-11-02 12:56:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/58e6c346c72a
Comment 12 Leman Bennett [Omega] 2011-11-02 14:29:55 PDT
Is there a pref to try this yet? Or is it just prep work?
Comment 13 Matt Woodrow (:mattwoodrow) 2011-11-02 14:39:30 PDT
It's only working on mac currently, but you can try it there using 'gfx.canvas.azure.enabled=true'.

Should be in tonight's, or tomorrow night's nightly :)
Comment 14 DB Cooper 2011-11-02 15:11:01 PDT
Apologies for bugspam, but is there an ETA for it working in windows builds?
Comment 15 Matt Woodrow (:mattwoodrow) 2011-11-02 15:38:53 PDT
I haven't started on this yet.

I've just filed bug 699258 as a mentored bug, in case anybody is interested in tackling this themselves. Failing that, I should get to it sometime in the next few weeks.
Comment 16 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-11-03 08:44:38 PDT
https://hg.mozilla.org/mozilla-central/rev/58e6c346c72a

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