Right now all the GIF's are kept internally as 24 bit, this needs to go to 8 bit to save space and time for the animated GIF's. This can be a hug waste of time if we need to write the 24 bit images for each animation cycle.

Taking this

*** Bug 138034 has been marked as a duplicate of this bug. ***

Will it fix a huge slowdawn when opened a few pages with many animated pix? For example: http://www.o45m.ru/forum/read.php?f=4&i=3234&t=3233

*** Bug 143406 has been marked as a duplicate of this bug. ***

Is this a Platform/OS All/all bug?

If GIF's are kept as 24 bit internally , are they kept as 24 bit in the cache too ? Are 8 (or lower) bit PNG's kept as 24 bit too or is this just a problem with GIF's ? Would optimizing every image before it's stored in memory and in the diskcache give any benifits when you account for the time the browser takes optimizing/checking to see if it's optimized ?

Increasing priority as this one is blocking important bugs.

*** Bug 143219 has been marked as a duplicate of this bug. ***

*** Bug 158634 has been marked as a duplicate of this bug. ***

*** Bug 159508 has been marked as a duplicate of this bug. ***

*** Bug 166515 has been marked as a duplicate of this bug. ***

*** Bug 125892 has been marked as a duplicate of this bug. ***

Comment on attachment 101403 [details] [diff] [review] Update some interfaces. typically one edits out the 1416 useless lines of '?' files not managed by CVS before teh 326 of actual patch :-)

Please also remove the ldap Makefile changes (/cygdrive/... etc). Other than that, it looks like a good start, but the actual code to use that depth info is still needed of course. There will probably be a small perf hit for doing draw-time translations to screen depth or 24 bits for compositing, but we'll save a fair amount of footprint/cache space, and might have slightly better processor cache locality. It shouldn't affect pageload times. Overall I'm definitely in favor of this.

Two quick nits.. 1) whitespace in gfx/idl/gfxIImageFrame.idl ? Are those tabs or something? 2) ", 24", not ",24" as you have it in a few places.

Comment on attachment 101403 [details] [diff] [review] Update some interfaces. You probably don't really want to pass a depth of 12 for the jpeg decoder.

I am sorry.. this first patch I put in without looking at the patch.. so all that crap got in there. This patch takes out all the garbage and fixes some spacing issues and of course the famous 12 bits/pixel setting. (I was just seeing if you all were awake ;) This patch will be the first of a few to get GIF's saved at the original 8 bit per pixel. I always feel safer checking in these kinds of changes in pieces.. this interface change just seems like a logical thing to do for our images anyway.

In the idl, looks like tabs: + in gfx_format aFormat, + in gfx_depth aDepth); In the MNG decoder, need a space: + if (NS_FAILED(decoder->mImageFrame->Init(0, 0, width, height, format,24))) In the PPM decoder, need a space: + mFrame->Init(0, 0, mWidth, mHeight, gfxIFormats::RGB,24); Could we add an NS_PRECONDITION to the beginning of gfxImageFrame::Init that the depth arg is sane? Eg that it's a multiple of 8? Or is either 8 or 24? Or something like that (I'm not sure what we would allow as "valid" values)?

+ NS_ASSERTION(0, "This Depth is not supported\n"); NS_ERROR("This ..."). + gfx_depth depth = aWidth; aDepth, you mean? sr=me with those changes...

Comment on attachment 101436 [details] [diff] [review] Fix for aDepth... r=rods

Yikes. No idea how I missed this... + if ( (aDepth != 8) || (aDepth != 24) ){ This will always test true, no? + NS_ASSERTION(0, "This Depth is not supported\n"); Like I said, use NS_ERROR, please.

I tested this patch.. all works as advertised.

Comment on attachment 101467 [details] [diff] [review] Updated patch, fixes error output and the logical check. r=rods

Comment on attachment 101467 [details] [diff] [review] Updated patch, fixes error output and the logical check. sr=bzbarsky

*** Bug 173076 has been marked as a duplicate of this bug. ***

Did it fix things so far?

I am now putting in the support to read in 8 bit images. An nsImage is capable of holding 8 bit images but the GIF decoder assumes we want a 24 bit nsImage.

Bulk moving P1-P5 un-milestoned bugs to future.

This patch converts the gfx part for 8 bit gifs. This is version one.. because it works.. but is slow. I am putting it in this but for archival sake.

This part of the patch seems to be stable for the 8 bit gif. The slowness comes from the GFX part.

why only use an 8 bit depth on XP_PC?

This patch include an error diffusion dither to render the images. This error diffusion version of the 8 bit and 24 bit images on a palettized device is handled in the optimize call.. and the original verions are deleted.. saving on space. I did not use the windows dither call.. because its very slow going to the screen (like 100x slower) and very ugly if you go to an offscreen HDC. Rolled my own.. very fast because it is done once at optimize and looks just as good or better than windows.

looking good so far. The only thing I've spotted is in nsImageWin::DrawToImage, you replace dst's colormap with src. This is will make for horible looking gifs. For example, a 2 color gif with 2 frames, first frame's colormap is (black, red), the smaller second frame's colormap is (red, black).. the second frame will be inverted. I'd leave animated gifs out of it for this go around. They aren't being optimized anyway. Switching to optimized animated gifs is going to need a bit more coding in imgContainerGIF.

why only use an 8 bit depth on XP_PC?

1.) Size. There is no need to use 24 bits for an 8 bit gif. 2.) On a device which has a palette.. 8 bit is all you need, and you can optimize for that depth. Saves space and is faster. 3.) If the device is not palette'ized.. then the 24 bit images are not converted, the 8 bit still are. Dithering will be used on a non-palette'ized devices.

