Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

image/gif: generated image cannot be opened in xv and crashes OmniWeb 3.x web browser #33748

Closed
tenox7 opened this issue Aug 21, 2019 · 19 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@tenox7
Copy link

tenox7 commented Aug 21, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.5 linux/amd64

Does this issue reproduce with the latest release?

yes, any release

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/usr/local/google/home/tenox/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/usr/local/google/home/tenox/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/google-golang"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build197159774=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Create a simple gif image:

package main

import (
	"image"
	"image/color"
	"image/gif"
	"os"
	"fmt"
)

func main() {
	g, _ := os.Create("go.gif")
	defer g.Close()
	p := []color.Color{color.White, color.Black}
	r := image.Rect(0,0,100,100)
	i := image.NewPaletted(r, p)
	for x:=20; x<40; x++ {
		for y:=20; y<40; y++ {
			fmt.Printf("%d %d", x, y)
			i.SetColorIndex(x,y, 1)
		}
	}
	gif.Encode(g, i, nil)
}

What did you expect to see?

Expected to open in older image viewers, editors and browsers.

What did you see instead?

A number of old browsers, image viewers and editors report generated gif image as corrupted or display it mangled. For example xv image viewer, https://en.wikipedia.org/wiki/Xv_(software) says the file is turncated. OmniWeb browser (v2.x and v3.x) crashes. Most graphical software on SGI IRIX reports it corrupted.

@odeke-em odeke-em changed the title image/gif corrupted in older viewers image/gif: generated image cannot render and is reported as corrupted in older viewers Aug 21, 2019
@odeke-em
Copy link
Member

Thank you for reporting this issue @tenox7!

Kindly pinging some folks who might be able to help @nigeltao @horgh.

@odeke-em odeke-em added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 21, 2019
@odeke-em odeke-em added this to the Go1.14 milestone Aug 21, 2019
@smasher164
Copy link
Member

Xv states that the file is truncated.
xverr

I assume you mean <= Omniweb 5, since Omniweb 6 renders the gif properly.
correctomniweb6

@tenox7
Copy link
Author

tenox7 commented Aug 21, 2019

Yes OmniWeb 2.x and 3.x. I have updated the description, thank you. Actually in a fact reloading the gif image in OmniWeb 3.0 causes the browser to crash.

image

@tenox7
Copy link
Author

tenox7 commented Aug 21, 2019

XV debug mode says this:

LoadGIF: imagesep (got=0)    at start: offset=0x14
xv: LoadGIF() - picture is 100x100, 1 bits, non-interlaced
  at end:   dataptr=0xc9

LoadGIF: trailerxv: Desired colormap
(  0  ff,ff,ff 9978)
(  1  00,00,00 22)


xv: result of sorting colormap
(  0  ff,ff,ff)     (  1  00,00,00)     

@tenox7 tenox7 changed the title image/gif: generated image cannot render and is reported as corrupted in older viewers image/gif: generated image cannot be opened in xv and crashes OmniWeb 3.x web browser Aug 21, 2019
@nigeltao
Copy link
Contributor

It's possibly the same issue as #13746 (comment) with the eog ("Eye of GNOME") image viewer.

It's hard to say definitively, though. It's fair enough to say that Go's GIF encoder is writing something that old image viewers can't read, but it's hard to do something about that (in the Go code) without knowing what it is specifically those old image viewers don't like. And figuring out what they don't like probably means spelunking in xv or OmniWeb code, not in Go code.

Or, as I said in issue #13746, if it's:

For example, the "GIF image loader cannot understand this image" error message comes from https://github.com/GNOME/gdk-pixbuf/blob/master/gdk-pixbuf/io-gif.c#L634 in the lzw_read_byte function, and the comment immediately above that says: "FIXME - we should handle this case".

then it's really up to EoG to fix their bug. Which they might already have done (in GNOME/gdk-pixbuf@b88f1ce, not sure if it's rolled out to stable distro's yet).

@tenox7
Copy link
Author

tenox7 commented Aug 22, 2019

I also think that this may be to do with LZW compression. Would it be possible to add an option to disable LZW compression when creating GIFs?

@nigeltao
Copy link
Contributor

nigeltao commented Aug 22, 2019

I don't think you can have a GIF image without LZW compression.

This is in terms of the GIF file format, not any one specific encoder or decoder.

@mntn-xyz
Copy link

mntn-xyz commented Oct 7, 2021

I think I may have determined the source of this problem, in part at least. The generated GIFs have their color resolution bits and global color table size zeroed out. This is part of the Logical Screen Descriptor's packed byte. (See https://www.matthewflickinger.com/lab/whatsinagif/bits_and_bytes.asp for more information.)

(Edit: while it is a problem that this data is not being written properly, it's apparently not the main issue for the library I'm having trouble with. It may be compression clear codes, still investigating.)

