Use SurfacePipe in the GIF decoder

RESOLVED FIXED in Firefox 48

Status

()

Core
ImageLib
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8717759 [details] [diff] [review]
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.
Attachment #8717759 - Flags: review?(n.nethercote)
(Assignee)

Comment 2

2 years ago
Created attachment 8718191 [details] [diff] [review]
Use SurfacePipe in the GIF decoder.

This updated version of the patch is rebased against the latest version of bug
1246851.
Attachment #8718191 - Flags: review?(n.nethercote)
(Assignee)

Updated

2 years ago
Attachment #8717759 - Attachment is obsolete: true
Attachment #8717759 - Flags: review?(n.nethercote)
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)?
Attachment #8718191 - Flags: review?(n.nethercote) → review+
(Assignee)

Updated

2 years ago
Blocks: 1234077
(Assignee)

Comment 4

2 years ago
(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?
Flags: needinfo?(tnikkel)
I think Edwin knows the gif decoder.
Flags: needinfo?(tnikkel) → needinfo?(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).
Flags: needinfo?(edwin)
(Assignee)

Comment 8

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

Comment 9

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

Comment 14

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

Comment 16

2 years ago
Created attachment 8728270 [details] [diff] [review]
(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.
Attachment #8728270 - Flags: review?(edwin)
(Assignee)

Comment 17

2 years ago
Per comment 15 there's one more Windows header symbol conflict. Another try job coming up.
(Assignee)

Comment 19

2 years ago
Created attachment 8728456 [details] [diff] [review]
(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.
(Assignee)

Updated

2 years ago
Attachment #8718191 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8728460 [details] [diff] [review]
(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.
Attachment #8728460 - Flags: review?(n.nethercote)
(Assignee)

Comment 22

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

Comment 23

2 years ago
Thanks, Edwin!
(Assignee)

Updated

2 years ago
Blocks: 1255109
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.
Attachment #8728460 - Flags: review?(n.nethercote) → review+

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1c6dad11536
https://hg.mozilla.org/mozilla-central/rev/cb965dc05ec7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Comment 27

2 years ago
It seems that these changes breaks gif file rendering at http://www.dmm.com/netgame/ on Win7 64bit.
(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?
Flags: needinfo?(yamadat501)
(Assignee)

Comment 29

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

Comment 30

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

Updated

2 years ago
Depends on: 1255675

Comment 31

2 years ago
(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.

Updated

2 years ago
Flags: needinfo?(yamadat501)

Updated

2 years ago
Depends on: 1255774
Depends on: 1255930
Blocks: 1251382
Depends on: 1262338

Updated

2 years ago
Blocks: 1267865

Updated

2 years ago
Duplicate of this bug: 1267865
Depends on: 1274057

Updated

2 years ago
Depends on: 1274402
(Assignee)

Updated

2 years ago
Blocks: 1227546
Depends on: 1303427
It looks like this regressed GIF decode performance. See bug 1382683
You need to log in before you can comment on or make changes to this bug.