848482 - [SkiaGL] SourceSurface doesn't currently work with SkiaGL

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[George Wright (:gw280) (needinfo me!)]

Our SourceSurface implementation in the Azure Skia backend doesn't handle SkiaGL backed DrawTargets properly.

Comment 1

[George Wright (:gw280) (needinfo me!)]

Attachment: patch v1

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 728039 [details] [diff] [review]
patch v1

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +101,5 @@
>    RefPtr<SourceSurfaceSkia> source = new SourceSurfaceSkia();
> +
> +  bool success = false;
> +
> +  if (!mBitmap.getPixels()) {

Can we instead create two subclasses, DrawTargetSkiaBitmap and DrawTargetSkiaGPU (or equivalent names), and move mBitmap into the bitmap one only.

It seems pretty confusing to have mBitmap exist, but not actually initialized with anything in the GPU case.

::: gfx/2d/SourceSurfaceSkia.h
@@ +61,5 @@
>    IntSize mSize;
>    int32_t mStride;
>    DrawTargetSkia* mDrawTarget;
> +
> +  bool mVolatile;

Would prefer mOwnsData or similar, or at least documentation of this.

Comment 3

[George Wright (:gw280) (needinfo me!)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Can we instead create two subclasses, DrawTargetSkiaBitmap and
> DrawTargetSkiaGPU (or equivalent names), and move mBitmap into the bitmap
> one only.
> 

I originally went down that route with the initial integration work with SkiaGL, but decided against it because there is so much shared code that it doesn't make sense. Would it be better for DrawTarget to hold a pointer to the bitmap instead, and allow that to be nullptr in the GPU-case?

> 
> Would prefer mOwnsData or similar, or at least documentation of this.

Makes sense. I called it volatile because I figured it was an indicator of whether another object (ie - the DrawTarget) could modify its bitmap data behind its back.

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to George Wright (:gw280) from comment #3)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> > Can we instead create two subclasses, DrawTargetSkiaBitmap and
> > DrawTargetSkiaGPU (or equivalent names), and move mBitmap into the bitmap
> > one only.
> > 
> 
> I originally went down that route with the initial integration work with
> SkiaGL, but decided against it because there is so much shared code that it
> doesn't make sense. Would it be better for DrawTarget to hold a pointer to
> the bitmap instead, and allow that to be nullptr in the GPU-case?

All the shared code would still be in DrawTargetSkia. DrawTargetSkiaBitmap would just add mBitmap, the relevant Init() functions, and an overload of Snapshot(). The GPU version would be similar.

> 
> > 
> > Would prefer mOwnsData or similar, or at least documentation of this.
> 
> Makes sense. I called it volatile because I figured it was an indicator of
> whether another object (ie - the DrawTarget) could modify its bitmap data
> behind its back.

I don't mind really, documenting it as that would be fine too.

Comment 5

[George Wright (:gw280) (needinfo me!)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> (In reply to George Wright (:gw280) from comment #3)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> > > Can we instead create two subclasses, DrawTargetSkiaBitmap and
> > > DrawTargetSkiaGPU (or equivalent names), and move mBitmap into the bitmap
> > > one only.
> > > 
> > 
> > I originally went down that route with the initial integration work with
> > SkiaGL, but decided against it because there is so much shared code that it
> > doesn't make sense. Would it be better for DrawTarget to hold a pointer to
> > the bitmap instead, and allow that to be nullptr in the GPU-case?
> 
> All the shared code would still be in DrawTargetSkia. DrawTargetSkiaBitmap
> would just add mBitmap, the relevant Init() functions, and an overload of
> Snapshot(). The GPU version would be similar.

How about getting rid of the bitmap entirely and just wrapping the SkCanvas (which can be backed by either a bitmap or a texture)? I'd like to avoid differentiating too much between a GPU-backed DrawTarget and a bitmap-backed one, if at all possible.

Comment 6

[Matt Woodrow (:mattwoodrow)]

As you wish! I think mDevice can go away too.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SourceSurface with SkiaGL

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 743764 [details] [diff] [review]
Fix SourceSurface with SkiaGL

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

Skia parts look good, just a few issues with the other parts.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +707,5 @@
>      NS_ASSERTION(mDocShell, "Redraw with no canvas element or docshell!");
>      return NS_OK;
>    }
>  
> +  mThebesSurface = nullptr;

I'm not sure what these changes are trying to do.

On non-skia-gl platforms it seems that this would just skip the MarkDirty call (since we cache the gfxASurface pointer internally).

I believe you're trying to solve the problem where the gfxASurface for an accelerated skia surface isn't actually live, and is instead a readback copy.

All the callers of GetThebesSurfaceForDrawTarget expect a live surface.

My suggestion would be to take the patches from bug 836130 (the relevant ones should be reviewed, just not landed), which make GetThebesSurfaceForDrawTarget fail if we can't get a live surface, and instead adds code paths to the callers to handle this case.

::: gfx/thebes/gfxPlatform.cpp
@@ +747,5 @@
>  
>    surf->SetData(&kDrawSourceSurface, data.forget().drop(), DataSourceSurfaceDestroy);
>    // keep the draw target alive as long as we need its data
>    aTarget->AddRef();
> +  source->AddRef();

Can you explain how this is correct?

We use this code already in a lot of places, and it doesn't explode, so i'd assume we're not missing references.
It seems like this would leak.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> Comment on attachment 743764 [details] [diff] [review]
> Fix SourceSurface with SkiaGL
> 
> Review of attachment 743764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Skia parts look good, just a few issues with the other parts.
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +707,5 @@
> >      NS_ASSERTION(mDocShell, "Redraw with no canvas element or docshell!");
> >      return NS_OK;
> >    }
> >  
> > +  mThebesSurface = nullptr;
> 
> I'm not sure what these changes are trying to do.
> 
> On non-skia-gl platforms it seems that this would just skip the MarkDirty
> call (since we cache the gfxASurface pointer internally).
> 
> I believe you're trying to solve the problem where the gfxASurface for an
> accelerated skia surface isn't actually live, and is instead a readback copy.

Yes, that's the goal here.

> 
> All the callers of GetThebesSurfaceForDrawTarget expect a live surface.

> 
> My suggestion would be to take the patches from bug 836130 (the relevant
> ones should be reviewed, just not landed), which make
> GetThebesSurfaceForDrawTarget fail if we can't get a live surface, and
> instead adds code paths to the callers to handle this case.

Ok, that looks like a good way to go. I'll get a new patch up that includes those changes.

> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +747,5 @@
> >  
> >    surf->SetData(&kDrawSourceSurface, data.forget().drop(), DataSourceSurfaceDestroy);
> >    // keep the draw target alive as long as we need its data
> >    aTarget->AddRef();
> > +  source->AddRef();
> 
> Can you explain how this is correct?
> 
> We use this code already in a lot of places, and it doesn't explode, so i'd
> assume we're not missing references.
> It seems like this would leak.

Well it looks pretty fish that:

a) We're going to use the SourceSurface data without refing the SourceSurface
b) We have a destroy callback that unrefs the SourceSurface without refing it earlier

