997758 - Make SourceSurfaces read-only

Status: NEW

(Core :: Graphics, defect)

Description

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

Attachment: overview patch

This is the overview patch so you can see how it works out. It does many things, which I'd separate out for review and landing. It builds on Linux but I haven't tested it yet.

SourceSurfaces become immutable. Well, not completely; there are places in the code that currently call DataSourceSurface::GetData and then write to it, which I haven't fixed in this patch. But everything that currently uses Map has been converted. I still think this is very important. For example, all our optimizations that cache Snapshot() in an internal mSnapshot depend on the invariant that SourceSurfaces are immutable. The places where we modify DataSourceSurfaces are often quite hairy, for example the CopyableCanvasLayer code is quite nasty, especially when it tries to call Snapshot() and then deliberately modify the surface as an alternative to calling LockBits!

DataSourceSurface::Map can now only be used read-only so there's no need for a MapType parameter.

I've added size and format fields to MappedSurface and moved it out of DataSourceSurface so it can be used in more places. It's a useful little struct for anywhere we want to pass around just the essential data for an in-memory image surface without specifying a particular ownership model or other API. There's also a ReadOnlyMappedSurface version that only gives you a const pointer to the data. DataSourceSurface::Map returns a ReadOnlyMappedSurface.

I've introduced MutableDataSurface, which owns a mutable in-memory image data buffer. The only implementation is GenericMutableDataSurface currently, but we could have other implementations, for examples ones that allocate memory in special ways (e.g. gralloc or shared memory). MutableDataSurfaces are not SourceSurfaces so you can't draw them directly. You call MutableDataSurface::Snapshot to generate a SourceSurface with the data. This is zero-copy if you don't call MutableDataSurface::Map again.

GenericMutableDataSurface is very simple to implement. If you call GenericMutableDataSurface::Snapshot(), use and destroy the DataSourceSurface, and then Map()/Unmap() again and repeat the cycle, no copying occurs. Copying only occurs if you call Map() while the last Snapshot() is still alive.

I've made DrawTarget::LockBits return a MappedSurface and renamed ReleaseBits to UnlockBits since Lock/Unlock is the usual pairing.

I've added CreateMutableDataSurface(WithStride) to Factory. CreateDataSourceSurface(WithStride) should be removed from Factory, since it doesn't make sense to create empty surfaces that are immutable, but we can't do that yet because of those places that create empty DataSourceSurfaces and mutate them through GetData().

Lots of places that perform read-only Map() calls on DataSourceSurfaces are tweaked for the new syntax.

I fix some bugs in DrawTargetCairo:
-- If someone does drawTargetCairo->Snapshot()->GetDataSurface(), we create a SourceSurfaceCairo and then a DataSourceSurfaceCairo both referring to the same cairo surface. But DrawTargetCairo doesn't know about DataSourceSurfaceCairo so the DataSourceSurfaceCairo won't get notified properly if someone renders again through the DrawTarget. I fix that by having Snapshot() create the DataSourceSurfaceCairo directly if we can.
-- If you call Snapshot()->GetDataSurface() and then Map() it, and at the same time do more drawing through the DrawTarget, things would go wrong. I prevent that by making a copy if Map() is called while we're still sharing the cairo surface with the DrawTarget.

GLScreenBuffer::Readback takes a MappedSurface to modify instead of a DataSourceSurface. We should probably modify all our functions that just want to modify an in-memory image buffer to operate on MappedSurfaces, and those that just want to read one, ReadOnlyMappedSurfaces.

SharedSurfaceGL::mData changes from a DataSourceSurface to a MutableDataSurface. We do the same in a few other places.

CopyableCanvasLayer::UpdateTarget got a serious makeover which fixes at least one bug according to Matt. The most important change is to use LockBits instead of trying to get a DataSourceSurface that shared memory with the DrawTarget and writing into that. Also, CopyableCanvasLayer caches a MutableDataSurface to avoid allocations, instead of a DataSourceSurface.

I made PremultiplySurface in LayerUtils operate on a MappedSurface. BTW that should move to gfxUtils and LayerUtils can just go away.

TextRenderer::mGlyphBitmaps seemed like we were abusing a long-lived Map just to provide a semi-convenient way to allocate a memory buffer. I modified it so mGlyphBitmaps is just a memory buffer, and that seemed to simplify things.

