975900 - Convert imgUtils to Moz2D (from gfxASurface to SourceSurface)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

Converting imgUtils to Moz2D (from gfxASurface to SourceSurface) will help ease the path to making imgIContainer::GetFrame return a SourceSurface.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: WIP

Comment 2

[Jonathan Watt [:jwatt]]

I've been having trouble getting this to pass Try, although I'm now down to only a couple of xpcshell test files failing. Specifically:

xpcshell/tests/image/test/unit/test_imgtools.js
xpcshell/tests/toolkit/components/places/tests/favicons/test_favicons_conversions.js

https://tbpl.mozilla.org/?tree=Try&rev=211a6a4235a7

Focusing on the first failure in test_imgtools.js, it seems that the anti-aliasing when down scaling using imgTools::EncodeScaledImage is wrong. I'll attach some screenshots of the differences shortly, but in summary I've tried passing every permutation of DrawSurfaceOptions and DrawOptions arguments to DrawSurface.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: reference (zoomed 20x)

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: default DrawSurface args (scaled 20x)

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: passing DrawSurfaceOptions(Filter::GOOD) (scaled 20x)

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8380403 [details] [diff] [review]
WIP

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

::: image/src/imgTools.cpp
@@ +104,5 @@
> +  aContainer->GetFrame(imgIContainer::FRAME_FIRST,
> +                       imgIContainer::FLAG_SYNC_DECODE);
> +  NS_ENSURE_TRUE(frame, nullptr);
> +
> +  return gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(nullptr, frame);

The old code both made a copy, and potentially converted to RGBA32. Why don't we need to that anymore?

@@ +116,5 @@
> + */
> +static nsresult EncodeImageData(SourceSurface* aSurface,
> +                                const nsACString& aMimeType,
> +                                const nsAString& aOutputOptions,
> +                                nsIInputStream **aStream)

Comment doesn't match the actual parameters.

@@ +201,5 @@
>  
> +  RefPtr<DrawTarget> dt =
> +    gfxPlatform::GetPlatform()->CreateOffscreenContentDrawTarget(
> +                                          IntSize(aScaledWidth, aScaledHeight),
> +                                          SurfaceFormat::B8G8R8A8);

Previously the code was using a gfxImageSurface (which guarantees pixman as the rasterizer), and now we're using the platform default rasterizer.

The failing tests are comparing against static png images, so changing which engine does the scaling will almost certainly causes the failures you're seeing.

Given that we know that we need the data in system memory, it's probably best to stick with wrapping a gfxImageSurface (since d2d will upload everything, and force a readback when we call GetDataSurface).

Alternatively you could add a way to specify 'i want the best surface for this platform, but it needs to be optimized for readback'. You'd then also need to modify the test to have per-platform reference images, and generate new references images as needed.

@@ +205,5 @@
> +                                          SurfaceFormat::B8G8R8A8);
> +  dt->DrawSurface(frame,
> +                  Rect(0, 0, aScaledWidth, aScaledHeight),
> +                  Rect(0, 0, frameWidth, frameHeight),
> +                  DrawSurfaceOptions(Filter::GOOD, SamplingBounds::BOUNDED),

BOUNDED isn't actually implemented in any of the backends except D2D1.1 (which isn't turned on yet)

@@ +258,4 @@
>  
> +  RefPtr<DrawTarget> dt =
> +    gfxPlatform::GetPlatform()->CreateOffscreenContentDrawTarget(
> +                                                      frame->GetSize(),

This should be aWidth/aHeight, not the frame size, right?

@@ +263,5 @@
> +  dt->DrawSurface(frame,
> +                  Rect(0, 0, aWidth, aHeight),
> +                  Rect(aOffsetX, aOffsetX, aWidth, aHeight),
> +                  DrawSurfaceOptions(),
> +                  DrawOptions(1.0f, CompositionOp::OP_SOURCE));

I think you can use CopySurface here, since there shouldn't be any scaling happening.

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> The old code both made a copy, and potentially converted to RGBA32. Why
> don't we need to that anymore?

I'm not sure why we'd need to copy. The RGBA32 thing is an issue in the imgTools::EncodeImage case by the looks of it, but not in the case of EncodeScaledImage/EncodeCroppedImage, since the drawing into a B8G8R8A8 DT/SS should handle that I'd think. I'll make GetFirstImageFrame convert if necessary though.

> Comment doesn't match the actual parameters.

It will in the next patch.

> Previously the code was using a gfxImageSurface (which guarantees pixman as
> the rasterizer), and now we're using the platform default rasterizer.
> 
> The failing tests are comparing against static png images, so changing which
> engine does the scaling will almost certainly causes the failures you're
> seeing.
> 
> Given that we know that we need the data in system memory, it's probably
> best to stick with wrapping a gfxImageSurface (since d2d will upload
> everything, and force a readback when we call GetDataSurface).

I'd rather stay away from Thebes stuff, so I'll use a BackendType::CAIRO DT instead.

> BOUNDED isn't actually implemented in any of the backends except D2D1.1
> (which isn't turned on yet)

Just cruft from my permutation testing.

> This should be aWidth/aHeight, not the frame size, right?

Yeah, xpcshell tests failed because of this. I was just getting your general feedback on the patch. Will be fixed in next patch.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 9

[Jonathan Watt [:jwatt]]

Unfortunately this still fails the xpcshell tests on Windows:

https://tbpl.mozilla.org/?tree=Try&rev=09e79c2ac993

The problem seems to be that the use of the GetSourceSurfaceForSurface call in GetFirstImageFrame results in the pixel data changing slightly, which seems like a gfx bug to me. After all the copying is between pixel aligned surfaces of the same size, so it doesn't seem like the fact that GetSourceSurfaceForSurface forces a surface upload to D2D should matter.

Comment 10

[Jonathan Watt [:jwatt]]

Using gfxImageSurface::CopyTo(SourceSurface) seems to avoid the problem, but it does require calling CopyToARGB32ImageSurface to get a gfxImageSurface. I'll do that for now, but that just means punting on the issue of the pixel data changing.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: patch with changes to GetFirstImageFrame

This passes Try:

https://tbpl.mozilla.org/?tree=Try&rev=c756ed5e713e

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8380758 [details] [diff] [review]
patch with changes to GetFirstImageFrame

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

::: image/src/imgTools.cpp
@@ +113,5 @@
> +                                             imageSurface->GetSize().height),
> +                                     SurfaceFormat::B8G8R8A8);
> +  NS_ENSURE_TRUE(dataSurface, nullptr);
> +
> +  imageSurface->CopyTo(dataSurface);

How about adding gfxImageSurface::CopyToARB32DataSurface() to do both these steps with only a single copy?

Comment 13

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a109eb551fc

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a109eb551fc
https://hg.mozilla.org/mozilla-central/rev/9f28160c038f