Remove many calls to DataSourceSurface::GetData

RESOLVED FIXED in Firefox 58



2 years ago
2 years ago


(Reporter: dvander, Assigned: dvander)


40 Branch
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 fixed)


(Whiteboard: [gfx-noted])


(6 attachments, 1 obsolete attachment)

DataSourceSurface::GetData automatically maps a surface and does not require the caller to unmap it. OMTP has added mutexes to DataSourceSurface, so now this API must go.

Unfortunately it has a *lot* of callers, the worst of which is GetSkImageFromSurface. We have to bend over backwards to return SkImages without having the underlying data get freed on return. This function was also causing problems for bug 1395478, so I think it's time to change it.
Jeff, the goal of these patches is to remove DataSourceSurface::GetData calls and replace them with properly scoped Map/Unmap calls.
Attachment #8914875 - Flags: review?(jgilbert)
This should work in theory, but I need to run it through try to see if the scoping conflicts with map calls outside DrawTargetSkia.
Attachment #8914906 - Flags: review?(bas)
Comment on attachment 8914875 [details] [diff] [review]
part 2, fix cases in gl/canvas

Review of attachment 8914875 [details] [diff] [review]:

If you're removing GetData(), shouldn't you also remove Stride()?

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +6000,4 @@
>      if (!dstData) {
>        return NS_ERROR_OUT_OF_MEMORY;
>      }
>      dstStride = sourceSurface->Stride();

map.mStride, surely?

::: gfx/gl/GLUploadHelpers.cpp
@@ +535,1 @@
>      int32_t stride = aSurface->Stride();

Attachment #8914875 - Flags: review?(jgilbert) → review+
Good point, I missed that since I was only looking at Skia.
Attachment #8914827 - Flags: review?(bas) → review+
Comment on attachment 8914906 [details] [diff] [review]
part 3, fix GetSkImageForSurface

not working yet
Attachment #8914906 - Flags: review?(bas)
These are cases with trivial calls to GetStride, mostly where a mapping already existed.
Attachment #8914983 - Flags: review?(bas)
Attachment #8914906 - Attachment is obsolete: true
Attachment #8914983 - Flags: review?(bas) → review+
This is wrapped in Maybe<> so it's easier to construct the ScopedMap.
Attachment #8919167 - Flags: review?(bas)
Separate patch since mapping to get the stride seems heavyweight, but I wasn't sure if there's a better option.
Attachment #8919168 - Flags: review?(bas)
Using Maybe<> here just makes it easier to change which surface is mapped.
Attachment #8919169 - Flags: review?(jgilbert)
Attachment #8919169 - Flags: review?(jgilbert) → review+
Attachment #8919167 - Flags: review?(bas) → review+
Comment on attachment 8919168 [details] [diff] [review]
part 5, fix ImageUtils.cpp

Review of attachment 8919168 [details] [diff] [review]:

It's important to note here that the reason there's no better options, is that subsequent maps have no guarantee to have the same stride.. a surface as such does not have a stride, it has things like a width, a height and a format. The stride is an attribute of the mapping of that surface into system memory, it should be rare for that to change, obviously, and I don't think this happens often in practice, but in principle that's the way it is.
Attachment #8919168 - Flags: review?(bas) → review+
Summary: Remove DataSourceSurface::GetData → Remove many calls to DataSourceSurface::GetData
I'd like to land these patches without having to go through the last and most difficult bits, so renaming the bug.

Comment 13

2 years ago
Pushed by
Remove easily-removed cases of DataSourceSurface::GetData(). (bug 1405390 part 1, r=bas)
Fix canvas and GL uses of DataSourceSurface::GetData. (bug 1405390 part 2, r=jgilbert)
Remove trivial calls to DataSourceSurface::Stride. (bug 1405390 part 3, r=bas)
Fix DataSourceSurface mapping in PaintCounter. (bug 1405390 part 4, r=bas)
Fix DataSourceSurface mapping in ImageUtils.cpp. (bug 1405390 part 5, r=bas)
Fix DataSourceSurface mapping in GLReadTexImageHelper.cpp. (bug 1405390 part 6, r=jgilbert)
You need to log in before you can comment on or make changes to this bug.