In several places, e.g. imgTools::EncodeScaledImage, we do a complicated dance where we create a DataSourceSurface, Map it, create a DrawTargetCairo pointing at those bits, draw to it, and then use the resulting DataSourceSurface. This is much cleaner expressed as just creating a DrawTargetCairo, drawing to it, and taking a Snapshot --- as long as we destroy the DrawTargetCairo before doing anything significant with the Snapshot, to ensure the Snapshot doesn't incur a copy.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8408273 [details] [diff] [review]
overview patch

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

Looks awesome!

::: gfx/2d/SourceSurfaceCairo.h
@@ +54,5 @@
>    virtual ~DataSourceSurfaceCairo();
>    virtual unsigned char *GetData();
>    virtual int32_t Stride();
>  
>    virtual SurfaceType GetType() const { return SurfaceType::CAIRO_IMAGE; }

Since you're not using DataSourceSurfaceWrapper any more, I think this needs to return SurfaceType::DATA.

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 8408273 [details] [diff] [review]
overview patch

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

There's a bunch of improvements this patch does which I really like, and it does indeed fix several bugs, as well as improving a bunch of the existing code which doesn't really need to mutate source surfaces.

I'm not a fan, we're doing something different from what other APIs do and I think there's a reason other APIs do things the way they do it. However if there's significant support for it I'm willing to give in and clean up the mess that will pop up later :-). Note at the very least, as you already hinted at, we'll be needing backend specific mutable data surfaces longer term. A bunch of upload optimizations we plan doing in the future depend on decoding directly into a D2D SystemMem bitmap. When you for example decode an image into a D2D SystemMem bitmap, the upload can be pipelined and happen synchronously, preventing the need for a copy.

We should note we lose our ability to get a DataSourceSurface snapshot and mutate the data in there in-place, we will now always need to make a copy, this should usually not matter too much, and in the cases where we need it we can probably live with the perf impact of the additional copy, since we're doing readback anyway, but it's still wasteful. Maybe we should add a GetMutableSourceSurface on SourceSurface to get a copy-on-write version of the SourceSurface? It all gets really ugly from there though.

f- because I still don't think the immutable source surfaces it's a good idea, but as I said, if it somehow makes people's lives easier (although I'm not buying it has a significant advantage), that's something I wouldn't mind facilitating. f+ for a bunch of the other improvements that this makes.

I'm not entirely sure how I feel about Format moving into MappedSurface, but I don't foresee any problems now or in the future, and it seems to make things cleaner, so that seems fine as well.

::: gfx/2d/SourceSurfaceCairo.h
@@ +54,5 @@
>    virtual ~DataSourceSurfaceCairo();
>    virtual unsigned char *GetData();
>    virtual int32_t Stride();
>  
>    virtual SurfaceType GetType() const { return SurfaceType::CAIRO_IMAGE; }

It can't cairo uses this to determine if it can cast to DataSourceSurfaceCairo, that's why DataSourceSurfaceWrapper exists :).

Comment 3

[Bas Schouten (:bas.schouten)]

Fwiw, I'd be much more open to something like a creation flag or something to normal DataSourceSurfaces that simply makes a Write Map fail if it's called on those surfaces. And then most users could simply use that.

Comment 4

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

(In reply to Bas Schouten (:bas.schouten) from comment #2)
> I'm not a fan, we're doing something different from what other APIs do and I
> think there's a reason other APIs do things the way they do it.

OK, but what do you think that reason is? :-)

> However if
> there's significant support for it I'm willing to give in and clean up the
> mess that will pop up later :-).

Any thoughts on what kind of mess is at risk here?

> Note at the very least, as you already
> hinted at, we'll be needing backend specific mutable data surfaces longer
> term. A bunch of upload optimizations we plan doing in the future depend on
> decoding directly into a D2D SystemMem bitmap. When you for example decode
> an image into a D2D SystemMem bitmap, the upload can be pipelined and happen
> synchronously, preventing the need for a copy.

That's easily added on top of this patch, I think.

> We should note we lose our ability to get a DataSourceSurface snapshot and
> mutate the data in there in-place, we will now always need to make a copy,
> this should usually not matter too much, and in the cases where we need it
> we can probably live with the perf impact of the additional copy, since
> we're doing readback anyway, but it's still wasteful. Maybe we should add a
> GetMutableSourceSurface on SourceSurface to get a copy-on-write version of
> the SourceSurface? It all gets really ugly from there though.