sorry... Dithering will NOT be used on a non-palette'ized devices. As far as the "replacing the dst's colormap with src". I am assuming the src colormap is the same as the destinations color map.. which so far seems to be a good assumption (with it this way, images look great, with it not this way, images look aweful).. because we copy the src pixel to the destination.. this pixel is an index into a color table.. if the dest does not have these same index to color mapping.. then we would have to do a color lookup for each index to the destinations color... this would be a serious hit for time. So far so good on how I have fixed this.. if you have an animation that would test this scenario.. send it to me and I will make sure I have made the right assumptions. So far for my tests, things are working. The reason for the copy in the first place.. is to make sure the dest color map has been put into the proper color table mode (iusage).

dcone, you misread comment 38. The "only" goes with "XP_PC" not with "8". Or to rephrase "Why not make this great change on all platforms, not just Windows?" Which is a valid question, since the bug is marked "All" and there is no mention of platform-specific anything anywhere in this bug... If you're only planning to fix windows, please make that clear and file equivalent bugs for other platforms, at the very least.

Opps.. ok on to the real question... 1.) I think Mac could also use this change.. next platform on my list. 2.) I heard that GTK had no support for Dithering.. but since I since my first try rolled my own dither.. GTK could also go this route.. that would be the third platform on my list.. so we could do all platforms. The one question I still have to answer for this.. is what about printing. Since the printer is a higher resolution device..and we are optimizing for the screen.. the printouts will suffer unless we cache, reload, something for the images that go to the printer. Is a high quality printout needed for an 8 bit device? I am sure someone would want it.. can we make this an option.. all this needs to be discussed.

Opps.. ok on to the real question... 1.) I think Mac could also use this change.. next platform on my list. 2.) I heard that GTK had no support for Dithering.. but since I hit the windows dither roadblock.. I rolled my own dither.. GTK could also go this route.. that would be the third platform on my list.. so we could do those platforms for sure. Each platform could then follow. The one question I still have to answer for this.. is what about printing. Since the printer is a higher resolution device..and we are optimizing for the screen.. the printouts will suffer unless we cache, reload, something for the images that go to the printer. Is a high quality printout needed for an 8 bit device? I am sure someone would want it.. can we make this an option.. all this needs to be discussed.

Just an opinion on printing. It's unlikely that an image will be dynamically generated, so reloading from the web should be no problem. One caveat might be images that are dynamically generated, like on Mapquest. I'm not sure that would even be an issue, though.

> It's unlikely that an image will be dynamically generated Wrong. It's a trivial exercise to find the dozens of bugs we have on issues related to precisely this assumption... (most of them thankfully fixed by now). Not to mention the performance penalty of having the network latency in the middle of the print job, of course.

ah yeah and one more, important, thing that I mentioned. XP_PC does _not_ only apply to windows. It is also used for OS2. Maybe you should use XP_WIN, iirc that's its name.

*** Bug 179986 has been marked as a duplicate of this bug. ***

dcone, why do we store the color map in two places? From what I can tell, it's being stored in mColorMap as well as in mBHead. Is mColorMap needed at all?

mColormap is a permenent place to store the colormap in a cross platform way. The color map in the DIB header is for the DIB. This can change based on the type of DIB we are using or optimized for. For example this color map will be just indexes into a color table held by the destination bitmap. So the actually colors will be lost. This is the mode parameter of the windows bliting operations.

Comment on attachment 106946 [details] [diff] [review] New GFX patch uses the prefs for dithering support. CreateDitheredImage and ConvertTo8BitHalftone look remarkably similar :-)

they are.. but one is for dithering an 8 bit image.. the other for dithering a 24 bit image to an 8 bit image.

With the following in prefs.js: user_pref("images.gifis8bit", true ); user_pref("images.xpdither", true ); The following things do not work: - icons on Window menu, tab bar, etc have weird background - seems to crash randomly - resource:///res/samples/test2.html (the wheels looked dithered) - http://animecity.nu/mozilla/IMGTest/images/redblue.gif (Doesn't load.. if it did, image should not change color) - http://bugzilla.mozilla.org/attachment.cgi?id=86361&action=view doesn't show at all (almost makes mozilla.exe not unload when closed too) - http://animecity.nu/mozilla/IMGTest/AnimatedTest.html Some gifs mess up - http://bugzilla.mozilla.org/attachment.cgi?id=26957&action=view - http://bugzilla.mozilla.org/attachment.cgi?id=58510&action=view - http://bugzilla.mozilla.org/attachment.cgi?id=58511&action=view - http://bugzilla.mozilla.org/attachment.cgi?id=58512&action=view There's others, but that's enough to give you an idea that something needs fixed. removing the prefs makes everything okay again.

*** Bug 181419 has been marked as a duplicate of this bug. ***

k.. fixed a few problems and crasher.. because some of those gifs were using a local color table. Question , the large tiled animated GIF .. that draws real slow.. it seems that each frame has its own color table.. to create the large image. Is that true? If that is true.. should this gif only animate once.. or does it animate every time it redraws. In the current code we convert this to 24 bit so it was not an issue, but now we keep the color tables around.. so is this a case we need to keep track of each local color table when we re-draw. Any information on how this type of GIF is drawn would be appreciated.

Yes, the large tiled animated gif (of fruit) does animate once. It was an attempt by someone to form true color image with gif (pretty silly if you ask me). However, banner ads (the 468x60 kind) often have local color palettes, result in more than 256 colors, and loop, loop, loop.

