1162357 - Mixed usage of DataSourceSurfaceD2D1::GetData()/Map()

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andreas Pehrson [:pehrsons]]

With my patches for bug 1032848 I hit a path where DataSourceSurfaceD2D1::GetData and DataSourceSurfaceD2D1::Map are both used. This is not allowed on the same surface - assertions are failing.

See my original try run with Windows failures here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=483aa1f2470d

After converting a bunch of the DataSourceSurface::GetData() users over to Map, and putting an NS_ASSERTION(false) in GetData() I had the following try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0560515d2529

The log reveals that there was some use of GetData() left:
> (GetData) 04:15:14 INFO - [Parent 1300] ###!!! ASSERTION: Someone called GetData on a DataSourceSurfaceD2D1: 'false', file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/gfx/2d/SourceSurfaceD2D1.cpp, line 168
> (Map)     04:15:15 INFO - Assertion failure: !mMapped, at c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/gfx/2d/SourceSurfaceD2D1.cpp:178
> (Map)     04:15:29 INFO - #01: nsLayoutUtils::SurfaceFromElement(mozilla::dom::HTMLVideoElement *,unsigned int,mozilla::gfx::DrawTarget *) [layout/base/nsLayoutUtils.cpp:6775]
> (GetData) 04:15:29 INFO - #01: mozilla::gfx::CreatePartialBitmapForSurface [gfx/2d/HelpersD2D.h:579]

Comment 1

[Andreas Pehrson [:pehrsons]]

I tried converting DataSourceSurface::GetData() cases to Map(), but it soon exploded and to my surprise caused some reftest failures on Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c170ab1bb95f

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Revert some cases of DataSourceSurface::Map() to GetData() on Windows

This worked for me locally.

Waiting for the try push before requesting r?,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5f21543c903

Comment 3

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #1)
> I tried converting DataSourceSurface::GetData() cases to Map(), but it soon
> exploded and to my surprise caused some reftest failures on Android:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c170ab1bb95f

The reftest failure was not caused by me apparently, but it's from a test that's not run on m-i or m-c. :/

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Convert some usage of DataSourceSurface::GetData() to Map()

Here's what I have now. This converts a bunch of GetData() calls to Map().

It also reveals a race condition. Two threads (main and compositor) are competing to Map() the same DataSourceSurface.

This is from using said DataSourceSurface for two purposes. See stack traces from the race contenders using this patch on Windows 7 below. To reproduce, run dom/canvas/test/test_capture.html from bug 1032848 a couple of iterations with this patch applied on top (I had also added NS_ASSERTION(false) in DataSourceSurfaceD2D1::Map() to get the stack traces).

1. CanvasRenderingContext2D::DrawImage() the DataSourceSurface from one canvas into another. Main thread.
> [5704] ###!!! ASSERTION: Map: 'false', file c:/Users/APehrson/gecko-dev/gfx/2d/SourceSurfaceD2D1.cpp, line 177
> #01: mozilla::gfx::DataSourceSurface::ScopedMap::ScopedMap (c:\users\apehrson\gecko-dev\gfx\2d\2d.h:402)
> #02: mozilla::gfx::DrawTargetD2D1::OptimizeSourceSurface (c:\users\apehrson\gecko-dev\gfx\2d\drawtargetd2d1.cpp:1462)
> #03: nsLayoutUtils::SurfaceFromElement (c:\users\apehrson\gecko-dev\layout\base\nslayoututils.cpp:6779)
> #04: nsLayoutUtils::SurfaceFromElement (c:\users\apehrson\gecko-dev\layout\base\nslayoututils.cpp:6807)
> #05: mozilla::dom::CanvasRenderingContext2D::DrawImage (c:\users\apehrson\gecko-dev\dom\canvas\canvasrenderingcontext2d.cpp:4317)
> #06: mozilla::dom::CanvasRenderingContext2D::DrawImage (c:\users\apehrson\gecko-dev\obj-i686-pc-mingw32\dist\include\mozilla\dom\canvasrenderingcontext2d.h:215)
> #07: mozilla::dom::CanvasRenderingContext2DBinding::drawImage (c:\users\apehrson\gecko-dev\obj-i686-pc-mingw32\dom\bindings\canvasrenderingcontext2dbinding.cpp:4123)

2. Compositing video frames originating from HTMLCanvasElement::CaptureStream and then played in a video element. Compositor thread.
> [5704] ###!!! ASSERTION: Map: 'false', file c:/Users/APehrson/gecko-dev/gfx/2d/SourceSurfaceD2D1.cpp, line 177
> #01: mozilla::gfx::DataSourceSurface::ScopedMap::ScopedMap (c:\users\apehrson\gecko-dev\gfx\2d\2d.h:402)
> #02: mozilla::gfx::CreatePartialBitmapForSurface (c:\users\apehrson\gecko-dev\gfx\2d\helpersd2d.h:569)
> #03: mozilla::gfx::DrawTargetD2D1::GetImageForSurface (c:\users\apehrson\gecko-dev\gfx\2d\drawtargetd2d1.cpp:1444)
> #04: mozilla::gfx::DrawTargetD2D1::CopySurface (c:\users\apehrson\gecko-dev\gfx\2d\drawtargetd2d1.cpp:318)
> #05: mozilla::layers::CairoImage::GetTextureClient (c:\users\apehrson\gecko-dev\gfx\layers\imagecontainer.cpp:562)
> #06: mozilla::layers::ImageClientSingle::UpdateImage (c:\users\apehrson\gecko-dev\gfx\layers\client\imageclient.cpp:161)
> #07: mozilla::layers::UpdateImageClientNow (c:\users\apehrson\gecko-dev\gfx\layers\ipc\imagebridgechild.cpp:386)
> #08: RunnableFunction<void (__cdecl*)(mozilla::layers::ImageClient *,mozilla::layers::ImageContainer *),Tuple2<mozilla::layers::ImageClient *,nsRefPtr<mozilla::layers::ImageContainer> > >::Run (c:\users\apehrson\gecko-dev\ipc\chromium\src\base\task.h:420)