We could easily add to DrawTarget
  virtual TemporaryRef<MutableDataSurface> SnapshotMutable();
for consumers who need to take a snapshot and mutate it. I was going to add it to this patch actually, but it turned out we don't need it yet.

(In reply to Bas Schouten (:bas.schouten) from comment #3)
> Fwiw, I'd be much more open to something like a creation flag or something
> to normal DataSourceSurfaces that simply makes a Write Map fail if it's
> called on those surfaces. And then most users could simply use that.

If we go down that "immutable flag" path, we'd probably need the ability to create a mutable SourceSurface and then later mark it immutable after you've finished modifying it. That approach is not too bad but I think having SourceSurfaces always be immutable is simpler and better.

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> (In reply to Bas Schouten (:bas.schouten) from comment #2)
> > I'm not a fan, we're doing something different from what other APIs do and I
> > think there's a reason other APIs do things the way they do it.
> 
> OK, but what do you think that reason is? :-)

I just think it's generally desirable to pass around a surface that generally -can- be mutable but doesn't necessarily -have- to be. And also to be able to pass that directly to a drawing API avoiding both the API and implementation clutter of an additional snapshot.

i.e.

ModifyMyImage(mutableSourceSurface);
snapshot = mutableSourceSurface->Snapshot();
dt->DrawImage(Snapshot);

Is just a little uglier, I think in general there's just a -lot- of cases where you don't want to care, i.e. it's feasible to have an API where you have an input surface, and you want to -maybe- make some modifications to that. It's nicest if you can input something into that, and -if- it does want to make modifications it will try to MAP_WRITE, and if it fails it will go and create a writable surface and make that copy (guaranteeing the input surface's immutability). But if it succeeds it can just happily keep using it, but at the same time you don't want to always pass in a mutable data sourcesurface, because maybe it -doesn't- need to change it most of the time and then you don't want to needlessly make it mutable. You could of course simply have two function signatures for this but I think this would cause more API clutter.

I do think in some cases making things more explicit is good, but in the case of mutability I think there's just too many people along the way that 'don't care' to make it a good idea. We often have long control paths through a lot of callers, i.e. imagine a stack like this:

<actual effect code, might want to do it in-place and care>
<code that applies the effects, doesn't care itself yet>
<code that wants to draw the surface to some draw target, doesn't care>
<code that gets the surface from an element, doesn't care>
<create surface code, could be mutable or not>

There won't always be effects, and there won't always be effects that can efficiently do this in-place. But if the effect -can- do this efficiently in place, it's terribly nice if the CreateSurface code happens to know it doesn't care about immutability (i.e. the SourceSurface was created once, just for this frame anyway), it can do this in place and make no guarantees on the surface.

> > We should note we lose our ability to get a DataSourceSurface snapshot and
> > mutate the data in there in-place, we will now always need to make a copy,
> > this should usually not matter too much, and in the cases where we need it
> > we can probably live with the perf impact of the additional copy, since
> > we're doing readback anyway, but it's still wasteful. Maybe we should add a
> > GetMutableSourceSurface on SourceSurface to get a copy-on-write version of
> > the SourceSurface? It all gets really ugly from there though.
> 
> We could easily add to DrawTarget
>   virtual TemporaryRef<MutableDataSurface> SnapshotMutable();
> for consumers who need to take a snapshot and mutate it. I was going to add
> it to this patch actually, but it turned out we don't need it yet.
> 
> (In reply to Bas Schouten (:bas.schouten) from comment #3)
> > Fwiw, I'd be much more open to something like a creation flag or something
> > to normal DataSourceSurfaces that simply makes a Write Map fail if it's
> > called on those surfaces. And then most users could simply use that.
> 
> If we go down that "immutable flag" path, we'd probably need the ability to
> create a mutable SourceSurface and then later mark it immutable after you've
> finished modifying it. That approach is not too bad but I think having
> SourceSurfaces always be immutable is simpler and better.

I think this is actually more desirable (I'm even fine with that decision being irreversible, so that if you have a codepath where you know if you sent it down that codepath you don't ever want it to change again, you can have the guarantee that callers who try to MAP_WRITE simply fail), at that point also a question actually becomes whether you ever want DataSourceSurface, or whether you just want SourceSurface, and if Map(READ) fails you want to do something like a GetDataSurface call that returns a regular SourceSurface but just one where you know Map(READ) will succeed. This would actually simplify a lot of the code inside the backends as you'd no longer need to have the casts to DataSourceSurface which are currently tricky because of the aforementioned WrappedDataSourceSurface stuff. Now that I think of it, that is a change we could, and probably should, make regardless of whether we have a mutable class or not. At that point if we did choose to have a MutableDataSurface type we should probably call it MutableSurface (as there's no guarantee, for example with D2D, that it's internally a 'raw' data surface).

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas.schouten) from comment #5)
> 
> ModifyMyImage(mutableSourceSurface);
> snapshot = mutableSourceSurface->Snapshot();
> dt->DrawImage(Snapshot);
> 
> Is just a little uglier,

It is, but this is a rare case (as you can see from the patches). I don't think a bit of uglyness in these rare cases is much of a drawback.

> I think in general there's just a -lot- of cases
> where you don't want to care, i.e. it's feasible to have an API where you
> have an input surface, and you want to -maybe- make some modifications to
> that. It's nicest if you can input something into that, and -if- it does
> want to make modifications it will try to MAP_WRITE, and if it fails it will
> go and create a writable surface and make that copy (guaranteeing the input
> surface's immutability). But if it succeeds it can just happily keep using
> it, but at the same time you don't want to always pass in a mutable data
> sourcesurface, because maybe it -doesn't- need to change it most of the time
> and then you don't want to needlessly make it mutable. You could of course
> simply have two function signatures for this but I think this would cause
> more API clutter.

Isn't that going to be just as ugly though (or worse)? We need to try MAP_WRITE, if that fails than allocate a new surface, MAP_READ the old one and MAP_WRITE the new one, do a copy, and then carry on.

Having two function signatures, where the one that takes the immutable source surface just calls SameFunction(aSurface->AsMutable()); seems better to me :)

> 
> I do think in some cases making things more explicit is good, but in the
> case of mutability I think there's just too many people along the way that
> 'don't care' to make it a good idea. We often have long control paths
> through a lot of callers, i.e. imagine a stack like this:
> 
> <actual effect code, might want to do it in-place and care>
> <code that applies the effects, doesn't care itself yet>
> <code that wants to draw the surface to some draw target, doesn't care>
> <code that gets the surface from an element, doesn't care>
> <create surface code, could be mutable or not>
> 
> There won't always be effects, and there won't always be effects that can
> efficiently do this in-place. But if the effect -can- do this efficiently in
> place, it's terribly nice if the CreateSurface code happens to know it
> doesn't care about immutability (i.e. the SourceSurface was created once,
> just for this frame anyway), it can do this in place and make no guarantees
> on the surface.
> 
> > > We should note we lose our ability to get a DataSourceSurface snapshot and
> > > mutate the data in there in-place, we will now always need to make a copy,
> > > this should usually not matter too much, and in the cases where we need it
> > > we can probably live with the perf impact of the additional copy, since
> > > we're doing readback anyway, but it's still wasteful. Maybe we should add a
> > > GetMutableSourceSurface on SourceSurface to get a copy-on-write version of
> > > the SourceSurface? It all gets really ugly from there though.

For both of these we can solve this with:

MutableSurface *mutable = data->AsMutable();
data = nullptr;
// start mutating mutable and no copy should be required.

Or just by having the SnapshotMutable() option on DT as roc suggeagted.

> 
> I think this is actually more desirable (I'm even fine with that decision
> being irreversible, so that if you have a codepath where you know if you
> sent it down that codepath you don't ever want it to change again, you can
> have the guarantee that callers who try to MAP_WRITE simply fail), at that
> point also a question actually becomes whether you ever want
> DataSourceSurface, or whether you just want SourceSurface, and if Map(READ)
> fails you want to do something like a GetDataSurface call that returns a
> regular SourceSurface but just one where you know Map(READ) will succeed.
> This would actually simplify a lot of the code inside the backends as you'd
> no longer need to have the casts to DataSourceSurface which are currently
> tricky because of the aforementioned WrappedDataSourceSurface stuff. Now
> that I think of it, that is a change we could, and probably should, make
> regardless of whether we have a mutable class or not. At that point if we
> did choose to have a MutableDataSurface type we should probably call it
> MutableSurface (as there's no guarantee, for example with D2D, that it's
> internally a 'raw' data surface).

As soon as MAP_WRITE can fail because a surface is marked as immutable then you need to have two code paths for all code that mutates surfaces. I don't see how that's any different to the patch, except the surfaces share a class name.

One nice part about having separate class types (or separate map functions) is that we can return a const pointer to the data when it's meant to be read only. It's a small bit of type safety, but as the patch shows it would have prevented one bug already.

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> (In reply to Bas Schouten (:bas.schouten) from comment #5)
> > 
> > ModifyMyImage(mutableSourceSurface);
> > snapshot = mutableSourceSurface->Snapshot();
> > dt->DrawImage(Snapshot);
> > 
> > Is just a little uglier,
> 
> It is, but this is a rare case (as you can see from the patches). I don't
> think a bit of uglyness in these rare cases is much of a drawback.

Right, it's not that bad, but since I don't see upsides, I am taking that downside into account :-).

> > I think in general there's just a -lot- of cases
> > where you don't want to care, i.e. it's feasible to have an API where you
> > have an input surface, and you want to -maybe- make some modifications to
> > that. It's nicest if you can input something into that, and -if- it does
> > want to make modifications it will try to MAP_WRITE, and if it fails it will
> > go and create a writable surface and make that copy (guaranteeing the input
> > surface's immutability). But if it succeeds it can just happily keep using
> > it, but at the same time you don't want to always pass in a mutable data
> > sourcesurface, because maybe it -doesn't- need to change it most of the time
> > and then you don't want to needlessly make it mutable. You could of course
> > simply have two function signatures for this but I think this would cause
> > more API clutter.
> 
> Isn't that going to be just as ugly though (or worse)? We need to try
> MAP_WRITE, if that fails than allocate a new surface, MAP_READ the old one
> and MAP_WRITE the new one, do a copy, and then carry on.

Right, but in the rest of the stack we -don't- need to care about the distinction, which I think is valuable. I'd even be okay with having a GetWritableSurface() method that will do the copy-work. It might even return 'this' for surfaces which are already writable, preventing that problem.

> Having two function signatures, where the one that takes the immutable
> source surface just calls SameFunction(aSurface->AsMutable()); seems better
> to me :)

Not really, that assumes you -know- that function or something underneath it will want to mutate it.

> > > > We should note we lose our ability to get a DataSourceSurface snapshot and
> > > > mutate the data in there in-place, we will now always need to make a copy,
> > > > this should usually not matter too much, and in the cases where we need it
> > > > we can probably live with the perf impact of the additional copy, since
> > > > we're doing readback anyway, but it's still wasteful. Maybe we should add a
> > > > GetMutableSourceSurface on SourceSurface to get a copy-on-write version of
> > > > the SourceSurface? It all gets really ugly from there though.
> 
> For both of these we can solve this with:
> 
> MutableSurface *mutable = data->AsMutable();
> data = nullptr;
> // start mutating mutable and no copy should be required.

Ugly, and very error prone, people -will- forget to null data and get stuck with a copy without knowing.

> 
> Or just by having the SnapshotMutable() option on DT as roc suggeagted.

It has the same problem, since they're not inheriting from the same class you can't return the same object if mutability is just fine.

> > I think this is actually more desirable (I'm even fine with that decision
> > being irreversible, so that if you have a codepath where you know if you
> > sent it down that codepath you don't ever want it to change again, you can
> > have the guarantee that callers who try to MAP_WRITE simply fail), at that
> > point also a question actually becomes whether you ever want
> > DataSourceSurface, or whether you just want SourceSurface, and if Map(READ)
> > fails you want to do something like a GetDataSurface call that returns a
> > regular SourceSurface but just one where you know Map(READ) will succeed.
> > This would actually simplify a lot of the code inside the backends as you'd
> > no longer need to have the casts to DataSourceSurface which are currently
> > tricky because of the aforementioned WrappedDataSourceSurface stuff. Now
> > that I think of it, that is a change we could, and probably should, make
> > regardless of whether we have a mutable class or not. At that point if we
> > did choose to have a MutableDataSurface type we should probably call it
> > MutableSurface (as there's no guarantee, for example with D2D, that it's
> > internally a 'raw' data surface).
> 
> As soon as MAP_WRITE can fail because a surface is marked as immutable then
> you need to have two code paths for all code that mutates surfaces. I don't
> see how that's any different to the patch, except the surfaces share a class
> name.

You -could- have a more efficient thing you could do in that case. But really data = data->GetWritableSurface(); is just prettier where data could now be the safe surface, or not, but at least you no longer need to care (if you really do need write access anyway!)

> One nice part about having separate class types (or separate map functions)
> is that we can return a const pointer to the data when it's meant to be read
> only. It's a small bit of type safety, but as the patch shows it would have
> prevented one bug already.

I'm fine with having a second ReadOnlyMappedSurface signature. The caller will know it's only passing MAP_READ and can use that struct and make sure that signature will be called. I might even go so far as to say it's a good idea :-).

Comment 8

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

Here's my summary of where we're at:
-- I think statically indicating mutability of surfaces in their type is a good thing.
-- Bas thinks it's better to have a single SourceSurface type that at runtime can either be immutable or not, so a single piece of code can operate on mutable or immutable surfaces.
Is that a fair summary?

The thing is, so far all the code I've converted (which is all the code currently using Map) statically knows whether its surface(s) are mutable or immutable, or can be rewritten that way with no increase in complexity (and generally a decrease in bugginess). I haven't yet found any code that needs to operate on both mutable and immutable surfaces.

So:
> I think in general there's just a -lot- of cases where you don't want to care, i.e. it's feasible to
> have an API where you have an input surface, and you want to -maybe- make some modifications to that.

What are these cases? I haven't encountered any yet.

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (PTO April 25 to May 3) from comment #8)
> Here's my summary of where we're at:
> -- I think statically indicating mutability of surfaces in their type is a
> good thing.
> -- Bas thinks it's better to have a single SourceSurface type that at
> runtime can either be immutable or not, so a single piece of code can
> operate on mutable or immutable surfaces.
> Is that a fair summary?

Yes, I think it is :).

