1247152 - Use SurfacePipe in the GIF decoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

In this bug we'll modify the GIF decoder to use SurfacePipe, a safer API for write image decoder output to surfaces. (Please see bug 1246851 for much more details about SurfacePipe's design and motivation.)

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Use SurfacePipe in the GIF decoder.

This patch converts the GIF decoder to use SurfacePipe. In particular the LZW
code is now implemented as a generator that gets passed to WritePixels.

As usual, some complexity here will be eliminated in the near future when
support for paletted surfaces gets removed.

Because DeinterlacingFilter replaces Deinterlacer, we can totally remove
Deinterlacer in this patch. We're also able to remove a lot of state from the
LZW state machine and the GIF decoder because SurfacePipe handles that
responsibility now. We can't remove Downscaler yet (because it's a member of
Decoder), but we'll be able to once all of the Decoder implementations are
converted to use SurfacePipe.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: Use SurfacePipe in the GIF decoder.

This updated version of the patch is rebased against the latest version of bug
1246851.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8718191 [details] [diff] [review]
Use SurfacePipe in the GIF decoder.

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

Consider this a very weak r+. Not because I have problems with the patch, but because I'm not at all familiar with the GIF decoder. If there is anyone else familiar with the GIF decoder you should strongly consider asking them for an additional review.

I do give two thumbs up to this:

> 7 files changed, 186 insertions(+), 511 deletions(-)

::: image/decoders/nsGIFDecoder2.cpp
@@ +207,5 @@
> +    // This is the first frame. We may be downscaling, so compute the target
> +    // size and target frame rect.
> +    IntSize targetSize = mDownscaler ? mDownscaler->TargetSize()
> +                                     : GetSize();
> +    IntRect targetFrameRect = mDownscaler ? IntRect(IntPoint(), targetSize)

Clearer as IntPoint(0, 0)?

Comment 4

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Consider this a very weak r+. Not because I have problems with the patch,
> but because I'm not at all familiar with the GIF decoder. If there is anyone
> else familiar with the GIF decoder you should strongly consider asking them
> for an additional review.

Unfortunately the bigger obstacle is likely to be familiarity with SurfacePipe; right now you're the foremost expert on that other than myself.

Tim, do you have any comments?

Comment 5

[Timothy Nikkel (:tnikkel)]

I think Edwin knows the gif decoder.

Comment 6

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aac5a470cb01

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Most of the GIF decoder is just shifting around here (or being removed). If the SurfacePipe bit is kosher, the only comment from me is \o/.

Although, it would be nice to clean up any variables that are no longer used (for instance, the horrible |clamped_{width,height}| stuff I added a while ago).

Comment 8

[Seth Fowler [:seth] [:s2h]]

The try job mostly looks good. I'm seeing an unexpect pass of |image/test/reftest/gif/tile-transform.html|, which is good because I marked that test as failing on OS X due to a bug in the graphics code that must have been recently fixed. There are also some Windows-specific build issues I'll have to debug.

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #7)
> Most of the GIF decoder is just shifting around here (or being removed). If
> the SurfacePipe bit is kosher, the only comment from me is \o/.
> 
> Although, it would be nice to clean up any variables that are no longer used
> (for instance, the horrible |clamped_{width,height}| stuff I added a while
> ago).

Thanks for the feedback, Edwin!

I thought I got all those variables, but I'll double check. With so much removed in one patch it's definitely possible I missed some stuff.

Comment 10

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee13d4c3f314

Comment 11

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d61bac90cbf3

Comment 12

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9831bb843e5

Comment 13

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df92de57c888

Comment 14

[Seth Fowler [:seth] [:s2h]]

So the try jobs above are testing two things:

- Everything is good on try except that we fail to build on Windows, apparently due to one of the Windows headers getting pulled in and #define'ing symbols that we're using. I'm experimenting with a fix.

- Edwin was right; there's some more stuff we can get rid of. The try job in comment 13 tests a patch that removes several more fields from gif_struct and about 50 more lines from the GIF decoder. If everything looks good I'll upload that as a part 2.

Comment 15

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed52e006b706

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Remove even more code from the GIF decoder.

Edwin, you were right. There was a significant amount of additional code we
could remove. That includes:

- Unnecessary fields from GIF2.h. We can get rid of the 'clamped' fields now
  (they weren't used anyway), and several additional fields can be removed with
  some simple modifications to the BeginImageFrame() method.

- Some code that was copy-pasted between the two frame header decoder states.
  There's no reason to have that second state anyway, so I just removed it and
  merged the two together. Presumably it was useful at some point. I remember we
  noticed this over the summer when Mihai was working on his intern project, and
  I think this same change was made in his patches as well.

- Invalidation-related code that's taken care of by SurfacePipe.

The tests are green on try after removing all this stuff.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Per comment 15 there's one more Windows header symbol conflict. Another try job coming up.

Comment 18

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e268b3d8f84

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1a) - Use SurfacePipe in the GIF decoder.

Renumbered the patch to 'part 1a' since there is now a 'part 1b' and a 'part 2'.

Also, the decoders now #include "imgFrame.h" to make sure they don't rely on
unified compilation to get it.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1b) - Rename some symbols to fix conflicts with the Windows headers.

There are some conflicts with symbols defined in Windows headers in the existing
code which part 1a has revealed. Because they're preprocessor macros,
namespacing doesn't help, so the simplest solution is just to rename the
symbols. We need to rename both Opacity::OPAQUE and WriteState::ERROR.

This patch will be folded into part 1a before landing, since part 1a doesn't
build on Windows without it.

Comment 21

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=651879301581

Comment 22

[Seth Fowler [:seth] [:s2h]]

OK, everything looks good, but I want to go ahead and run the reftests / mochitests / etc on Windows since we haven't been able to do that up to now. If the try job in comment 21 is green and both of the new patches get an r+, this is ready to land.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Thanks, Edwin!

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8728460 [details] [diff] [review]
(Part 1b) - Rename some symbols to fix conflicts with the Windows headers.

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

Please fold this into part 1a before landing. Thanks.

Comment 25

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c6dad1153682c6c9fcafe85157bc2b9e02b419
Bug 1247152 (Part 1) - Use SurfacePipe in the GIF decoder. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb965dc05ec7d56b64581b6bbdb70357f6435052
Bug 1247152 (Part 2) - Remove even more code from the GIF decoder. r=edwin

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1c6dad11536
https://hg.mozilla.org/mozilla-central/rev/cb965dc05ec7

Comment 27

[Toshihiro Yamada]

It seems that these changes breaks gif file rendering at http://www.dmm.com/netgame/ on Win7 64bit.

Comment 28

[Timothy Nikkel (:tnikkel)]

(In reply to Toshihiro Yamada from comment #27)
> It seems that these changes breaks gif file rendering at
> http://www.dmm.com/netgame/ on Win7 64bit.

I just tested and that site looks okay to me (compared a build before these patches to one with them). Can you be more specific on what the problem is and how to reproduce it?

Comment 29

[Seth Fowler [:seth] [:s2h]]

Actually, even better, file a new bug that blocks this one (with, as Tim requested, a description of the problem and how to reproduce it).

Comment 30

[Seth Fowler [:seth] [:s2h]]

FWIW I can't see any issue on OS X either.

Comment 31

[Toshihiro Yamada]

(In reply to Seth Fowler [:seth] [:s2h] from comment #29)
> Actually, even better, file a new bug that blocks this one (with, as Tim
> requested, a description of the problem and how to reproduce it).

filed as Bug 1255675.

Comment 33

[Jeff Muizelaar [:jrmuizel]]

It looks like this regressed GIF decode performance. See bug 1382683