Closed Bug 997551 Opened 7 years ago Closed 7 years ago

Avoid copying in GetSourceSurfaceForSurface if at all possible

Categories

(Core :: Graphics: Layers, defect)

29 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file, 4 obsolete files)

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: nobody → matt.woodrow
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)
Attached patch Fix snapshot attaching (obsolete) — Splinter Review
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)
Attached patch Fix snapshots option b (obsolete) — Splinter Review
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)
Attached patch Cleanup some dead code (obsolete) — Splinter Review
Attachment #8407995 - Flags: review?(bas)
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 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);
}
Attached patch Avoid copiesSplinter Review
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)
Attachment #8412342 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/7eba5b89f831
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Target Milestone: mozilla32 → mozilla31
You need to log in before you can comment on or make changes to this bug.