> The thing is, so far all the code I've converted (which is all the code
> currently using Map) statically knows whether its surface(s) are mutable or
> immutable, or can be rewritten that way with no increase in complexity (and
> generally a decrease in bugginess). I haven't yet found any code that needs
> to operate on both mutable and immutable surfaces.
> 
> So:
> > I think in general there's just a -lot- of cases where you don't want to care, i.e. it's feasible to
> > have an API where you have an input surface, and you want to -maybe- make some modifications to that.
> 
> What are these cases? I haven't encountered any yet.

We haven't converted a lot of layout to use Moz2D directly though :-) I'd prefer to keep focused on designing for the long run, but an example could be:

TemporaryRef<SourceSurface> ConvertToFormatSurface(SourceSurface *aSurface, SurfaceFormat aFormat);

If SourceSurface mutability would be guaranteed, you'd either need a second signature:

TemporaryRef<MutableDataSurface> ConvertToFormatSurfaceMutable(MutableDataSurface *aSurface, SurfaceFormat aFormat);

And then since users probably want a SourceSurface again afterwards they'd be taking Snapshots again of the return value.

Or you'd just have the last version. Now the interesting bit comes when a user has a SourceSurface, they'd probably do:

sourceSurface = ConvertToFormatSurface(sourceSurface, format);

