960254 - Add ability to properly manage write access for DataSourceSurfaces

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Right now we have a GetData() caller on DataSourceSurfaces. This is not really what we want, in some cases you can write, in other cases you can't. An explicit Map/Unmap pair is much more like what we want.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add Map/Unmap API to DataSourceSurface

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Convert some users using the old MarkDirty API.

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Remove MarkDirty API to avoid confusion.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Part 4: Implement the Map/Unmap API on Direct2D.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8360653 [details] [diff] [review]
Part 1: Add Map/Unmap API to DataSourceSurface

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

::: gfx/2d/2D.h
@@ +361,5 @@
>     * Can return null if there was OOM allocating surface data.
>     */
>    virtual uint8_t *GetData() = 0;
>  
> +  /* [DEPTRECATED]

spelling is wrong.

Comment 6

[Andreas Gal :gal]

Thanks. I really disliked MarkDirty() but I didn't have the right fix for it.

Comment 7

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f6fbc397bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd9b6b044a5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f499e5f7954
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aecc5ca2a19
https://hg.mozilla.org/integration/mozilla-inbound/rev/4441c32c724c

Comment 8

[Bas Schouten (:bas.schouten)]

Backed out for wrong bug number:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2628e64cf375

Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b61f6abbd13d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae8e5f144f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbad01205856
https://hg.mozilla.org/integration/mozilla-inbound/rev/327d381e24c8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b61f6abbd13d
https://hg.mozilla.org/mozilla-central/rev/3ae8e5f144f1
https://hg.mozilla.org/mozilla-central/rev/bbad01205856
https://hg.mozilla.org/mozilla-central/rev/327d381e24c8

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't there be an RAII helper object for this?

Are we going to convert all users of Data/Stride to this?

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away January 20-24) from comment #10)
> Shouldn't there be an RAII helper object for this?
> 
> Are we going to convert all users of Data/Stride to this?

Eventually I'd like to, yes. Also I have no objection to an RAII helper. Although personally I often to prefer explicit locking/unlocking because it makes us explicitly think about where we do the lock when we for example start adding a lot of branches that exit a function. On the other hand I've been guilty of forgetting to add an unlock in one of these branches regardless so I also see the value in using RAII helpers :). In this case all the locks/unlocks could occur close together so I didn't feel there was a lot of value in having a helper here.

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Shouldn't there be an RAII helper object for this?

I think there should be. Bug 979853 comment 6 notes that we're already screwing up the Unmap() calls, and calling DataSourceSurfaceD2D::Unmap() seems guaranteed to crash debug builds. Probably best to discuss this in bug 980233 though.