The problem can be fixed using GraphicsMagick: gm convert broken.gif -colors 8 fixed.gif. (Edit: original image I used was also 8 colors, this isn't changing the image.) In the image I tested below, This adds some bytes to the image, sets the color resolution bits to 111 (8 bit color), and sets the global color table size to 010 (8 colors). I'm not sure what the additional bytes are or where they are placed, I haven't looked that deep into it yet.

Problem image:
test1

Problem image (corrected):
test2

Related issues: nothings/stb#1222 skyjake/lagrange#353 makew0rld/didder#7

@mntn-xyz
Copy link

mntn-xyz commented Oct 7, 2021

Looking here it would seem that this information should be written properly: https://cs.opensource.google/go/go/+/refs/tags/go1.16.8:src/image/gif/writer.go;l=158;drc=refs%2Ftags%2Fgo1.16.8

Unless, that is, the gif data doesn't include a ColorModel/Palette. But it appears that this is populated correctly during Encode(), as far as I can tell.

@mntn-xyz
Copy link

mntn-xyz commented Oct 7, 2021

The author of the stb library, which also has trouble parsing these images, says the problem may be with the lack of a "clear code" which should be present per the GIF RFC: nothings/stb#1222 (comment)

@mntn-xyz
Copy link

mntn-xyz commented Oct 7, 2021

To help with debugging this, I generated a 1x1 gif. Here's the hex of it before and after the GraphicsMagick "fix":

Broken:

00000000: 4749 4638 3961 0100 0100 8000 0000 0000  GIF89a..........
00000010: 0000 002c 0000 0000 0100 0100 0002 0128  ...,...........(
00000020: 003b                                     .;

Fixed:

00000000: 4749 4638 3961 0100 0100 f000 0000 0000  GIF89a..........
00000010: 0000 0021 f904 0000 0000 002c 0000 0000  ...!.......,....
00000020: 0100 0100 0002 0244 0100 3b              .......D..;

@mntn-xyz
Copy link

mntn-xyz commented Oct 8, 2021

OK, so with some digging into the actual bytes I am certain that the issue has something to do with the format of the image data bytes (last section before the trailer).

Original file:

Header
  47 49 46 38 39 61

LSD
  0100 width (little endian)
  0100 height (little endian)
  80 packed field
    1 global color table flag
    000 color resolution
    0 sort flag
    000 size of global color table
  00 background color index
  00 aspect ratio

Global color table
  000000 color 1
  000000 color 2

Image descriptor
  2c separator
  0000 x location
  0000 y location
  0100 width
  0100 height
  00 packed field
    0 local color table flag
    0 interlace flag
    0 sort flag
    00 reserved
    000 size of local color table

Image data
  02 LZW minimum code size
  01 number of bytes
  28 data
  00 block terminator

Trailer
  3b

Fixed file:

Header
  47 49 46 38 39 61

LSD
  0100 width (little endian)
  0100 height (little endian)
  f0 packed field
    1 global color table flag
    111 color resolution
    0 sort flag
    000 size of global color table
  00 background color index
  00 aspect ratio

Global color table
  000000 color 1
  000000 color 2

Application extension block
  21 GIF extension code
  f9 application extension label (graphic control label)
  04 block size
  00 packed field
    000 reserved
    000 disposal method (not animated)
    0 user input flag
    0 transparency flag
  0000 delay time
  00 transparency index
  00 block terminator

Image descriptor
  2c separator
  0000 x location
  0000 y location
  0100 width
  0100 height
  00 packed field
    0 local color table flag
    0 interlace flag
    0 sort flag
    00 reserved
    000 size of local color table

Image data
  02 LZW minimum code size
  02 number of bytes
  4401 data
  00 block terminator

Trailer
  3b

Disregarding the application extension block (GraphicsMagick must have felt this was necessary despite the lack of animation), it's obvious that there is a difference in the image data. I manually edited the hex data of the broken image to match the data of the fixed image, and lo and behold, the image is now working. Here's the hex of the manually corrected image:

00000000: 4749 4638 3961 0100 0100 8000 0000 0000  GIF89a..........
00000010: 0000 002c 0000 0000 0100 0100 0002 0244  ...,...........D
00000020: 0100 0a                                  ...

The new image is one byte longer.

@mntn-xyz
Copy link

mntn-xyz commented Oct 8, 2021

OK, checking the LZW bytes, it appears that the author of the stb library was correct. The code is not outputting a clear code before it begins writing the LZW data. Every source I come across treats this as mandatory, except for the W3C spec which says it "should" be included. (In this case, though, I think that "should" was actually intended to be "MUST," especially in light of how many older implementations handle it as a requirement.)

Perhaps the blockWriter setup() function could write a clear code (here: https://cs.opensource.google/go/go/+/refs/tags/go1.16.8:src/image/gif/writer.go;l=73;drc=refs%2Ftags%2Fgo1.16.8)? The clear code is 2 * the LZW code size (litWidth as defined here: https://cs.opensource.google/go/go/+/refs/tags/go1.16.8:src/image/gif/writer.go;l=327;drc=refs%2Ftags%2Fgo1.16.8;bpv=0;bpt=1).

Edit: ok, it's not quite as simple as just writing a clear code since the data length also changes, but writing that code should fix the issue for libraries that expect the data to start with a conventional clear code.

@makew0rld
Copy link

Fixing this issue will probably also close #26108

@gopherbot
Copy link

Change https://golang.org/cl/354710 mentions this issue: compress/lzw: output a Clear code first, per GIF spec

@nigeltao
Copy link
Contributor

nigeltao commented Oct 9, 2021

@mntn-xyz thanks for the excellent and thorough debugging. I have sent out a fix for review.

@tenox7
Copy link
Author

tenox7 commented Oct 9, 2021

Thank you for fixing this!!!

@makew0rld
Copy link

Great to see this fixed, thank you! I assume the fix will find its way into the next release, Go 1.17.3?

@nigeltao
Copy link
Contributor

I expect it to release in Go 1.18.

If you must have it sooner, you can always fork the compress/lzw and image/gif packages from the standard library, as a temporary measure.

@golang golang locked and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants