979853 - Convert the Windows widget consumers of imgIContainer::GetFrame to act on a Moz2D SourceSurface instead of a Thebes gfxASurface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

As part of converting imgIContainer::GetFrame to return a Moz2D SourceSurface instead of a Thebes gfxASurface, we should convert the Windows widget consumers of imgIContainer::GetFrame to act on a Moz2D SourceSurface instead of a Thebes gfxASurface.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8386067 [details] [diff] [review]
patch

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

::: widget/windows/nsImageClipboard.cpp
@@ +129,5 @@
> +      thebesSurface->GetAsReadableARGB32ImageSurface();
> +    NS_ENSURE_TRUE(thebesImageSurface, NS_ERROR_FAILURE);
> +
> +    RefPtr<DataSourceSurface> dataSurface =
> +      thebesImageSurface->CopyToB8G8R8A8DataSourceSurface();

Should use Factory::CreateWrappingDataSurface here instead of copying.

::: widget/windows/nsWindowGfx.cpp
@@ +688,5 @@
> +  } else {
> +    dataSurface = surface->GetDataSurface();
> +    dataSurface->Map(DataSourceSurface::MapType::READ, &map);
> +  }
> +  NS_ENSURE_TRUE(dataSurface && map.mData, NS_ERROR_FAILURE);

map.mData is probably uninitialized if Map() failed. We should check that return value instead.

@@ +693,5 @@
> +  MOZ_ASSERT(dataSurface->GetFormat() == SurfaceFormat::B8G8R8A8);
> +
> +  uint8_t* data = nullptr;
> +  nsAutoArrayPtr<uint8_t> autoDeleteArray;
> +  if (map.mStride == 4 * sizeof(uint8_t) * iconSize.width) {

gfx/2d/Tools.h has BytesPerPixel(SurfaceFormat) for this.

Comment 4

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e34e33e9ee

Comment 5

[Carsten Book [:Tomcat]]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/22e34e33e9ee

hi sorry had to backout this change for win7/8 debug crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=35715170&tree=Mozilla-Inbound

Comment 6

[Jonathan Watt [:jwatt]]

This is because I added a "missing" Unmap() call and we now hit the MOZ_ASSERT(mIsMapped) in DataSourceSurfaceD2D::Unmap(). It seems DataSourceSurfaceD2D::Map doesn't actually set mIsMapped, so I don't see how this can work anywhere else in the codebase unless all those calls are either (a) not on a DataSourceSurfaceD2D DataSourceSurface, or (b) the Unmap() calls are missing. I've noticed that the latter is the case in some places (Unmap calls are missing).

In other words I seem to have stumbled upon an existing DataSourceSurfaceD2D::Map bug, and some existing Unmap-missing bugs.

Comment 7

[Jonathan Watt [:jwatt]]

Relanded, now that bug 980272 is fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4efacf2f947b

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4efacf2f947b