Separating out some of the work from bug 143046 to make it more reviewable.

Comment on attachment 120139 [details] [diff] [review] Patch * Remove the GIF_ColorMap and GIF_IRGB structures from GIF2.h * After closer examination of the gif specification I don't believe the trick of zeroing the colormap entry when reading the gif_control_extension block will work. Two problems: + there can be multiple image blocks all using the global colormap and with different transparent indexes + the control extension block occurs in the stream before its corresponding local colormap (if one is used)

> After closer examination of the gif specification I don't believe > the trick of zeroing the colormap entry when reading the > gif_control_extension block will work. Does that mean I should move the code to zero the colormap entry for the transparent color back into nsGIFDecoder2::HaveDecodedRow(), or will that not work either?

The HaveDecodedRow() mechanism that was used previously should be correct.

In that case it's not needed in this patch, since the original code in HaveDecodedRow was only in the 8-bit code path. I've removed the unused structured from nsGIF2.h

Comment on attachment 120474 [details] [diff] [review] Addressing tor's comments I'm also not a fan of ++[ix], but not a reason to block. sr=tor

Comment on attachment 120474 [details] [diff] [review] Addressing tor's comments + decoder->mImageFrame->SetImageData(nsnull, + bpr, (aRowNumber+i)*bpr); + if (format == gfxIFormats::RGB_A1 || + format == gfxIFormats::BGR_A1) { + decoder->mImageFrame->SetAlphaData(nsnull, + abpr, (aRowNumber+i)*abpr); At least gtk requires that you call SetAlphaData before SetImageData

>At least gtk requires that you call SetAlphaData before SetImageData Thanks, I didn't know that. I've swapped the two round and will check in in that way.

Attaching a new patch with that change, just in case this gets reviewed after I leave tonight and before the freeze. If that happens, it would be really good if someone could check it in for me.

Comment on attachment 121365 [details] [diff] [review] Changed order of SetAlphaData and SetImageData Transferring sr=tor

Comment on attachment 121365 [details] [diff] [review] Changed order of SetAlphaData and SetImageData Requesting 1.4 approval: this is a performance and footprint win. I'm seeing 1.5% gain on Txul and 4-5% on Ts

Comment on attachment 121365 [details] [diff] [review] Changed order of SetAlphaData and SetImageData a=sspitzer, assuming this has been well tested on mac os, linux, win32 if you've got a list of gifs you've been using to test, can you attach them (or the links to them) for QA?

There are lots of gifs in the UI, so if there are problems they usually show up even before you get to load a testcase. Testcases I have been using: (mostly from bug 143046 comment 54) http://animecity.nu/mozilla/IMGTest/images/redblue.gif attachment 86361 [details] (very slow, but that is bug 149205) http://animecity.nu/mozilla/IMGTest/AnimatedTest.html attachment 26957 [details] attachment 58510 [details] attachment 58511 [details] attachment 58512 [details] http://www.animationfactory.com/ Random searches on http://images.google.com

Fix checked in.

simon, thanks for that test case info. I'm setting hardware / os to all, to let QA (tpreston?) know that this fix affected every platform.