985545 - Add a helper method to convert SourceSurfaces to a given format

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Bas doesn't want this in Moz2D, so I've put it in gfxUtils.

Comment 2

[Bas Schouten (:bas.schouten)]

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).

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

(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.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Bas Schouten (:bas.schouten)]

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.

Comment 6

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd9f799fd5a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bd9f799fd5a