And if there's no format change that api would just immediately return the sourceSurface. However if there -is- a format change that API will create a mutable copy of the source surface, modify the bits, and return an immutable copy again. If the caller goes on working with that, and calls another function which needs to modify the source surface, it'll create a copy -again- even though the caller is now already working with a copy and doesn't care about immutability. Alternatively, the user could do something like:

if (sourceSurf->GetFormat() != format) {
  RefPtr<MutableDataSurface> surf = sourceSurface->GetMutableSurface();

  surf = ConvertToSourceSurfaceMutable(surf, format);
  sourceSurface = surf->Snapshot();
}

But it would face the same repetitive mutation problem unless you maintained two completely separated control flows for mutable and non-mutable.

Now I can already hear your objection: 'But we can take care of wasteful copies by doing copy-on-writes internally'. But that's actually really tricky, because take two subsequent:

sourceSurface = ConvertToFormatSurface(sourceSurface, formatA);

<dostuff with sourceSurface>

sourceSurface = ConvertToFormatSurface(sourceSurface, formatB);

When the second executes, and grabs a mutable copy, the new assignment to sourceSurface hasn't happened yet, and so as far as GetAsMutable is concerned, it -has- to make a copy because somebody still cares about the old sourceSurface. Now of course our caller could take care of that in the other case, like so:


