Closed Bug 688367 Opened 8 years ago Closed 8 years ago

[Azure] Implement Skia backend for Azure

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Implement Skia backend (obsolete) — Splinter Review
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.
Attachment #561658 - Flags: review?(jmuizelaar)
Attachment #561658 - Flags: review?(bas.schouten)
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 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.
Attached patch Implement Skia backend v2 (obsolete) — Splinter Review
Fixed initial review comments, added android support
Attachment #561658 - Attachment is obsolete: true
Attachment #561658 - Flags: review?(jmuizelaar)
Attachment #561658 - Flags: review?(bas.schouten)
Attachment #562335 - Flags: review?(jmuizelaar)
Attached patch Implement Skia backend v3 (obsolete) — Splinter Review
Fixed a large collection of memory leaks.
Attachment #562335 - Attachment is obsolete: true
Attachment #562335 - Flags: review?(jmuizelaar)
Attachment #562340 - Flags: review?(jmuizelaar)
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 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
Attachment #562340 - Flags: review?(jmuizelaar) → review+
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 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
(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.
Attachment #562340 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/58e6c346c72a
Assignee: nobody → matt.woodrow
Whiteboard: [inbound]
Is there a pref to try this yet? Or is it just prep work?
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 :)
Apologies for bugspam, but is there an ETA for it working in windows builds?
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.
https://hg.mozilla.org/mozilla-central/rev/58e6c346c72a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.