Closed Bug 1162357 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(1 file, 3 obsolete files)

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]
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
This worked for me locally.

Waiting for the try push before requesting r?,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5f21543c903
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
(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. :/
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?
Attachment #8603978 - Attachment is obsolete: true
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(bas)
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.
(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.
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.
Flags: needinfo?(nical.bugzilla)
(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 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.
Attachment #8604508 - Flags: review?(bas)
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.
Attachment #8604508 - Flags: review?(bas) → review-
/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 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.
Attachment #8605635 - Flags: review?(bas)
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/
I went through all uses of ScopedMap and most scopes are already good. I fixed a few. You can see the diff on reviewboard.
Flags: needinfo?(bas)
Attachment #8604508 - Attachment is obsolete: true
No longer blocks: 1032848
See Also: → 1032848
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).
Attachment #8605635 - Flags: review?(bas) → review+
Blocks: 1169125
Bug 1162357 - Convert some usage of DataSourceSurface::GetData() to Map(). r=bas
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.
Attachment #8617718 - Flags: review+
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
Attachment #8617718 - Flags: review+
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
Attachment #8617718 - Flags: review+
Attachment #8605635 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/536bd9910bc2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Regressions: 1651419
You need to log in before you can comment on or make changes to this bug.