if (sourceSurf->GetFormat() != format) {
  RefPtr<MutableDataSurface> surf = sourceSurface->GetMutableSurface();

  surf = ConvertToSourceSurfaceMutable(surf, formatA);
  sourceSurface = surf->Snapshot();
}
<do stuff with sourceSurface>
if (sourceSurf->GetFormat() != format) {
  RefPtr<MutableDataSurface> surf = sourceSurface->GetMutableSurface();
  sourceSurface = nullptr;

  surf = ConvertToSourceSurfaceMutable(surf, format);
  sourceSurface = surf->Snapshot();
}

But it would get really fiddly and counterintuitive (I mean, do you expect your average user to think about nulling out sourceSurface there?)

All in all I think it makes the optimal way of using the API counterintuitive and offers very little advantage that a simple (irreversible) setMutable function would not offer.

Comment 10

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

I do not like your example for a couple of reasons:
1) You just made it up. I can't think of a single place in our code where we'd want to do multiple format conversions like that. Indeed, we'd try hard for performance reasons to avoid ever converting a surface more than once.
2) ConvertToFormatSurface, as you've written it using SourceSurface, is a dangerous API because it sometimes returns a new surface and sometimes returns the old surface, and these surfaces are generally mutable. That means if someone tries to reuse the old surface in any way, that will sometimes (e.g. on some platforms) work fine and sometimes go horribly wrong. You'd have to document that the old surface can't be used anymore and enforce that during code review. And make sure that whoever created it and passed it along either isn't holding a reference anymore or marked it immutable.

