GIF comment handling is broken

[raygard]

Pillow's handling of comments in GIF files is broken.

I hesitated to write this as an issue because I'm not sure where Pillow should go with this.

Current handling of GIF comment blocks appears to be as follows. On reading, comment blocks are merged on a per-image basis. That is, they are seen before an image and merged into the frame's .info["comment"] dict. If a frame image is preceded by separate comment blocks "foo" and "bar" they will appear as "foobar" in frame.info["comment"]. I believe this is wrong. Comments that appear after the last image frame are lost completely, because there's no frame associated with them.

Input comments before the first image block will be (by default) written on output when .save() is called. If a comment= arg is in the .save() it will override the input comment. Comments after the first image will be lost unless the user takes care to grab them from the frame.info dicts.

Currently only one comment can be written, and it will be duplicated in every frame. This is an error similar to the current handling of the Netscape looping block (fixed by @radarhere in PR #6211).

Also, they are in the wrong place in the output file; they are placed between the Graphic Control Block and the Image Descriptor Block. I know the spec says of the GCB "The scope of this Extension is the graphic rendering block that follows it; it is possible for other extensions to be present between this block and its target." (23d in spec) But it also has a EBNF grammar (Appendix B) of allowable block structure that does not admit anything between the GCB and the Image block. I suspect that some programs (MS Teams maybe?) may not like a comment there.

Is it OK that the canonical methods of Pillow reading and writing of GIF files do not preserve most comments? I mean code like:

with Image.open(infile) as im:
    im.save(outfile, save_all=True)

I started a PR to address all this but decided to see if anyone else cares before putting more work into it.

I was thinking along these lines: Normally code as above would preserve at least all the comments except any after the last image. If a comment= arg is given to .save() then it would be written just after the Netscape looping block (if any). An empty comment="" or comment=None could suppress existing comment blocks. Maybe some other convention (a new arg?) could leave only the user's comment= and suppress other comments. Also, maybe comments after the last image could be stored internally in a variable self.comment and made available to the user. Maybe they could also be saved in the main .encoderinfo to be written to output.

At the least, I would like to see the comment= arg (or comment retained from the input) get written once, at the front of the output file. I can do a PR for that if @radarhere and/or others agree.

BTW the documentation could use some work too.

[radarhere]

https://www.fileformat.info/format/gif/egff.htm

The Comment Extension block is used to insert a human-readable string of text into a GIF file or data stream. Each comment may contain up to 255 7-bit ASCII characters, including all the ASCII control codes. Any number of Comment Extension blocks may occur in a GIF file, and they may appear anywhere after the Global Color Table. It is suggested, however, that comments should appear before or after all image data in the GIF file.

So I'm inclined to agree that comments shouldn't exist an on per-image basis.

---

it also has a EBNF grammar (Appendix B) of allowable block structure that does not admit anything between the GCB and the Image block.

I realise it may seem completely obvious to you, but could you spell out for me exactly how Appendix B says this?

If a comment= arg is given to .save() then it would be written just after the Netscape looping block (if any)

https://en.wikipedia.org/wiki/GIF states that "Graphic Control Extension (comment fields precede this in most files)", so it may be better to have it before, not after.

Maybe some other convention (a new arg?) could leave only the user's comment= and suppress other comments. Also, maybe comments after the last image could be stored internally in a variable self.comment and made available to the user. Maybe they could also be saved in the main .encoderinfo to be written to output.

Up until this point, I was following your argument to have all of the comments together, but now I'm lost. Are you trying to account for the fact that when loading a GIF file, it might be a file that Pillow has saved previously with repeated comments? I think it would be reasonable to just combine all of the comment blocks into a single string when reading. If the comment block is duplicated like Pillow has been doing up until now, then only one copy could be used.

BTW the documentation could use some work too.

What do you feel is missing?

[raygard]

From Appendix B:

<Graphic Block> ::=       [Graphic Control Extension] <Graphic-Rendering Block>
<Graphic-Rendering Block> ::=  <Table-Based Image> | Plain Text Extension
<Table-Based Image> ::=   Image Descriptor [Local Color Table] Image Data

By substitution

<Graphic Block> ::=       [Graphic Control Extension] <Table-Based Image> 
<Graphic Block> ::=       [Graphic Control Extension] Plain Text Extension

We ignore Plain Text Extensions (they're rare but I have a few files with them), so

<Graphic Block> ::=       [Graphic Control Extension]  Image Descriptor [Local Color Table] Image Data

Graphic Control Extension, if present, is followed directly by Image Descriptor. Again, this contradicts 23d (and the latitude given for comment blocks in 24 e ii), but it would not hurt us to follow it in what we emit (Postel's law).

https://en.wikipedia.org/wiki/GIF states that "Graphic Control Extension (comment fields precede this in most files)", so it may be better to have it before, not after.

Not sure what you mean here. (Edit - maybe you were just agreeing?) I was saying the desired output should be: Header, Logical Screen Descriptor, Global Color Table, NETSCAPE Application Extension for looping, Comment Extension, Graphic Control Extension, Image Descriptor, Local Color Table, Image Data, ...
... where the GCT, looping block, comment block, Graphic Control Extension and LCT are all optional by the spec. We currently always write a GCT. The looping block and comment block are currently optional. The GC extension is currently written only if we have transparency, duration, or disposal specified, and LCT is optional as well, but I think if these are written they should be in the order above.

I have probably been making this more complex (in my mind) than it needs to be. I have GIFs collected over decades that include comments all over the place (though except from PIL/Pillow I don't think any of them come between a Graphic Control block and its Image Descriptor). Maybe we only need to collect all the comments together when reading a GIF, and write them in a single block. The current behavior seems to be to keep a comment that comes at the beginning of a file and write it on output, but if the user specified a comment= arg then the incoming comment is ignored and replaced with the one specified. Is it OK that existing comments get replaced? If we make existing comments available in .info then the user can decide whether to include them in a comment= arg. Of course the user would have to do something to cause the entire file to be read before the later comments can be collected, but I think that usually happens. That could be pointed out in the docs.

I can work on a PR to that effect. Existing comments will be collected on reading, dups removed, placed in the main .info of the GifImageFile. User comment= will replace any other comments. Empty comment="" (or comment=None ?) could suppress all comments. Whaddya think?

Regarding documentation, it doesn't mention that currently only an "up front" comment will be detected upon opening. But I was probably thinking about some other documentation issues with GIFs. For example, I recently tweaked the first paragraph to refer to LZW:

The library writes LZW encoded files in GIF87a by default, unless GIF89a features are used or GIF89a is already in use.

It think it would be clearer as: "The library writes files in GIF87a format by default, unless GIF89a features are used or GIF89a is already in use. The pixel data is written using LZW encoding as specified in the GIF89a specification." The way it is now, one could infer that LZW is only used for GIF87a.

Also, there's an example in a section "Reading local images" that includes im.size = (x1 - x0, y1 - y0). im.size is a read-only property and I bet this has been in the documentation for decades since it actually worked that way.

The third paragraph at the beginning of image-file-formats.html says

So if copy() is called on an image, or another method internally creates a copy of the image, the fp (file pointer), along with any methods and attributes specific to a format.

I have tried but cannot parse that; I think there are words missing.

[raygard]

I think my original posting here was overly complicated and not well thought out. Maybe incorrect in places as well. The ability to write a single comment is probably sufficient and I have made PR #6300 to fix most of the issues I see with comments (mainly being in every frame and between the GC extension and the Image Descriptor). Also PR #6294 to keep two adjacent comments from being run together and PR #6292 to fix a little bug I found while working on this. PR #6300 overlaps PR #6211 and will need to be merged carefully.

It's also possible that the docs on GIF in the Handbook can use some work and I'll try to do that if I have time.

I'm still not comfortable with making PRs and hope I have not made any significant mistakes. I am intending to work on the transparency issue #6260 I reported. It's harder to make the tests than it is to make the code to be tested.

After that I will try to get to the issue of the LZW coding always using at least 9 bits ("LZW Minimum Code Size" is always 8 and that means codes of 9 to 12 bits). The codes for e.g. monochrome images could start with 4 bits and make smaller files.

And BTW the claim that GIFs (except for very small ones) are interlaced by default is not right. Multi-frame GIFs are written without interlace and the interlace= arg is ignored for them. The ImageFile._save() in _write_frame_data() is not preceded with a setup of .encoderconfig so it gets the default empty config from Image.save(). The .encoderconfig tuple has both the LZW Minimum Code Size and the interlace flag, so I am intending to address them together. But that raises (for me) a question of whether we really want the default to be interlace=True. That made more sense 20+ years ago when we used dialup modems. Now it may be more important to minimize file size, and interlace works against LZW encoding on compression.

[radarhere]

I've created #6313 for the documentation issues.

[raygard]

I’ll work on this.

[radarhere]

#6292, #6300 and #6313 have been merged. Is #6325 the last piece necessary to consider this done?

[raygard]

Yes, I believe so. Thank you so much. Now we need your PR #6211 merged along with #6300 so that the NETSCAPE2.0 looping extension comes just before any comment.

[hugovk]

Merged, thanks all!