366465 - GIF Decoding in Cairo can skip all row buffers

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

Currently the GIF decoder first decodes the lzw data into a rowbuffer with the color index as a byte value, and then it is colormapped to mRGBLine in Cairo format, and then that line is copied to the Cairo image buffer.

However, the 'do_lzw' function can also directly colormap the pixel into the Cairo image data, skipping the two row buffers completely.

Note, this is similar to what BMP/ICO decoders also do (see bug 360000 for an example of it is fixed over there).

Note 2: It is probably best to do bug 196295 first, so that the decoder bits are all in one file making it easier to rearrange.

Note 3: When this and bug 196295 are done, the only dynamically allocated data in the gif decoder is the 'local_colormap' which is for animated frames only.
(besides all the image frames themselves...)

Comment 1

[Alfred Kayser]

Another item to look for in optimizing GIF decoding:

Found on vector64.com:
http://www.vector64.com/todo.html

# When writing out GIF image data in nsGIFDecoder2.cpp, and in some types of nsPNGDecoder.cpp, we currently write out three bytes at a time. We actually write out a doubleword but the last character gets overwritten the next time around. For Win64, we have __int64 so we could theoretically store the data in an integer and then write out the 8-byte integer instead of  two four-byte integers reducing the number of writes by a factor of two.
# The colormaps used in GIF are three characters and perhaps we could improve GIF rendering performance if we made them four so that they would be aligned.

Note, this can also apply to palette based decoding of BMP, ICO and PNG images.

Comment 2

[Alfred Kayser]

Attachment: Patch to directly decode from LZW data into the Cairo image buffer

This patch makes the image decoder directly store the pixels into the Cairo image buffer. Previously each row was first decoded to rowbuf (byte per pixel), and then into a RGB row buffer (mRGBLine), and then copied into the image buffer.

By converting the colormaps from the GIF data directly (after optional CMS transformation) into a Cairo compatible colormap, we can directly decode from the LZW decoding into the image data. 

This saves two mallocs (width bytes and width*4 bytes), as well as two copies.
Also because the pixel conversion is done in the colormap instead of for each pixel this also saves CPU cycles.

Comment 3

[Alfred Kayser]

Attachment: V2: Updated to current trunk

Comment 4

[Stuart Parmenter]

Comment on attachment 276010 [details] [diff] [review]
V2: Updated to current trunk

can you please change all the places you're using 'foo* bar' to 'foo *bar'?

Comment 5

[Stuart Parmenter]

Comment on attachment 276010 [details] [diff] [review]
V2: Updated to current trunk

waiting on new patch then r+

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(there might be a few small conflicts in this with my work in bug 391583 but they should be trivial to merge)

Comment 7

[Alfred Kayser]

Attachment: V3: Updated to trunk, and fixed '*' spacing

Comment 8

[Phil Ringnalda (:philor)]

modules/libpr0n/decoders/gif/GIF2.h 1.25
modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp 1.82
modules/libpr0n/decoders/gif/nsGIFDecoder2.h 1.30

Comment 9

[Alfred Kayser]

Green tree's, and current nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082618 Minefield/3.0a8pre) working fine.