Avoid copying in GetSourceSurfaceForSurface if at all possible

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

29 Branch
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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)
Posted 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)
Posted 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)
Posted 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);
}
Posted 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Target Milestone: mozilla32 → mozilla31
Depends on: 1004437
You need to log in before you can comment on or make changes to this bug.