Closed Bug 985545 Opened 6 years ago Closed 6 years ago

Add a helper method to convert SourceSurfaces to a given format

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

We have a bunch of places where we want to access the pixel data of a SourceSurface. However, the pixel data is useless without code written to specifically handle the pixel format. We don't want each caller of SourceSurface::GetDataSurface() to have to have its own code for handling all the various formats that it may encounter. Better is to reduce code complexity (and duplication) by having a helper that converts surfaces to a given format so that the caller only needs to handle that one format.
Attached patch patch (obsolete) — Splinter Review
Bas doesn't want this in Moz2D, so I've put it in gfxUtils.
Assignee: nobody → jwatt
Attachment #8393596 - Flags: review?(bas)
Blocks: 985477
Comment on attachment 8393596 [details] [diff] [review]
patch

Review of attachment 8393596 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUtils.cpp
@@ +901,5 @@
> +  if (!dt) {
> +    dataSurface->Unmap();
> +    return nullptr;
> +  }
> +  dt->CopySurface(aSurface, IntRect(IntPoint(0, 0), aSurface->GetSize()),

There's no guarantee a Cairo dt will know how to deal with aSurface, you'll need to pass aSurface->GetDataSurface() (or rather, assign it to a temp and pass that). There' also no guarantee CopySurface will do the content conversion, DrawSurface would be a safer bet (CopySurface is meant to be optimized with memcpy's).
Attachment #8393596 - Flags: review?(bas) → review-
Attached patch patch (obsolete) — Splinter Review
(In reply to Bas Schouten (:bas.schouten) from comment #2)
> There's no guarantee a Cairo dt will know how to deal with aSurface, you'll
> need to pass aSurface->GetDataSurface()

DrawTargetCairo::DrawSurface() already handles that, since it calls GetCairoSurfaceForSourceSurface() and that handles the case that it needs to be converted to a DataSourceSurface.

> There' also no guarantee CopySurface will do the content
> conversion, DrawSurface would be a safer bet (CopySurface is meant to be
> optimized with memcpy's).

Fixed.
Attachment #8393596 - Attachment is obsolete: true
Attachment #8400900 - Flags: review?(bas)
Attached patch patchSplinter Review
Attachment #8400900 - Attachment is obsolete: true
Attachment #8400900 - Flags: review?(bas)
Attachment #8401022 - Flags: review?(bas)
Comment on attachment 8401022 [details] [diff] [review]
patch

Review of attachment 8401022 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUtils.cpp
@@ +856,5 @@
> +{
> +  MOZ_ASSERT(aFormat != aSurface->GetFormat(),
> +             "Unnecessary - and very expersive - surface format conversion");
> +
> +  Rect bounds(0, 0, aSurface->GetSize().width, aSurface->GetSize().height);

It should probably be noted there's many format conversions this function won't support.

@@ +861,5 @@
> +
> +  if (aSurface->GetType() != SurfaceType::DATA) {
> +    // If the surface is NOT of type DATA then its data is not mapped into main
> +    // memory. Format conversion is probably faster on the GPU, and by doing it
> +    // there we can return the resulting SourceSurface without having done any

It seems this comment is outdated and you're now always returning a DataSourceSurface.

@@ +868,5 @@
> +      CreateOffscreenContentDrawTarget(aSurface->GetSize(), aFormat);
> +    // Using DrawSurface() here rather than CopySurface() because CopySurface is
> +    // optimized for memcpy and therefore isn't good for format conversion.
> +    dt->DrawSurface(aSurface, bounds, bounds, DrawSurfaceOptions(),
> +                    DrawOptions(1.0f, CompositionOp::OP_SOURCE));

OP_SOURCE is going to be slow on D2D probably, probably best to just do OVER since it's equivelant here.
Attachment #8401022 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/2bd9f799fd5a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.