I cannot really explain how it didn't crash, but this looks more "right" to me. Anyway, with the changes described above this code should just be removed, so maybe it's a moot point.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SourceSurface with SkiaGL

This version incorporates some of the changes from bug 836130 as Matt suggested.

Comment 11

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 744149 [details] [diff] [review]
Fix SourceSurface with SkiaGL

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +712,5 @@
>      mThebesSurface =
>        gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mTarget);
> +
> +  if (mThebesSurface)
> +    mThebesSurface->MarkDirty();

This can just be in an else {}

@@ +746,5 @@
>      mThebesSurface =
>        gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mTarget);
> +
> +  if (mThebesSurface)
> +    mThebesSurface->MarkDirty();

Likewise.

@@ +935,5 @@
> +      return gfxASurface::ImageFormatRGB16_565;
> +    default:
> +      return gfxASurface::ImageFormatARGB32;
> +  }
> +}

gfx2DGlue.h -> SurfaceFormatToImageFormat()

::: gfx/thebes/gfxPlatform.cpp
@@ +724,5 @@
>        static_cast<cairo_surface_t*>(aTarget->GetNativeSurface(NATIVE_SURFACE_CAIRO_SURFACE));
>      return gfxASurface::Wrap(csurf);
>    }
>  
> +  if (aTarget->GetType() == BACKEND_SKIA) {

Won't this trigger when using Skia software as well?

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to George Wright (:gw280) from comment #11)
> Comment on attachment 744149 [details] [diff] [review]
> Fix SourceSurface with SkiaGL
> 
> Review of attachment 744149 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks, fixed all of that.

> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +724,5 @@
> >        static_cast<cairo_surface_t*>(aTarget->GetNativeSurface(NATIVE_SURFACE_CAIRO_SURFACE));
> >      return gfxASurface::Wrap(csurf);
> >    }
> >  
> > +  if (aTarget->GetType() == BACKEND_SKIA) {
> 
> Won't this trigger when using Skia software as well?

It will, yes, but in practice I don't think it makes much difference.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #12)
> (In reply to George Wright (:gw280) from comment #11)
> > >  
> > > +  if (aTarget->GetType() == BACKEND_SKIA) {
> > 
> > Won't this trigger when using Skia software as well?
> 
> It will, yes, but in practice I don't think it makes much difference.

Probably worth a comment to that effect in the code?

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 744149 [details] [diff] [review]
Fix SourceSurface with SkiaGL

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +712,5 @@
>      mThebesSurface =
>        gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mTarget);
> +
> +  if (mThebesSurface)
> +    mThebesSurface->MarkDirty();

I think we want to call MarkDirty even if we only just retrieved the surface. That's what it was doing previously at least. So I'd leave this as is.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SourceSurface with SkiaGL

Final patch. Added comment as Milan suggested.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=83b0c425c6bc

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Busted on WinXP, new Try run: https://tbpl.mozilla.org/?tree=Try&rev=129b43d6f6d9

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SourceSurface with SkiaGL

Bas, the previous patch crashed on XP in CanvasLayerD3D9::UpdateSurface because mSurface was null (as expected). I added some stuff there similar to what we do for CopyableCanvasLayer which avoids the crash, but stuff is hosed on Try. Does that change look ok? Any idea what's going on with the Try failures? https://tbpl.mozilla.org/?tree=Try&rev=8af5c5c2e876

Comment 19

[Bas Schouten (:bas.schouten)]

Comment on attachment 744730 [details] [diff] [review]
Fix SourceSurface with SkiaGL

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +711,5 @@
>    if (!mThebesSurface)
>      mThebesSurface =
>        gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mTarget);
> +
> +  if (mThebesSurface)

nit: Single line if's should still have {}, ignore that someone forgot to do so above :P

::: gfx/2d/DrawTargetSkia.cpp
@@ +627,4 @@
>    
> +  SkAutoTUnref<SkDevice> device(new SkDevice(GfxFormatToSkiaConfig(aFormat), aSize.width, aSize.height, isOpaque));
> +
> +  SkBitmap bitmap = (SkBitmap)device->accessBitmap(true);

We're ignoring the stride here as far as I can tell ... that's baaaad.

::: gfx/layers/d3d9/CanvasLayerD3D9.cpp
@@ +122,5 @@
>      D3DLOCKED_RECT lockedRect = textureLock.GetLockRect();
>  
>      nsRefPtr<gfxImageSurface> sourceSurface;
>  
> +    if (!mSurface && mDrawTarget->GetType() == BACKEND_SKIA) {

D3D10 layers can have Skia canvases as well for very large surfaces ( > 8192x8192), will this still work with this patch?

::: gfx/thebes/gfxPlatform.cpp
@@ +727,5 @@
>  
> +  if (aTarget->GetType() == BACKEND_SKIA) {
> +    // Unsupported, because SkiaGL cannot return a live surface. Of course, this also means we don't support
> +    // software Skia either, but that's fine. The fallback works fine for both.
> +    return nullptr;

Which fallback are we talking about here? This seems to refer to the place where we use this function, in itself this function doesn't provide a fallback. This needs to be carefully documented, potential users, as found in all kinds of places (https://mxr.mozilla.org/mozilla-central/search?string=GetThebesSurfaceForDRawTarget) may not feel so lucky.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Bas Schouten (:bas.schouten) from comment #19)
> Comment on attachment 744730 [details] [diff] [review]
> Fix SourceSurface with SkiaGL
> 
> Review of attachment 744730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +711,5 @@
> >    if (!mThebesSurface)
> >      mThebesSurface =
> >        gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mTarget);
> > +
> > +  if (mThebesSurface)
> 
> nit: Single line if's should still have {}, ignore that someone forgot to do
> so above :P

Fixed.

> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +627,4 @@
> >    
> > +  SkAutoTUnref<SkDevice> device(new SkDevice(GfxFormatToSkiaConfig(aFormat), aSize.width, aSize.height, isOpaque));
> > +
> > +  SkBitmap bitmap = (SkBitmap)device->accessBitmap(true);
> 
> We're ignoring the stride here as far as I can tell ... that's baaaad.

Good catch. Fixed.

> 
> ::: gfx/layers/d3d9/CanvasLayerD3D9.cpp
> @@ +122,5 @@
> >      D3DLOCKED_RECT lockedRect = textureLock.GetLockRect();
> >  
> >      nsRefPtr<gfxImageSurface> sourceSurface;
> >  
> > +    if (!mSurface && mDrawTarget->GetType() == BACKEND_SKIA) {
> 
> D3D10 layers can have Skia canvases as well for very large surfaces ( >
> 8192x8192), will this still work with this patch?

No, I guess the D3D10 layer will need a fallback.

> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +727,5 @@
> >  
> > +  if (aTarget->GetType() == BACKEND_SKIA) {
> > +    // Unsupported, because SkiaGL cannot return a live surface. Of course, this also means we don't support
> > +    // software Skia either, but that's fine. The fallback works fine for both.
> > +    return nullptr;
> 
> Which fallback are we talking about here? This seems to refer to the place
> where we use this function, in itself this function doesn't provide a
> fallback. This needs to be carefully documented, potential users, as found
> in all kinds of places
> (https://mxr.mozilla.org/mozilla-central/
> search?string=GetThebesSurfaceForDRawTarget) may not feel so lucky.

Indeed. I think there might be too many call sites to implement a fallback for each one. Maybe instead of having GetThebesSurfaceForDrawTarget fail, we should have some flag on the resulting surface that says whether or not it's live or not? I mean that's kind of a cludge too, but....ugh.

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

Alright, I talked about this with Jeff and we think maybe the best approach is to make it so GetThebesSurfaceForDrawTarget() is always assumed to be returning a copy. This means we need to just stop caching the result of this and always call it when the DrawTarget has changed.

In the mean time, I think I'm just going to add a hack similar to the Core Graphics one so I can land this, then work on the real fix afterwards.

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SourceSurface with SkiaGL

Ok, this version of the patch has two main changes:

1) Only fail GetThebesSurfaceForDrawTarget when we're using accelerated Skia. This *should* reduce the number of fallbacks we need to implement elsewhere.

2) Make sure we have RGBA in CanvasRenderingContext2D::GetInputStream, since that seems to be what the jpg and png encoders want. Cairo on Android uses 565 right now so it was causing some mochitest failures.

Started a Try run here: https://tbpl.mozilla.org/?tree=Try&rev=213b4293bc86

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 745311 [details] [diff] [review]
Fix SourceSurface with SkiaGL

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

::: gfx/2d/2D.h
@@ +840,5 @@
>    bool GetPermitSubpixelAA() {
>      return mPermitSubpixelAA;
>    }
>  
> +  virtual bool IsAccelerated() {

Was Bas ok with this change? 'Accelerated' seems like a somewhat meaningless term here.

I'd prefer to have a BACKEND_SKIA_ACCELERATED enum value that we return for GetType(), or something that more accurately describes 'can access raw pixel data without having to do readback'

Comment 24

[George Wright (:gw280) (needinfo me!)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> I'd prefer to have a BACKEND_SKIA_ACCELERATED enum value that we return for
> GetType(), or something that more accurately describes 'can access raw pixel
> data without having to do readback'

The problem I have with adding an extra backend type is that for the vast majority of cases where we'd be checking what the backend type is, BACKEND_SKIA would be equivalent to BACKEND_SKIA_ACCELERATED, and it seems a little weird to have "if (aBackend == BACKEND_SKIA || aBackend == BACKEND_SKIA_ACCELERATED)" cluttering up the codebase. I think it makes more sense to have a capability on the DrawTarget that lets other people know whether it supports live surface wrapping (for want of a better description).

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to George Wright (:gw280) from comment #24)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> > I'd prefer to have a BACKEND_SKIA_ACCELERATED enum value that we return for
> > GetType(), or something that more accurately describes 'can access raw pixel
> > data without having to do readback'
> 
> The problem I have with adding an extra backend type is that for the vast
> majority of cases where we'd be checking what the backend type is,
> BACKEND_SKIA would be equivalent to BACKEND_SKIA_ACCELERATED, and it seems a
> little weird to have "if (aBackend == BACKEND_SKIA || aBackend ==
> BACKEND_SKIA_ACCELERATED)" cluttering up the codebase. I think it makes more
> sense to have a capability on the DrawTarget that lets other people know
> whether it supports live surface wrapping (for want of a better description).

Yes, this is precisely the reason I added DrawTarget::IsAccelerated(). Let's get Bas to approve :)

Comment 26

[Bas Schouten (:bas.schouten)]

Comment on attachment 745311 [details] [diff] [review]
Fix SourceSurface with SkiaGL

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

This doesn't seem to implement the changes we discussed in #gfx?

::: gfx/2d/2D.h
@@ +840,5 @@
>    bool GetPermitSubpixelAA() {
>      return mPermitSubpixelAA;
>    }
>  
> +  virtual bool IsAccelerated() {

I agree with Matt. I think the enum value is the right thing to do.

::: gfx/thebes/gfxPlatform.cpp
@@ +731,5 @@
>      return gfxASurface::Wrap(csurf);
>    }
>  
> +  if (aTarget->GetType() == BACKEND_SKIA && aTarget->IsAccelerated()) {
> +    // Unsupported, because SkiaGL cannot return a live surface.

From what I recall from our discussion we don't want this to necessarily return a live surface, we want it to return -either- a live surface or not, and the owners not to guarantee extended usage.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Bas Schouten (:bas.schouten) from comment #26)
> Comment on attachment 745311 [details] [diff] [review]
> Fix SourceSurface with SkiaGL
> 
> Review of attachment 745311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem to implement the changes we discussed in #gfx?

Yeah, but then Matt alerted me to a new problem. The issue with GetThebesSurfaceForDrawTarget() returning a copy is that we *write* to it in CanvasRenderingContext2D::DrawWindow(). By failing it when we can't return a live surface, DrawWindow() can take another route. It is very sad.

> 
> ::: gfx/2d/2D.h
> @@ +840,5 @@
> >    bool GetPermitSubpixelAA() {
> >      return mPermitSubpixelAA;
> >    }
> >  
> > +  virtual bool IsAccelerated() {
> 
> I agree with Matt. I think the enum value is the right thing to do.

We can do it, but if (BACKEND_SKIA || BACKEND_SKIA_ACCELERATED) is annoying. I guess we could add some if (IsSkia(backend)) thing.


> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +731,5 @@
> >      return gfxASurface::Wrap(csurf);
> >    }
> >  
> > +  if (aTarget->GetType() == BACKEND_SKIA && aTarget->IsAccelerated()) {
> > +    // Unsupported, because SkiaGL cannot return a live surface.
> 
> From what I recall from our discussion we don't want this to necessarily
> return a live surface, we want it to return -either- a live surface or not,
> and the owners not to guarantee extended usage.

Right, but see above.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Testcase: check if dataUrl works

So, I am currently getting lots of Valgrind errors about uninitialized data, which boil down to toDataURL() passing uninitialized data to the PNG encoder, and Snorp tells me that this should be the same as the present bug. So here's my testcase, please check that the patch here fixes it. Thanks!

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Branched off into 875216 as I'm not able to apply cleanly the current patch.

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix SourceSurface with SkiaGL

Rebased upon bug 854556. I'm getting some strange failures on Linux where it looks like DrawWindow() just puts zeros.

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

Matt, you can see the failures with this patch here: https://tbpl.mozilla.org/?tree=Try&rev=c8811c552308

I guess something is wrong in DrawWindow, but what? Is there a better way to do that kind of thing?

Comment 34

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 755999 [details] [diff] [review]
Fix SourceSurface with SkiaGL

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3221,5 @@
>    unused << shell->RenderDocument(r, renderDocFlags, backgroundColor, thebes);
> +  if (drawSurf) {
> +    gfxIntSize size = drawSurf->GetSize();
> +
> +    nsRefPtr<gfxImageSurface> img = drawSurf->GetAsReadableARGB32ImageSurface();

Looks like the implementation of this doesn't take the device offset you set above into account.

You should try zeroing it, or change the impl.

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Make SourceSurface work with SkiaGL

Alright, this patch passes Try. Finally. I don't really understand why the floor() stuff was needed, however. Is it important to know how your patches work? https://tbpl.mozilla.org/?tree=Try&rev=e0b964cc8530

Comment 36

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1d47b8501d

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a1d47b8501d