I want to pass a DataSourceSurface that was created by Factory::CreateDataSourceSurface to DrawTargetCG::DrawSurface, but at the moment it assumes that all data source surfaces it gets are DataSourceSurfaceCG instances. I'm not sure where CreateCGImage should live, do you have a better idea?

(In reply to Markus Stange [:mstange] from comment #0) > Created attachment 822248 [details] [diff] [review] > drawtargetcg-drawsurface-datasourcesurface > > I want to pass a DataSourceSurface that was created by > Factory::CreateDataSourceSurface to DrawTargetCG::DrawSurface, but at the > moment it assumes that all data source surfaces it gets are > DataSourceSurfaceCG instances. > > I'm not sure where CreateCGImage should live, do you have a better idea? What is the lifetime of the surface you're creating? Will it stay around for a long time?

I will destroy it right after the DrawSurface call.

(In reply to Markus Stange [:mstange] from comment #2) > I will destroy it right after the DrawSurface call. Out of curiosity what is the surface for?

It's the result from software filter rendering: http://hg.mozilla.org/users/bschouten_mozilla.com/moz2d/file/4374a3915f04/FilterNodeSoftware.cpp#l713 DrawTargetCG::DrawFilter calls FilterNodeSoftware::Draw with itself as the target.

The aSurface->GetType() == SURFACE_DATA check was actually counterproductive and unnecessary. We can just call GetDataSurface instead - if the surface already is a DataSourceSurface, this will give the same result. And more importantly, it will also work for DataSourceSurfaceCairo/Skia which don't return SURFACE_DATA from GetType().

Comment on attachment 8334686 [details] [diff] [review] better patch Review of attachment 8334686 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetCG.cpp @@ +1185,5 @@ > * - fancy things with clip and different dest rects */ > { > subimage = CGImageCreateWithImageInRect(image, IntRectToCGRect(aSourceRect)); > + CGImageRelease(image); > + image = CGImageRetain(subimage); Why do we need this additional retain and release? Since this is called unconditionally it seems to me like we could lose the release down below and this retain?

True enough. I've now removed the image/subimage swapping and the braces in order to make the code less confusing.

Comment on attachment 8336311 [details] [diff] [review] even better patch Review of attachment 8336311 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, let's make sure it doesn't break anything though!

Wait, I'm having second thoughts here. When the source surface is not a DataSourceSurface, and when calling GetDataSurface does not just rewrap the existing data storage, the data that we initialize the CGImage with will go away as soon as the DataSourceSurface goes out of scope when we return from GetRetainedImageFromSourceSurface. Is that a problem? We could just abort when we hit that case. But detecting it is the tricky part - Skia and Cairo DataSourceSurfaces do not return SURFACE_DATA from GetType(). Should they? But we can probably detect it like this: RefPtr<DataSourceSurface> dataSource = aSurface->GetDataSurface(); if (dataSource != aSurface) { MOZ_CRASH("aSurface needs to be a DataSourceSurface."); } What do you think?

I'd say CGImageCreateWithImageInRect grabs a reference or copies to what it needs. Since we're using what -that- returns and releasing only the input we gave it. It seems unlikely that the object that function returns would rely on the object being passed in being kept around.

The problem I'm concerned about arises before the call to CGImageCreateWithImageInRect, in the call to GetRetainedImageFromSourceSurface: static CGImageRef GetRetainedImageFromSourceSurface(SourceSurface *aSurface) { if (aSurface->GetType() == SURFACE_COREGRAPHICS_IMAGE) return CGImageRetain(static_cast<SourceSurfaceCG*>(aSurface)->GetImage()); else if (aSurface->GetType() == SURFACE_COREGRAPHICS_CGCONTEXT) return CGImageRetain(static_cast<SourceSurfaceCGContext*>(aSurface)->GetImage()); RefPtr<DataSourceSurface> dataSource = aSurface->GetDataSurface(); // <---- Data could be allocated here return CreateCGImage(nullptr, dataSource->GetData(), dataSource->GetSize(), dataSource->Stride(), dataSource->GetFormat()); // <--- and freed here }

With the crash added.

Comment on attachment 8336377 [details] [diff] [review] refuse to draw non-CG non-DataSourceSurfaces Review of attachment 8336377 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetCG.cpp @@ +220,5 @@ > + return CGImageRetain(static_cast<SourceSurfaceCGContext*>(aSurface)->GetImage()); > + > + RefPtr<DataSourceSurface> dataSource = aSurface->GetDataSurface(); > + if (dataSource != aSurface) { > + MOZ_CRASH("Non-CG SourceSurfaces need to be DataSourceSurfaces.") Why's this? Can't we just check for !dataSource?

Well, no, due to the problem I tried to explain in comment 9 and comment 11. Let's say aSurface is a SourceSurfaceD2D. That's impossible, but please hear me out. Calling GetDataSurface() on that will give us a DataSourceSurfaceD2D, and calling GetData() on that will map its texture data into its mData field. Then we initialize the CGImage with a pointer to that data - CreateCGImage does not make a copy of the data, and we do not want it to. Then, at the end of GetRetainedImageFromSourceSurface, dataSource goes out of scope, the DataSurfaceD2D is destroyed, its mapped data is freed, and the CGImage refers to freed data.

Even if it didn't crash, I don't think we want SourceSurfaces from a different backend to implicitly work, so this is a double win imo :)

So, let me describe the scenario I actually care about: In bug 924102 I'm adding a transform filter, which is implemented in the software filters by painting into a DrawTargetCairo, no matter what backend actually created the FilterNode. If this is the final filter in the filter chain, we want to draw a snapshot of that DrawTarget directly into the DrawTargetCG that we called DrawFilter on. Doing drawTargetCG->DrawSurface(drawTargetCairo->Snapshot()->GetDataSurface()) can do exactly that, with the patch in this bug.

The problem I see with this, is that it exposes a slight inconsistency. Some backends return a DataSourceSurface from Snapshot() directly (CG and skia both do at least). So for these examples you could omit the GetDataSurface(), but for others, like d2d, you couldn't. I guess the decision is, how strongly do we want to discourage drawing between backends? Option a: We allow drawing of any DataSourceSurface to any destination (even if it's a skia source surface being drawn to CG). It has the above inconsistency, but there aren't any performance pitfalls (like readback). If you wanted to draw D2D into cairo, then you'd have to call GetDataSurface(), which makes the readback cost explict(ish). Option b: We don't allow any cross-backend drawing. The only surfaces you can draw are ones from the same backend, or SourceSurfaceRawData. This is more consistent, but maybe it's unnecessarily restrictive. For Markus' example above he would need to get the DataSourceSurface for his cairo source, and then re-wrap the pixels using CreateWrappingDataSourceSurface to get a backend independent SourceSurface. Very very verbose and explicit, but maybe that's a good thing. I'm also not sure if we have the API's to actually implement this, but that could be fixed.

Having the callers of DrawSurface call CreateWrappingDataSourceSurface first sounds like the simplest solution. I'm going to add a patch in bug 924102 that does that. Then we can bring back the GetType() == SURFACE_DATA check and the cast to DataSourceSurface here, and all is fine.

without unnecessary RefPtr

(In reply to Matt Woodrow (:mattwoodrow) from comment #17) > The problem I see with this, is that it exposes a slight inconsistency. > > Some backends return a DataSourceSurface from Snapshot() directly (CG and > skia both do at least). So for these examples you could omit the > GetDataSurface(), but for others, like d2d, you couldn't. > > I guess the decision is, how strongly do we want to discourage drawing > between backends? > > Option a: We allow drawing of any DataSourceSurface to any destination (even > if it's a skia source surface being drawn to CG). It has the above > inconsistency, but there aren't any performance pitfalls (like readback). If > you wanted to draw D2D into cairo, then you'd have to call GetDataSurface(), > which makes the readback cost explict(ish). > > Option b: We don't allow any cross-backend drawing. The only surfaces you > can draw are ones from the same backend, or SourceSurfaceRawData. This is > more consistent, but maybe it's unnecessarily restrictive. > > For Markus' example above he would need to get the DataSourceSurface for his > cairo source, and then re-wrap the pixels using > CreateWrappingDataSourceSurface to get a backend independent SourceSurface. > Very very verbose and explicit, but maybe that's a good thing. > > I'm also not sure if we have the API's to actually implement this, but that > could be fixed. You always have to use GetDataSurface() and just send that in, any DrawTarget should be able to deal with DataSourceSurface. If it doesn't than that's a bug. Obviously you can't just blindly send a Snapshot in.

Do note GetDataSurface() is simply implemented as 'return this;' on DataSourceSurface, so there's no cost involved.

(In reply to Bas Schouten (:bas.schouten) from comment #21) > You always have to use GetDataSurface() and just send that in, any > DrawTarget should be able to deal with DataSourceSurface. If you add "with GetType() == SURFACE_DATA" to this sentence then I agree. Otherwise this patch won't quite get us there. I'm still not sure if I was able to get this part across: If you call GetDataSurface on a cairo source surface, the resulting DataSourceSurface will have GetType() == SURFACE_CAIRO. Same for Skia and CG.

(In reply to Bas Schouten (:bas.schouten) from comment #21) > > You always have to use GetDataSurface() and just send that in, any > DrawTarget should be able to deal with DataSourceSurface. If it doesn't than > that's a bug. Obviously you can't just blindly send a Snapshot in. I agree that you *shouldn't* blindly send a Snapshot in, but if the source DT was skia then you *can*, and it's correct and working code. Until someone flips the pref to enable skia-gl, and suddenly it's completely bogus code that crashes. That seems like a trap, and one that is easily avoided if we just refuse all cross-backend drawing, and instead require using CreateWrappingDataSourceSurface.