950647 - Make nsImageToPixbuf::ImgSurfaceToPixbuf act on a Moz2D SourceSurface instead of a Thebes gfxASurface

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Jonathan Watt [:jwatt]]

As part of converting imgIContainer::GetFrame to return a Moz2D SourceSurface instead of a Thebes gfxASurface, we should make nsImageToPixbuf::ImgSurfaceToPixbuf act on a Moz2D SourceSurface instead of a Thebes gfxASurface.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Hmm, actually I think I was going to put back the aWidth/aHeight args since I didn't convince myself that nsDragService::SetDragIcon didn't need to pass them. Anyway, if you can take a look at the rest.

Comment 3

[Matt Woodrow (:mattwoodrow)]

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

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

::: widget/gtk/nsImageToPixbuf.cpp
@@ +73,5 @@
>  
>      uint32_t rowstride = gdk_pixbuf_get_rowstride (pixbuf);
>      guchar* pixels = gdk_pixbuf_get_pixels (pixbuf);
>  
> +    nsAutoArrayPtr<uint8_t> bits(SurfaceToPackedBGRA(aSurface));

Why do we need to use SurfaceToPackedBGRA? This makes a copy of all the data unnecessarily. We should just be able to use aSurface->GetDataSurface().

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Why do we need to use SurfaceToPackedBGRA? This makes a copy of all the data
> unnecessarily. We should just be able to use aSurface->GetDataSurface().

This is a non-perf critical path, so it seemed to me that rather than have branches in the code for each possible SourceSurface format, it was just cleaner to convert to an RGBA array.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

I've not tested this yet and may well need to tweak things to get the components in the right order or whatever, but this should be close enough to let you proceed. I'll create a Linux build, test and tweak as necessary tomorrow.

Comment 6

[Matt Woodrow (:mattwoodrow)]

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

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

::: widget/gtk/nsImageToPixbuf.cpp
@@ +8,5 @@
>  #include "gfxASurface.h"
>  #include "gfxImageSurface.h"
>  #include "gfxContext.h"
> +#include "gfxPlatform.h"
> +//#include "mozilla/gfx/DataSurfaceHelpers.h"

Get rid of this :)

@@ +53,5 @@
> +    NS_ENSURE_TRUE(thebesSurface, nullptr);
> +
> +    RefPtr<SourceSurface> surface =
> +      gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(
> +        gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget(), thebesSurface);

You can pass nullptr for the first param here and the impl will grab the screen reference DT.

@@ +69,4 @@
>  {
> +    MOZ_ASSERT(aWidth <= aSurface->GetSize().width &&
> +               aHeight <= aSurface->GetSize().height,
> +               "Requested rect is bigger than the supplied surface");

I'm at least 87.6% sure that these sizes are always the same (gfxASurface never used to have a guaranteed way of retrieving its size).

I think you could move this assertion into the gfxASurface version, and make it assert that they are identical.

And then not bother passing the aWidth/aHeight parameters at all.

::: widget/gtk/nsImageToPixbuf.h
@@ +36,5 @@
>          static GdkPixbuf* SurfaceToPixbuf(gfxASurface* aSurface,
>                                            int32_t aWidth, int32_t aHeight);
>      private:
> +        static GdkPixbuf* SourceSurfaceToPixbuf(SourceSurface* aSurface,
> +                                                int32_t aWidth, int32_t aHeight);

Make this public, since I want to access it from nsDragService :)

Comment 7

[Carsten Book [:Tomcat]]

sorry had to backout this change from inbound because of linux bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=33453715&tree=Mozilla-Inbound

Comment 8

[Jonathan Watt [:jwatt]]

Pushed:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c71a784e33

after testing on Fedora on:

  https://jwatt.org/svg/demos/custom-cursor.svg
  https://jwatt.org/svg/tests/custom-cursor-via-css.svg

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> @@ +69,4 @@
> >  {
> > +    MOZ_ASSERT(aWidth <= aSurface->GetSize().width &&
> > +               aHeight <= aSurface->GetSize().height,
> > +               "Requested rect is bigger than the supplied surface");
> 
> I'm at least 87.6% sure that these sizes are always the same (gfxASurface
> never used to have a guaranteed way of retrieving its size).
> 
> I think you could move this assertion into the gfxASurface version, and make
> it assert that they are identical.
> 
> And then not bother passing the aWidth/aHeight parameters at all.

When it comes to nsDragService::SetDragIcon it's a bit of a headache to walk back up the caller tree making sure that the size is always a match to the surface size, so I punted on this. I think the removal of aWidth/aHeight and the addition of an assertion would be better as a follow-up.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Carsten Book [:Tomcat] from comment #7)
> sorry had to backout this change from inbound because of linux bustages like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33453715&tree=Mozilla-
> Inbound

Darn unified builds. Thanks for backing it out.

Comment 10

[Jonathan Watt [:jwatt]]

Fixed includes and passed Try, so pushed again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d65e90d4c2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1d65e90d4c2