nical, bas: Would any of you have some advice on how to fix this?

Comment 5

[Andreas Pehrson [:pehrsons]]

I noted that CanvasRenderingContext2D::DrawImage() will lead to a DrawTarget::OptimizeSourceSurface() call that I am not doing in the HTMLCanvasElement::CaptureStream() path. Doing so could solve this issue for now I guess (haven't tried), but I also guess we're still exposed to the same race, say on platforms where the surface doesn't have to be optimized.

Comment 6

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #5)
> but I also guess we're still exposed to the
> same race, say on platforms where the surface doesn't have to be optimized.

Perhaps not. The compositor is obviously not racing itself.

I looped in OptimizeSourceSurface() in my code and it seems to work. Would still appreciate some feedback on the issue though.

Comment 7

[Nicolas Silva [:nical]]

Eventually we want to support multiple read-maps to DataSourceSurfaces. There's a bit of that happening in bug 1083101 (but only for some of the cpu backends initially). We could create a separate bug to track that, move what started in bug 1083101 there and do it for all backends.

Comment 8

[Andreas Pehrson [:pehrsons]]

(In reply to Nicolas Silva [:nical] from comment #7)
> Eventually we want to support multiple read-maps to DataSourceSurfaces.
> There's a bit of that happening in bug 1083101 (but only for some of the cpu
> backends initially). We could create a separate bug to track that, move what
> started in bug 1083101 there and do it for all backends.

Cool. I won't need these conversions for bug 1032848 when using DrawTarget::OptimizSourceSurface() it turns out. I still did them though, and the try looks OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0a157edc26

There are some failures but looking at recent m-c testing they seem unrelated to my patch here.

So in case gfx would like to get a step closer to removing GetData() and Stride(), feel free to take them. I'll request r? shortly.

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8604508 [details] [diff] [review]
Convert some usage of DataSourceSurface::GetData() to Map()

Noteworthy: I added a new class DataSourceSurface::ScopedMap for having a MappedSurface that automatically Unmap()s when leaving scope. It really made the chore of doing these conversions easier (Having 4 MappedSurfaces in the same scope and handling the cases of Map() failing for the latter ones, for instance). Not sure if it was a conscious move to not include one from the start though. Let me know what you think.

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 8604508 [details] [diff] [review]
Convert some usage of DataSourceSurface::GetData() to Map()

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

Excellent work! This looks pretty good as far as I can tell, however please make sure any places where you use the 'ScopedMap' you scope the object to the portion of the code where the map is required to prevent holding the surface's internal locks longer than needed.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4689,2 @@
>                                             data->GetFormat());
> +    data->Unmap();

i.e. object should go out of scope here

@@ +4950,1 @@
>    }

readback could be unmapped here I think? although it's a little hard to tell without the function context :)

::: gfx/2d/2D.h
@@ +397,5 @@
>  
> +  class ScopedMap {
> +  public:
> +    explicit ScopedMap(DataSourceSurface* aSurface, MapType aType)
> +      : mSurface(aSurface),

nit: please follow style in the rest of this file and put the comma on the next line.

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: bz://1162357/pehrsons

/r/8741 - Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas

Pull down this commit:

hg pull -r 7e63fcd36606b7af09f1bfaf56fdd74612b78415 https://reviewboard-hg.mozilla.org/gecko/

Comment 12

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8604508 [details] [diff] [review]
Convert some usage of DataSourceSurface::GetData() to Map()

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +5007,5 @@
>        *dst++ = a;
>      }
>      src += srcStride - (dstWriteRect.width * 4);
>      dst += (aWidth * 4) - (dstWriteRect.width * 4);
>    }

Inside this scope is a loop body, and you can see src being updated here. It's used in the body.

I'll put the next revision on reviewboard and it'll be a lot easier to see :-)

::: gfx/2d/2D.h
@@ +397,5 @@
>  
> +  class ScopedMap {
> +  public:
> +    explicit ScopedMap(DataSourceSurface* aSurface, MapType aType)
> +      : mSurface(aSurface),

Done, comma moved.

Comment 13

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8605635 [details]
MozReview Request: bz://1162357/pehrsons

/r/8741 - Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas

Pull down this commit:

hg pull -r 43e61138fb127472188be092903d58022a8b6d90 https://reviewboard-hg.mozilla.org/gecko/

Comment 14

[Andreas Pehrson [:pehrsons]]

I went through all uses of ScopedMap and most scopes are already good. I fixed a few. You can see the diff on reviewboard.

Comment 15

[Bas Schouten (:bas.schouten)]

https://reviewboard.mozilla.org/r/8741/#review9163

Ship It!

Comment 16

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8605635 [details]
MozReview Request: bz://1162357/pehrsons

r=bas

I think this wasn't updated due to an old-style reviewboard attachment (the entire patchset) while the r+ was for the new-style attachment (the single patch).

Comment 17

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas

Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas

Comment 18

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8617718 [details]
MozReview Request: Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas

Rebased on m-c. Carrying r=bas.

Comment 19

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8617718 [details]
MozReview Request: Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas

Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas

Comment 20

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8617718 [details]
MozReview Request: Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas

Trivial fix in HelpersD2D.h - bringing PreScale() into scope to have newSize available. PreScale() consists only of 4 assignments so no biggie.

Carries forward r=bas.

Based on this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19ba2347bd95
One more win-only with this fix on the way: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b1a250bc2a5

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/536bd9910bc2

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/536bd9910bc2