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

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks 2 bugs)

Trunk
mozilla29
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8348004 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

5 years ago
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 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().
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
Posted patch patchSplinter Review
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.
Attachment #8348004 - Attachment is obsolete: true
Attachment #8348004 - Flags: review?(matt.woodrow)
Attachment #8364022 - Flags: review?(matt.woodrow)
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 :)
Attachment #8364022 - Flags: review?(matt.woodrow) → review+
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
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
Fixed includes and passed Try, so pushed again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1d65e90d4c2
https://hg.mozilla.org/mozilla-central/rev/c1d65e90d4c2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.