Bug 15974 – Public GIF image decoder does not handle disposal of previous frames correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Peter Kasting]

GIFImageDecoder.cpp (not used by Safari, but used by the Cairo/Qt ports) does not correctly deal with disposal of previous frames in a multi-frame GIF.

GIFs can specify four disposal methods for a frame: "not specified", "keep", "clear to background color", and "clear to previous".  There are multiple issues:
* GIFImageReader doesn't provide the disposal method to GIFImageDecoder.cpp, just a flag indicating whether or not the method was "keep".  This makes it impossible to distinguish the two clear methods, and more noticeably, treats "not specified" like a clear method.  Mozilla, IE, and Safari, by contrast, treat "not specified" as "keep", and at least some GIFs on the web rely on this (example to be attached after filing).
* When clearing, the code in GIFImageDecoder.cpp:initFrameBuffer() reverts to the contents of frame 0, which is not the same as either of the clear methods in the spec.  By contrast, Mozilla treats both clear methods like "clear to background color".  I don't know what IE or Safari do or what actual images rely on.

I think the right fix here is to change RGBA32Buffer's "includeInNextFrame()" method (which is only used by the GIF decoder) to instead return an actual disposal method, have GIFImageReader provide the true disposal method, and then fix initFrameBuffer() to respect this method.  I don't know whether "clear to previous" should be treated like "clear to background color" (as Mozilla does) or made to follow the GIF spec.  Input welcome.

[Peter Kasting]

Created attachment 17247 [details]
Sample image relying on "not specified" disposal

This image never specifies a disposal method for any of its frames, and relies on that making each frame act like the disposal method was "keep".

[Peter Kasting]

Created attachment 17249 [details]
Sample image (1) relying on "clear to previous"

Here's an image that I think relies on "clear to previous" behavior to work right.

[Peter Kasting]

Created attachment 17250 [details]
Sample image (2) relying on "clear to previous"

...and another.

Firefox, IE and Safari all display both these images fine.  I need to verify exactly what the clearing behavior in the files is, though.

[Peter Kasting]

I lied, I don't think Mozilla treats "clear to previous" as "clear to background color".  I was misinterpreting their code.  I think everybody else actually gets this right according to the spec.

[Peter Kasting]

Created attachment 17320 [details]
patch v1

This patch makes all the above attached images work correctly.  It also calls ensureHeight() in a more consistent way (we don't ensureHeight(m_size.height()) on some frames before decoding anything the way we did before) and calls setHasAlpha() correctly (although a bit conservatively -- we might claim to have alpha when we don't, but never the other way), which was trickier with the new initialization code.

[Darin Adler]

Comment on attachment 17320 [details]
patch v1

r=me

[Mark Rowe (bdash)]

Landed in r27896.

[Mark Rowe (bdash)]

I had to roll this out as it caused a bunch of build failures in the ports using this class.

[Alp Toker]

Peter,

If you need some help integrating your modified GIF API into other ports, please do ask and I'll be happy to take a look.

[Peter Kasting]

(In reply to comment #9)
> If you need some help integrating your modified GIF API into other ports,
> please do ask and I'll be happy to take a look.

If you have the ability to build various other ports, help would be very appreciated.  Feel free to post an updated patch.  Otherwise, I will try to get to writing patches that cover the other ports as well in the next few days (although I don't have the ability to build them to test).

[Peter Kasting]

Created attachment 17541 [details]
patch v2

This attempts to fix build bustage problems.

[Alp Toker]

Landed in r28068.