Closed
Bug 997551
Opened 10 years ago
Closed 10 years ago
Avoid copying in GetSourceSurfaceForSurface if at all possible
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
Details
Attachments
(1 file, 4 obsolete files)
5.26 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
This function often uses DrawTarget::CreateSourceSurfaceFromData which is required (as part of the API contract) to make a copy of the pixel data. A lot of backends don't actually need this (like CG or skia), so it's wasteful. We should use create a data source surface wrapping the data instead (currently only a fallback path) and use OptimizeSourceSurface to give the backend an opportunity to copy if it's beneficial (like it is for uploading to d2d).
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → matt.woodrow
Assignee | ||
Comment 1•10 years ago
|
||
I think this actually makes the code simpler and should improve CG/Skia content rendering performance. https://tbpl.mozilla.org/?tree=Try&rev=61ccf41a259f
Attachment #8407978 -
Flags: review?(bas)
Assignee | ||
Comment 2•10 years ago
|
||
Looks like our code for attaching a cairo snapshot to detect changes to the original surface is a bit broken. I see two options to fix it. The first is this one, which attaches the snapshot to the real surface that was passed in (and the one we attach our normal user data to).
Attachment #8407990 -
Flags: review?(bas)
Assignee | ||
Comment 3•10 years ago
|
||
The other option is to just get rid of it. We already have code to drop our cached Moz2D surface if someone calls MarkDirty [1], so this should handle the imglib case. The only potentially broken case would be if we are actually drawing into the surface (using cairo API), and then converting it into a SourceSurface multiple times. I don't think this actually happens, or this bug would have bitten us properly. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.cpp#284
Attachment #8407991 -
Flags: review?(bas)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8407995 -
Flags: review?(bas)
Comment 5•10 years ago
|
||
Comment on attachment 8407991 [details] [diff] [review] Fix snapshots option b Review of attachment 8407991 [details] [diff] [review]: ----------------------------------------------------------------- Let's do this if it works.
Attachment #8407991 -
Flags: review?(bas) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8407978 [details] [diff] [review] Avoid copying if possible in GetSourceSurfaceForSurface Review of attachment 8407978 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatform.cpp @@ +758,5 @@ > + // If the image data is already available, then just wrap that data (which > + // doesn't require copies) and let the backend decide if it wants to > + // optimize further. > + RefPtr<SourceSurface> wrapped = GetWrappedDataSourceSurface(aSurface); > + RefPtr<SourceSurface> opt = aTarget->OptimizeSourceSurface(wrapped); I'm inclined to say we should return the data surface unconditionally rather than doing this. That, or we should be caching the result, OptimizeSourceSurface is intended to do 'more work than bare minimum' because it assumes the surface will be used multiple times. You're making the assumption here imgSurface isn't required to stay alive for the data to be valid. That could be true for us, but we should document that -very- carefully. If it -is- true, we can safely cache the optimized surface and should be doing that :). Couldn't we write this whole thing as follows: if (!srcBuffer) { nsRefPtr<gfxImageSurface> imgSurface = aSurface->GetAsImageSurface(); RefPtr<DataSourceSurface> dataSurf; if (imgSurface) { dataSurf = GetWrappedDataSourceSurface(aSurface); } else { dataSurf = CopySurface(aSurface); } if (!dataSurf) { return nullptr; } srcBuffer = aTarget->OptimizeSourceSurface(dataSurf); }
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8407978 -
Attachment is obsolete: true
Attachment #8407990 -
Attachment is obsolete: true
Attachment #8407991 -
Attachment is obsolete: true
Attachment #8407995 -
Attachment is obsolete: true
Attachment #8407978 -
Flags: review?(bas)
Attachment #8407990 -
Flags: review?(bas)
Attachment #8407995 -
Flags: review?(bas)
Attachment #8412342 -
Flags: review?(bas)
Updated•10 years ago
|
Attachment #8412342 -
Flags: review?(bas) → review+
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eba5b89f831
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7eba5b89f831
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
Target Milestone: mozilla32 → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•