#1 is what really bothers me though. We have no reason to believe this case exists.

Comment 11

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

(In reply to Bas Schouten (:bas.schouten) from comment #9)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (PTO April 25 to
> May 3) from comment #8)
> > So:
> > > I think in general there's just a -lot- of cases where you don't want to care, i.e. it's feasible to
> > > have an API where you have an input surface, and you want to -maybe- make some modifications to that.
> > 
> > What are these cases? I haven't encountered any yet.
> 
> We haven't converted a lot of layout to use Moz2D directly though :-)

We've converted more now. Have any such cases come up?

Comment 12

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> I do not like your example for a couple of reasons:
> 1) You just made it up. I can't think of a single place in our code where
> we'd want to do multiple format conversions like that. Indeed, we'd try hard
> for performance reasons to avoid ever converting a surface more than once.
> 2) ConvertToFormatSurface, as you've written it using SourceSurface, is a
> dangerous API because it sometimes returns a new surface and sometimes
> returns the old surface, and these surfaces are generally mutable. That
> means if someone tries to reuse the old surface in any way, that will
> sometimes (e.g. on some platforms) work fine and sometimes go horribly
> wrong. You'd have to document that the old surface can't be used anymore and
> enforce that during code review. And make sure that whoever created it and
> passed it along either isn't holding a reference anymore or marked it
> immutable.
> 
> #1 is what really bothers me though. We have no reason to believe this case
> exists.

It's not about that exact case though, it's an illustration of why doing this is a bad idea in my opinion.

As for 2), I think that's -exactly- why having it possible to SetImmutable (like for example Skia does), it not a bad idea, i.e. if a caller -wants- to rely on it staying the same, they can do that. If they -don't- want to rely on that, then we're good.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> (In reply to Bas Schouten (:bas.schouten) from comment #9)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (PTO April 25 to
> > May 3) from comment #8)
> > > So:
> > > > I think in general there's just a -lot- of cases where you don't want to care, i.e. it's feasible to
> > > > have an API where you have an input surface, and you want to -maybe- make some modifications to that.
> > > 
> > > What are these cases? I haven't encountered any yet.
> > 
> > We haven't converted a lot of layout to use Moz2D directly though :-)
> 
> We've converted more now. Have any such cases come up?

Jwatt did most of the conversions, I wouldn't know :-).