[Azure] Implement Skia backend for Azure

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Attachment #561658 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 3

6 years ago
Created attachment 562335 [details] [diff] [review]
Implement Skia backend v2

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)
(Assignee)

Comment 4

6 years ago
Created attachment 562340 [details] [diff] [review]
Implement Skia backend v3

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
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
Created attachment 570915 [details] [diff] [review]
Add Skia backend to Azure
(Assignee)

Updated

6 years ago
Attachment #562340 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
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?
(Assignee)

Comment 13

6 years ago
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

6 years ago
Apologies for bugspam, but is there an ETA for it working in windows builds?
(Assignee)

Comment 15

6 years ago
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
Last Resolved: 6 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.