The last two patches fix all the test cases, and there are preferences to turn on the dither and the 8 bit support. So when this is checked in with those preferences turned off.. this code can be tested by QA and anyone else before its a permenent part of the code. The last fix I did saves animations as 8 bit, but has a 24 bit frame for the screen. This is because a GIF can have a color table for each frame, that is not so bad except if one frame sits ontop of another (transparency) and switches the color table, bad things can happen. Or if frames are drawn in different areas of the target.. then when the entire target gets drawn only the last color table would be used and .. UGLY PICTURE.

Comment on attachment 107477 [details] [diff] [review] GFX patch.. replaced because of a bad initialization. Minor nit: dError*.187 should be dError * 0.187 (add spaces and put zero in front) Comment or remove: + //if ( mColorTableType == DIB_RGB_COLORS ) + //return; Also, I assume this doesn;t have anything to do with printing? (Or at least you tested print)

Comment on attachment 107477 [details] [diff] [review] GFX patch.. replaced because of a bad initialization. r=rods

Comment on attachment 107404 [details] [diff] [review] Updated patch, fixes animations and a crasher. ==== |depth| was added to this method, but it's never used? @@ -133,6 +137,7 @@ // for animation compositing (GIF) or for filling in with a background color. // XXX IMPORTANT: this means that the frame should be initialized BEFORE appending to container PRUint32 numFrames = inlinedGetNumFrames(); + PRUint32 depth; ==== Can we initialize during the declaration? + PRUint32 depth; + depth = 24; ==== I think further use of |nsIPref| is being dicouraged in favor of |nsIPrefBranch|. There was a large sweep done to the editor to do the switch over, so perhaps this should use |nsIPrefBranch| too? Why the need for declaring the const |k8BitSupport|, instead of just passing the constant string directly as an arg? +#if defined (XP_PC) + PRBool b; + const char k8BitSupport[] = "images.gifis8bit"; + nsCOMPtr<nsIPref> prefs = do_GetService(NS_PREF_CONTRACTID); ==== Put a space between the comma and the |&b| arg. Also, use |if (b)| instead of |if (b == PR_TRUE)| since that also seems to be the prevailing style in the file. + if (NS_SUCCEEDED(prefs->GetBoolPref(k8BitSupport,&b))) { + if ( b == PR_TRUE) { + depth = 8; + } + } +#endif ==== Add a space between the comma and |tranIndex|, and fix the indentation of the code you added. Same issues/questions I asked above apply to this block too: - PRUint32 bpr, abpr; + PRUint32 bpr, abpr, depth,tranIndex; + + depth = 24; + +#if defined (XP_PC) + PRBool b; + const char k8BitSupport[] = "images.gifis8bit"; + nsCOMPtr<nsIPref> prefs = do_GetService(NS_PREF_CONTRACTID); + if (NS_SUCCEEDED(prefs->GetBoolPref(k8BitSupport,&b))) { + if ( b == PR_TRUE) { + depth = 8; + } + } +#endif + // We have to delay allocation of the image frame until now because ==== Do we really have to fetch the service and check the pref for every frame of an animation and for every decoded row? Shouldn't this pref be checked once, it's value cached and then used in these methods? ==== Lose the spaces after '(' and before')': + if ( theColorTable ) { + if ( decoder->mGIFStruct->is_transparent ) { ==== Can we add some whitespace here so this is a bit more readable? + for (PRInt32 i=0,cindex=0;i<theColorTable->NumColors;i++,cindex+=3) { ==== The indentation of this |if| line doesn't match the indentation for it's |else|, and it's contents looks indented too far. In fact all of the code that was added to this method uses several different styles of indentation. Can that be cleaned up? + case gfxIFormats::RGB: + { + if (cmap) {// cmap could have null value if the global color table flag is 0 + while (rowBufIndex != decoder->mGIFStruct->rowend) { ==== I know some compilers used to choke on '#' directives that didn't have the '#' char at the beginning of the line, so just to be safe, can we move the |#if| and |#endif| to the start of the lines they are on? + #if defined(XP_MAC) || defined(XP_MACOSX) + *rgbRowIndex++ = 0; // Mac is always 32bits per pixel, this is pad + #endif ==== |tranIndex| wasn't initialized when it was declared, so there is the possibility that it could get used here with a bogus value: - memset(decoder->mRGBLine, 0, bpr); + memset(decoder->mRGBLine, tranIndex, bpr); ==== Lose the space after the |true| values, and losethe extra blank line. +pref("images.gifis8bit", true ); +pref("images.xpdither", true ); + +

Comment on attachment 108112 [details] [diff] [review] New modules patch with updated prefs, some minor fixes. r=rods

*** Bug 183891 has been marked as a duplicate of this bug. ***

Nominating nsbeta1/topembed.

-> Simon

Comment on attachment 108113 [details] [diff] [review] New GFX patch with update prefs. This is almost identical to attachment 107477 [details] [diff] [review], which had r=rods. I am giving r=smontagu on the changes.

From a structure point of view, I personally don't like how some things in the patch are done. Particularly, adding yet another function to gfxImageFrame that just returns an object from it's mImage. Having said that, however, I'm no expert or authority on this code. I emplore module owners and mozilla developers who like to keep Mozilla's structure clean to carefully look at these patches. At the very least could we at least get the patches to pass "JST Reviewer Simulacrum" with less warnings? http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl

I agree that the patch could do with some cleaning up.

Not blocking 1.3beta.

adding one XHTML-compliant URL with animated background image (worst case) to test with with, when patch will be in: http://www.blogblues.com/standards/conversion/blogor/ (perf is horrid on Win2k, 1GHz PIII, 32-bit desktop)

For reference: attachment 108112 [details] [diff] [review] and attachment 108113 [details] [diff] [review] combined and merged to tip.

Comment on attachment 108112 [details] [diff] [review] New modules patch with updated prefs, some minor fixes. patch obsolete, cleaning up requests queue

Comment on attachment 114943 [details] [diff] [review] Last 2 attachments updated first off, the prefs dependancy has got to go. Either always do it on Windows if it doesn't hurt speed or don't do it. Perhaps you could add an API to nsIImage and gfxIImageFrame to get this information? Don't include nsIImage in gfxIImageFrame. Why not just use: void getColorTable([array, size_is(length)] out PRUint8 index, out unsigned long length) instead? Its not like the colormap struct really adds much here. I'm sure that we don't want to get the prefs service and look up a pref every time a rendering context is created.. we create them all the time. How does this effect printing? Let me know when these issues have been addressed and you've got a final patch and I'll review it.

I have now got this working with the current trunk (at least as of this morning, I think the target has moved again since then :-) and corrected a bug with transparency. It's still work in progress: I haven't yet addressed stuart's concerns in comment 77.

"first off, the prefs dependancy has got to go." I think it would be good to land it with the pref initially, so we can easily tell if there are any regressions from landing this large change. Once we are confident that there aren't any regressions then we should remove the pref completely.

I am tracking this on the landings page as 8BitGifs. The latest patches work for me in all the tests I have made. If anyone wants to apply them and do some more testing I will be grateful.

We could also land this enabled by default with an env var to disable it for testing.. Either way, really, as long as we don't ship anything like a release with that prefs dependency.

Comment on attachment 117284 [details] [diff] [review] GFX patch fixing biesi's IRC comments +NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap) should you really return NS_OK if *aColorMap is nsnull? + * Convert a 24 bit image to an 8 bit image and do an error diffusion on it, + * returns a pointer to the new bits for the image + * @update dc - 11/05/2002 + */ +void +nsImageWin::ColorMapToDIBMap() comment and function declaration do not match... For that matter, the description of this function is wrong, too. The function just seems to copy the image map from mColorMap to mBHead. The comment would seem appropriate for DitherImage, which already has a similar comment. The use of @update in these comments is inconsistent. DitherImage does not have such a line, ColorMapToDIBMap has it. It would be nice if the comment before DitherImage would say what it returns. +#define TOPERRORSPREAD 0 +#define LEFTERRORSPREAD 1 +#define RIGHTERROSPREAD 1 +#define BOTTOMERROSPREAD 1 is there a reason why the first two have ERROR in the name, while the latter have ERRO?

Comment on attachment 117284 [details] [diff] [review] GFX patch fixing biesi's IRC comments +NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap) should you really return NS_OK if *aColorMap is nsnull? + * Convert a 24 bit image to an 8 bit image and do an error diffusion on it, + * returns a pointer to the new bits for the image + * @update dc - 11/05/2002 + */ +void +nsImageWin::ColorMapToDIBMap() comment and function declaration do not match... For that matter, the description of this function is wrong, too. The function just seems to copy the image map from mColorMap to mBHead. The comment would seem appropriate for DitherImage, which already has a similar comment. The use of @update in these comments is inconsistent. DitherImage does not have such a line, ColorMapToDIBMap has it. It would be nice if the comment before DitherImage would say what it returns. +#define TOPERRORSPREAD 0 +#define LEFTERRORSPREAD 1 +#define RIGHTERROSPREAD 1 +#define BOTTOMERROSPREAD 1 is there a reason why the first two have ERROR in the name, while the latter have ERRO?

Comment on attachment 117284 [details] [diff] [review] GFX patch fixing biesi's IRC comments +NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap) should you really return NS_OK if *aColorMap is nsnull? + * Convert a 24 bit image to an 8 bit image and do an error diffusion on it, + * returns a pointer to the new bits for the image + * @update dc - 11/05/2002 + */ +void +nsImageWin::ColorMapToDIBMap() comment and function declaration do not match... For that matter, the description of this function is wrong, too. The function just seems to copy the image map from mColorMap to mBHead. The comment would seem appropriate for DitherImage, which already has a similar comment. The use of @update in these comments is inconsistent. DitherImage does not have such a line, ColorMapToDIBMap has it. It would be nice if the comment before DitherImage would say what it returns. +#define TOPERRORSPREAD 0 +#define LEFTERRORSPREAD 1 +#define RIGHTERROSPREAD 1 +#define BOTTOMERROSPREAD 1 is there a reason why the first two have ERROR in the name, while the latter have ERRO?

Comment on attachment 117284 [details] [diff] [review] GFX patch fixing biesi's IRC comments +NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap) should you really return NS_OK if *aColorMap is nsnull? + * Convert a 24 bit image to an 8 bit image and do an error diffusion on it, + * returns a pointer to the new bits for the image + * @update dc - 11/05/2002 + */ +void +nsImageWin::ColorMapToDIBMap() comment and function declaration do not match... For that matter, the description of this function is wrong, too. The function just seems to copy the image map from mColorMap to mBHead. The comment would seem appropriate for DitherImage, which already has a similar comment. The use of @update in these comments is inconsistent. DitherImage does not have such a line, ColorMapToDIBMap has it. It would be nice if the comment before DitherImage would say what it returns. +#define TOPERRORSPREAD 0 +#define LEFTERRORSPREAD 1 +#define RIGHTERROSPREAD 1 +#define BOTTOMERROSPREAD 1 is there a reason why the first two have ERROR in the name, while the latter have ERRO?

Comment on attachment 117284 [details] [diff] [review] GFX patch fixing biesi's IRC comments +NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap) should you really return NS_OK if *aColorMap is nsnull? + * Convert a 24 bit image to an 8 bit image and do an error diffusion on it, + * returns a pointer to the new bits for the image + * @update dc - 11/05/2002 + */ +void +nsImageWin::ColorMapToDIBMap() comment and function declaration do not match... For that matter, the description of this function is wrong, too. The function just seems to copy the image map from mColorMap to mBHead. The comment would seem appropriate for DitherImage, which already has a similar comment. The use of @update in these comments is inconsistent. DitherImage does not have such a line, ColorMapToDIBMap has it. It would be nice if the comment before DitherImage would say what it returns. +#define TOPERRORSPREAD 0 +#define LEFTERRORSPREAD 1 +#define RIGHTERROSPREAD 1 +#define BOTTOMERROSPREAD 1 is there a reason why the first two have ERROR in the name, while the latter have ERRO?

gaaaaah. let me kick necko at this point. I clicked the button but nothing happened. so I clicked it again. ... sorry for the multiple comments.

I agree with bz that using an environment variable to disable this at runtime is the way to go. If we want to assess the impact on performance, the pref is going to obscure the results. I think GetColorTable() makes a lot more sense as SetColorTable()...

|SetTransparentColor()| no longer exists (bug 197485), but I'm not going to attach a new patch just to remove two lines.

Fixed a whole lot of stuff after testing on an 256-color display. The dither actually works now :-)

Comment on attachment 117684 [details] [diff] [review] Ready for review (modules/libpr0n) + PRUint8* theColorTable = new PRUint8[cmapsize * 3]; would it be possible to not allocate this from heap? Like, can you be sure that the colormap is not larger than 256*3? if so, a better (faster) solution might be: PRUint8 theColorTable[256*3]; + transColor |= cmap[decoder->mGIFStruct->tpixel].red; is this used anywhere?

Yes, we know that the colormap has a maximum of 256 colors, so biesi's suggestion makes sense. All references to |transColor| will be removed.

Also, could we use a _static_ array for the colortable? It looks like you always pass the sizes around so the previous data being in it should not be an issue, should it?

Boris Zbarsky wrote: > Also, could we use a _static_ array for the colortable? It looks like you > always pass the sizes around so the previous data being in it should not be an > issue, should it? The colormap of GIFs and other formats can always be smaller, even monocrome GIF pictures/animations are possible. Passing the size of the colormap around makes sense in this case...

Yes, yes. Read my comment again -- the fact that we pass the size may let us get away with: static PRUint8 theColorTable[256*3]; So we only allocate it once.

There is now an experimental build (based on 1.3) available at ftp://ftp.mozilla.org/pub/mozilla/nightly/experimental/8bitgif/mozilla-i586-pc-msvc-8bitgif.zip

Comment on attachment 117684 [details] [diff] [review] Ready for review (modules/libpr0n) + if (NOT_SETUP == gDisable8BitGIFs) { Please follow the rest of the file (and all of imagelib) and put gDisable8BitGIFs first. I don't really like that NOT_SETUP and gDisable8BitGIFs is in 2 files with the exact same code. I like Boris's idea of using a static colortable for 'theColorTable', although making it static could cause issues if we ever have multiple decoders running at once.. Maybe put it on the stack? It isn't there too long. aside from that, r=pavlov. Paper needs to r= this patch since he's going to have to live with the code.

Comment on attachment 117685 [details] [diff] [review] Ready for review (gfx) + [noscript] void setColorTable([array, size_is(length), const] in PRUint8 index, in unsigned long length); there is no reason for this to be noscript. Please move the method in the idl file after unlockAlphaData. Some comments explaining the API need to be added. +NS_IMETHODIMP gfxImageFrame::SetColorTable(const PRUint8 *aIndex, PRUint32 aNumColors) Please move this function to under UnlockAlphaData as well. There isn't any reason to declare |nsColorMap* colorMap| outside of the |if (mImage) {| + dstNumBytes = dstNumBits/8; + srcNumBytes = srcNumBits/8; Change those to shifts for dumb compilers? +static PRBool gDisableDither = NOT_SETUP; Again, I don't really like having this in now 4 files... + // Environment variable to disable the dither is this really needed twice? Why can't it go in the constructor? fix these problems and r=

>I don't really like that NOT_SETUP and gDisable8BitGIFs is in 2 files with the >exact same code. The only way I can see to avoid this is to promote mDepth from imgContainerGif up to imgIContainer and adding an aDepth argument to its Init(). Would you be happy with that? It would have the virtue of consistency with the recent additions to gfxIImageFrame.

I removed the duplicate use of the environment variable within nsRenderingContextWin.cpp, but again I don't see a way to remove the duplication completely.

Comment on attachment 118374 [details] [diff] [review] Addressed stuart's comments (gfx) Transferring r=pavlov and requesting sr

Adding depth to imgIContainer makes no sense. what would that even mean? There is no data directly associated with it.

Comment on attachment 118373 [details] [diff] [review] Addressed stuart's comments (modules/libpr0n) * mDepth in nsGIFDecoder2 and imgContainerGIF should be removed - just use the global gDisable8BitGIFs (perhaps renamed to something that flows a bit nicer) * instead of copying the colormap in HaveDecodedRow (which looks suspiciously like it's asking for problems if image decoding ever goes multithreaded), why not make the relatively minor changes needed in nsGIFDecoder2 so the decode stores in a form that can just be passed to gfx? * Paper - for the transparent color, can we just set the decoder's colormap entry to black, or will that cause problems with animated gifs? * 8-bit, case RGB: why copy the indices (which could be done with memcpy instead of by hand) instead of passing aRowBufPtr to SetImageData? * 8-bit, case RGB_A1: see above the image portion of the data. shouldn't the transparent test in the for loop be against tranIndex (which should probably be made a PRInt32 and set to -1 initially)?

Comment on attachment 118373 [details] [diff] [review] Addressed stuart's comments (modules/libpr0n) >Index: mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.cpp >@@ -582,9 +601,7 @@ > // could optimize further: > // if aPrevFrameIndex == 1 && mLastCompositedFrameIndex <> -1, > // only mFirstFrameRefreshArea needs to be drawn back to composite >- if (isFullPrevFrame) { >- CopyFrameImage(aPrevFrame, mCompositingFrame); >- } else { >+ if (!isFullPrevFrame || (!(CopyFrameImage(aPrevFrame, mCompositingFrame)))) { > BlackenFrame(mCompositingFrame); > SetMaskVisibility(mCompositingFrame, PR_FALSE); > aPrevFrame->DrawTo(mCompositingFrame, prevFrameRect.x, prevFrameRect.y, Was this change intentional, or a missed merge somewhere along the line?

It's intentional. CopyFrameImage() can fail if the frames have different color depths, and if that happens I need to fall back to DrawTo().

> * mDepth in nsGIFDecoder2 and imgContainerGIF should be removed - just > use the global gDisable8BitGIFs (perhaps renamed to something that > flows a bit nicer) Fixed > * instead of copying the colormap in HaveDecodedRow (which looks > suspiciously like it's asking for problems if image decoding ever > goes multithreaded), why not make the relatively minor changes > needed in nsGIFDecoder2 so the decode stores in a form that can > just be passed to gfx? Fixed > * Paper - for the transparent color, can we just set the decoder's > colormap entry to black, or will that cause problems with > animated gifs? I have tested with a lot of animated gifs (the links in comment 54 and others) and don't see any problems > * 8-bit, case RGB: why copy the indices (which could be done with > memcpy instead of by hand) instead of passing aRowBufPtr to > SetImageData? > * 8-bit, case RGB_A1: see above the image portion of the data. You lost me here. Won't that lose the transparent color information? > shouldn't the transparent test in the for loop be against > tranIndex (which should probably be made a PRInt32 and set > to -1 initially)? Fixed

Comment on attachment 118505 [details] [diff] [review] Addressed tor's comments (modules/libpr0n) * GIF2.cpp - case gif_global_colormap/gif_image_colormap: memory allocation can be a malloc instead of calloc. Copying of colormap can be done with memcpy. * HaveDecodedRow - was thinking that if modifying the colormap entry for the transparent color is a valid optimization (Paper?) it could be done in the gif decoder (case gif_control_extension). Minor nit: the comment above this section of code should probably be "set the..." instead of "copy the..." now. * HaveDecodedRow, 8-bit, case _A1 - the image portion can have the same optimization of eliminating the needless copy that you did for the RGB case (passing aRowBufPtr directly to SetImageData).

By the way, I'll make the same change in the gfx part of using the global variables directly and not adding new members. I won't attach a new patch for now.

After making the change to pass aRowBufPtr to SetImageData, I'm left with something rather kludgy in the case where there is no color map: + if (cmap) { // cmap could have null value if the global color table + // flag is 0 + for (int i = 0; i < aDuplicateCount; ++i) { + decoder->mImageFrame->SetImageData(aRowBufPtr, bpr, + (aRowNumber+i)*bpr); + } + } else { + memset(decoder->mRGBLine, 0, bpr); + for (int i = 0; i < aDuplicateCount; ++i) { + decoder->mImageFrame->SetImageData(decoder->mRGBLine, + bpr, (aRowNumber+i)*bpr); + } + } What about just doing decoder->mImageFrame->SetImageData(nsnull, bpr, (aRowNumber+i)*bpr); (without the memset) and then doing if (aData) memcpy(imgData + newOffset, aData, aLength); else memset(imgData + newOffset, 0, aLength); in gfxImageFrame::SetImageData() ?

Comment on attachment 118824 [details] [diff] [review] Patch addressing comments on IRC today (modules/libpr0n) so I can't say I ever liked this construct: +#define NOT_SETUP 0x33 +static PRBool gEnable8BitGIFs = NOT_SETUP; Would it be possible to do the real intialization in imglib_Initialize (in build/nsImageModule.cpp)?

Performance testing by me and Barrett has shown that decoding to 8 bits is good for performance when the display is 8-bit, and bad for performance when it is 24-bit; and enabling dithering is a huge performance win on a palettized device. Taking all that into account, I am removing the environment variables altogether and just enabling the features depending on the display capabilities.

In the spirit of "divide et impera", I've moved the style, whitespace, and infrastructure changes out into separate bugs, bug 201568 and bug 201576, which I hope I can get quickly reviewed and checked in.

*** Bug 216962 has been marked as a duplicate of this bug. ***

did patch for bug 216430 help here (I'm on Linux right now) ?

Comment on attachment 118373 [details] [diff] [review] Addressed stuart's comments (modules/libpr0n) removing obsolete review request

*** Bug 189805 has been marked as a duplicate of this bug. ***

Any progress on this one?

xah@myrealbox.com, only the assigned person should change the target milestone!

Mass reassign my image bugs to nobody@mozilla.org

Anyone willing to take this as this is topembed+ and P2?

What about Cairo? It doesn't support 8bit images, which makes this one useless. Options: 1. extend Cairo with 8bit images. 2. in the GifContainer keep the 8bit data, and only generate a Gfx image when the actual frame needs to be displayed. Both are difficult and have huge impact...

Stuart or vlad, is this still a reasonable bug given comment #130? If someone were to hack in 8bit support for Cairo, would this still result in a good performance win for gifs?

(In reply to comment #131) > Stuart or vlad, is this still a reasonable bug given comment #130? If someone > were to hack in 8bit support for Cairo, would this still result in a good > performance win for gifs? I doubt it; adding 8-bit support to cairo would be a pretty huge task and of very limited value. The only savings that I can think of is the memory savings of storing the images as 8-bit + palette instead of as ARGB in memory, but I think drawing them would actually be slower. I would suggest WONTFIX here, though imagelib and our image rendering story overall needs an overhaul. Stuart and I have talked about some various ideas, but nothing concrete yet.

don't think we're going to do this.

Proof of concept patch to keep the frames, except the first one, at 8bit depth. By keeping the first frame in current 24bit format, this wont impact performance of still gif images. This patch does: * extends gfxIImageFrame to also store 8bit images, including 'DrawTo' to a real 24bit frame. Note, these 8bit frames are not valid elsewhere. Note2: 8 bit frames don't create a 'nsIImage' object, they just store image data. * modified imgContainerGif to do the animation by composing every animated frame keeping the current frame in mCompositingFrame, and then clearing according the mDisposalMethod and the drawing the next frame (using the above DrawTo) on top. This makes actually the frame composition easier then before... * Only 2 or 3 real frames with images are created at most (the first frame, the current compositing frame and sometimes the previous comp. frame (for RESTORE_PREVIOUS). Savings: for a large image (like 'galaxy_anim_short.mov2.gif'), consists of 503 frames of 681x409 pixels. With 24bit storage this results in more than 420MB allocated memory. Using 8bit, this is then just 140MB. So, this saves about 300MB memory usage... Also because only 2 or 3 real images are created instead of 503 (!) this saves a lot of load on the OS image handling. The image above loads in my version within 2 seconds, in current FF builds this takes more than 5 seconds, and a lot of swapping. Performance impact during runtime seems also to be less, at least about the same.

This patch is completely working. I have tested with a large set of animated gifs, includes the ones that I could find attached to open&close bugs about gif decoding. Testing also confirmed the reduced memory usage. Like 140MB for the galaxy animation instead of 420MB. Also the cpu load seems to be half of what it was before. The total result is that these large animations don't freeze the browser that easily, and even also loads the animation even faster and starts the animation almost directly (instead of first swamping memory).

dveditz is almost certainly the wrong person to review this. the reason I said we weren't going to do it was because we're about to change all the image interfaces and we're about to land APNG (any day now) which redoes a bunch of the gif imgIContainer stuff (gets rid of imgContainerGIF and merges it all back in to imgContainer (yes, this undoes a previous patch)). I appreciate the work and the memory savings here but I really don't want to complicate the image code any more until we've redone it all. Can we hold on this patch for a while until we get some more of the image apis cleaned up?

Sure, no problem. I will await the refactoring. Removing the imgContainerGif is a good idea, and when we can fully switch to Cairo there is more tuning/optimisation to be had.

Comment on attachment 247469 [details] [diff] [review] V2: Update to current trunk with Cairo I might be OK for a sr= here if needed, but you need a gfx module owner/peer for review (stuart, vlad, roc) It's not always entirely up-to-date, but when looking for owner/reviewers start at http://www.mozilla.org/owners.html

Alfred, any chance of revisiting this bug now that APNG has landed?

Pavlov is still planning to revamp the image,frame,container structure, and has asked me to wait for that.

Stuart - are you planning on doing those changes still for 1.9? If not, would it be possible for Alfred to go ahead with this patch? It would be a nice win.

I am working on a new patch (where animation frames for GIF's are kept as 8bit in gfxImageFrame), but bug 367281, bug 366465, and bug 386268 need to be completed to make a good and clean patch.

Also bug 216682 needs to be SR'ed and committed before I can make the patch for this bug.

Once bug 367281 is checked in, I can generate a patch to keep animation frames of GIF's stored as 8 bit.

Note, long description of the changes will follow soon. Meanwhile people could try this out if it works in their tree.

Summary of approach: Let gfxImageFrame store 8bit frames for animation frames, as palette (with 2<<depth entries) and image buffer with a byte for each pixel. Let imgContainer draw 8bit frames into the composite frame during animation. Let nsGIFDecoder2 decode to 8bit frames for the animation frames and 32bit for the first frame. Annotation to the diff: * gfxIFormats.idl: add 'PAL' and 'PAL_A1' formats * gfxIImageFrame.idl: add 'GetPaletteData' to get pointer to palette data (like GetImageData) * gfxImageFrame.cpp: Add support for PAL and PAL_A1 formats. For PAL/PAL_A1 no 'nsImage' is created, only the memory buffer for the palette and the 8bit pixels. Make all getters/setters behave for PAL images. * gfxImageFrame.h: Define helpers to calc. palette and imagedata sizes * imgContainer.cpp: Make DoComposite handle 8bit frames, but sort of always draw into the composite frame. Note still some optimizations are applied to this. The ImageUpdated notification moved to 'Clear/DrawFrames' into DoComposite, so that is only performed once. Extend DrawFrameTo to also handle 8bit animation frames. So far, all these changes are compatible with the old way of doing animation (and thus also for APNG's). * GIF2.h: Make pointers to image 8bit based for easier calculation into 8bit frames. * nsGIFDecoder2.cpp: BeginImageFrame: take aDepth parameter to correctly select right type of frame to create. FlushImageData is only needed for first frame (other frames are not flushed, as they are only handled when completed). EndImageFrame: Haeberli-inspired hack only for first frame. For first frame, convert from 8bit to 32bit cairo here. DoLzw: instead direct colormapping store original 8bit value only. ConvertColormap: as palette is now rightly sized to the depth, no need to clear the remainder.

OK, I've been playing around with Alfred's patch. I haven't noticed any rendering issues for animated GIFs yet with it. Also, the memory savings are dramatic. For example, the image below used to cause Firefox' memory consumption to increase by nearly 24MB upon being loaded. With the patch applied, it only goes up by a bit over 9MB. http://thermoanalytics.com/applications/mom/aircraft_cabin_temperature/images/overview_animation_640.gif

This patch doesn't appear to handle the case of someone doing do_GetInterface() on a gfxIImageFrame to get the nsIImage -- various places in our code do this including the clipboard I believe. It looks like the code will just crash with this patch. If we care about the interface that we've been providing here, then you're going to need to return an object...

Alfred? Any ideas on my comment?

All callers/users of the imgContainer::GetFrameAt, only get the first frame like: aCursor->GetFrameAt(0, getter_AddRefs(frame)); Or they get the current frame: container->GetCurrentFrame(getter_AddRefs(gfxFrame)); With this patch, both will still result in a gfxIImageFrame, with a valid nsIImage. Several options to prevent misuse (getting a frame without an nsIImage): 1. Change GetFrameAt into GetFirstFrame, so that imgContainer only has GetFirstFrame, GetCurrentFrame. Both guaranteed to have a valid nsIImage. 2. Make the do_GetInterface() return NS_ERROR or such for gfxImageFrame without an nsIImage. 3. Make the do_GetInterface() create an nsIImage on the fly. Ad 1. The interface won't allow anymore to get the image of each frame. But the validity of these nsImage is questionable has it is sometimes a composed image and sometimes a original image. When an user of gfxImageFrame really wants to have the source data, getImageData can still supply this. Note, For the layout code, we only need GetFirstFrame and GetCurrentFrame. This means that the imgIContainer.idl needs to change. Ad 2. Callers may still have problems on the failing do_GetInterface. And while the idl doesn't change, the semantics has changed anyway: do_GetInterface on (GetFrameAt(n!=0, ...) will return NS_ERROR... Ad 3. Costs extra memory, can cause leaks, etc... Constructing an nsImage on the fly, will be difficult (and invalid), as the animation composition code can only do the 'current frame'. So, the cleanest way is option 1, and is the most easy way, and the most safe way. There are only 5 callers of GetFrameAt: s/GetFrameAt(0, /GetFirstFrame(/ would suffice. /widget/src/windows/nsWindow.cpp, line 2662 -- aCursor->GetFrameAt(0, getter_AddRefs(frame)); /widget/src/os2/nsWindow.cpp, line 1858 -- aCursor->GetFrameAt(0, getter_AddRefs(frame)); /widget/src/gtk2/nsWindow.cpp, line 1032 -- aCursor->GetFrameAt(0, getter_AddRefs(frame)); /modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp, line 456 -- mImage->GetFrameAt(0, getter_AddRefs(mFrame)); /content/base/src/nsContentUtils.cpp, line 2203 -- imgContainer->GetFrameAt(0, getter_AddRefs(imgFrame)); Note, making the imgContainer.idl only have GetFirstFrame and GetCurrentFrame would allow to also to for animations that only loop once just to keep the first frame and the current frame, and destroy the frames in between sooner. Note that the patch is currently bitrotted anyway, and needs updating anyway, so I will first attempt to update the patch, and test this approach.

As for comment 150 and comment 151. The patch already protects for do_GetInterface on a gfxImageFrame without a real nsIImage, by returning a NS_ERROR value. And currently all callers for GetFrameAt only ask for the first frame (0), which has always a valid nsIImage. And GetCurrentFrame always returns a frame with a valid nsIImage. So current code won't break. I would propose to replace GetFrameAt with GetFirstFrame in a separate bug. As for some statistics, for the image: http://vectormagic.stanford.edu/comparisons/rotational_invariance.gif, about:cache reports: Before: Data size: 368050176 bytes After: Data size: 108527616 bytes So, for this image we save thus 260 MB.

Alfred, this patch seems to have broken animated GIFs pretty badly. The throbber has a "flashing" going on where frames after the first one seem to be washed out. Also, the image in comment #148 seems to be completely missing the Red channel.

I'm also getting a LOT of random crashiness with v8 that I wasn't getting with v7.

I haven't tested this patch yet, but it looks like some parts might break apngs a bit

Note, this patch works with the image of comment #148, as well as my test of APNG's. For APNG images imgContainer follows the original logic of frame composition, as only frames of type PAL do get special treatment. Note2, with this patch, GIF animation creates at most 3 images in the X server space.

Comment on attachment 287392 [details] [diff] [review] V9: Better initialisation in gfxImageFrame to prevent crashes > PRUint32 nsGIFDecoder2::OutputRow() > { ... >+ // Row to process >+ const PRUint32 bpr = sizeof(PRUint32) * mGIFStruct.width; >+ PRUint8 *rowp = mImageData + (mGIFStruct.irow * bpr); Nit - fix spacing.

Did the issue from comment 149 ever get addressed?

mostly -- we're doing a followup patch to fully address them

Landed by stuart at 2007-11-07 13:33. mozilla/gfx/idl/gfxIFormats.idl 1.5 mozilla/gfx/idl/gfxIImageFrame.idl 1.19 mozilla/gfx/src/shared/gfxImageFrame.cpp 1.44 mozilla/gfx/src/shared/gfxImageFrame.h 1.14 mozilla/modules/libpr0n/src/imgContainer.cpp 1.60 mozilla/modules/libpr0n/decoders/gif/GIF2.h 1.27 mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp 1.86 mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.h 1.31

Verified using build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007110805 Minefield/3.0b2pre. According to the about:cache, the rotational_invariance.gif is now using 108527616 bytes instead of 368050176 bytes, saving 260MB. Thanks for the reviews, checkin and support!

We should get some testcases in for the situations where the intermediate patches had bugs in them, as such testcases would have more easily revealed some of the ways those patches were broken.