A lot of wasted cycles passing a struct back and forth between the two modules. It's not needed, they can be combined.

low priority. No use doing any work until Bug 143046 is in. Guessing on the safe side and saying this bug won't be done until 1.5a

Looking at memory allocaters (specifically the recyclingAllocator), I noticed that GIF2.cpp allocates in one go, three blocks of fixed size, and releases them at the same time. So these could also be allocated as one block: 535 if (!gs->prefix) 536 gs->prefix = (PRUint16 *)gif_calloc(sizeof(PRUint16), MAX_BITS); 537 if (!gs->suffix) 538 gs->suffix = ( PRUint8 *)gif_calloc(sizeof(PRUint8), MAX_BITS); 539 if (!gs->stack) 540 gs->stack = ( PRUint8 *)gif_calloc(sizeof(PRUint8), MAX_BITS); and: 1081 gif_free(gs->prefix); 1082 gif_free(gs->suffix); 1083 gif_free(gs->stack); Allocating them as one block, saves some memory allocation overhead, also in the recycler (or even more so!). Instead of 3 blocks of different sizes per decode run, it allocates always one block of the same size per run.

Some of the struct cleanup is in bug 274391.

Having the gif decoder itself, and the gif to frame handling specific to mozilla separately still makes sense. To optimize the passing of data between GIF2.cpp and nsGIFDecoder2.cpp we can remove the arguments, except for the client_data, from all of the callbacks to nsGIFDecoder2 because all data is allready in the struct itself.

This patch builds on bug 274391, and moves everything from GIF2 into nsGifDecoder2.cpp. This saves about 1.5K code (compared to 274391 - both together saves about 3K). Included are also some PR_Realloc optimizations, so that for large animated GIF's much less memory (re)allocations are done. Performance measurements are difficult, but it feels faster in loading of these large animated GIF's.

Comment on attachment 169629 [details] [diff] [review] A 'proof-of-concept' version Is this worth something? Note, there are some things which need some cleanup (unnecc. touched whitespace in imgContainer, and some printf's here and there). At least this patch compiles and tests with a a number of extreme gifs (very large animated and static images).

I was just thinking about this bug the other day. I hope to take a look at your patch within the next week. Sorry for the delay, holiday season and getting back into the groove and all :)

Comment on attachment 169629 [details] [diff] [review] A 'proof-of-concept' version I can't review this until bug 274391 is in. Also, it looks like the patch does more than just merge code, but also adds new code (or alters existing code in a way not representing the original unmerged files)

Fair enough, let first finish the review of bug 274391 ...

bug 285872 added to be handled first before the merge.

removing bug 285872 from depends list. They are not dependant. Bug 285872 is an code modification/enhancement bug, this one isn't.

No non-structural code has been changed in this patch. Structural changes are as follows: 1) Move functions in GIF2 into nsGIFDecoder2. GIFInit and gif_destroy were so small, that I removed the functions and moved the actual code to ::Init and ::Close. 2) passed in references to mGIFStruct (usually labelled gs) were removed from functions. "gs" replaced with mGIFStruct. 3) un-static-ize functions in nsGIFDecoder2, remove aClientData from function parameters, and remove clientptr from gif_struct. Since all the data is in nsGIFDecoder2 now, there's no need to pass clientptr around or store it (it would just point to "this"). Most of the code that made use of clientptr would "nsGIFDecoder2 *decoder = NS_STATIC_CAST(nsGIFDecoder2*, mGIFStruct->clientptr);", and similar for aClientData. As a result, all references to "decoder->" were removed. 4) Formatting fixes where I spotted them: "Comma, then space", "No lines over 80 in width)", and "removal of trailing spaces". Requesting review by biesi, since this is sort of an extension on Bug 189052 (done by him). I've tested this patch on Win98, Win2k, and Linux/GTK (FC3)

Note, some previously static functions take parameters which are allready members. Such as: BeginGIF(mGIFStruct->screen_width, mGIFStruct->screen_height, mGIFStruct->screen_bgcolor); and: EndGIF(mGIFStruct->loop_count); and: EndImageFrame(mGIFStruct->images_decoded + 1, mGIFStruct->delay_time); and: mGIFStruct->height = height; mGIFStruct->width = width; BeginImageFrame(mGIFStruct->images_decoded + 1, /* Frame number, 1-n */ mGIFStruct->x_offset, /* X offset in logical screen */ mGIFStruct->y_offset, /* Y offset in logical screen */ width, height);

(In reply to comment #13) > Note, some previously static functions take parameters which are allready members. Thanks, I missed those. I'll add them to my next patch (14-ish hours from now)

Comment on attachment 179928 [details] [diff] [review] Merge GIF2 into nsGIFDecoder, remove statics re-instating review request from biesi. After removing the useless variable passing mentioned in comment #13, the patched ballooned. I decided it would be better to do the changes in a seperate bug. There will certainly be a whole lot of clean-up to be done on nsGIFDecoder2 after the merge.

Comment on attachment 179928 [details] [diff] [review] Merge GIF2 into nsGIFDecoder, remove statics it may take me a week or so to get to this review...

(In reply to comment #16) > it may take me a week or so to get to this review... No rush, I don't want to check it in until after the Gecko 1.8 release

Actually, biesi, I wouldn't mind getting this in 1.8b3 (ie after the 1.8b2 release). A two month b3 period should be enough time to spot any problems that may arise (which really can only be caused by typo or a bad variable name change, or a bad copy/paste) But, it's still low priority and there's no rush.

Comment on attachment 179928 [details] [diff] [review] Merge GIF2 into nsGIFDecoder, remove statics + // Initialize mGIFStruct (origionally GIF2.cpp:GIFInit) "originally" :) and I'm not sure if putting history into comments is necessary... r=me otherwise, and I have to apologize for taking so long for this review :-/ also, pav should probably ok this.

Note, because of bug 274391 and bug 295872, the current patch is 'bitrotted' and needs to be updated.

Updated version to current trunk, but also: * removed redundant function parameters * Split gif_struct into gif_state (to be a fixed structure of nsGIFDecoder2) and gif_data (for the dynamically allocated large chunks of data using the recycling allocator). * The split allowed to access most of the gif_struct data as direct members of the nsGIFDecoder2 class, instead of through an additional pointer. * Merged gif_init and gif_destroy into the ::Init and ::Close of nsGIFDecoder2. * Removed redundant member mBackgroundRGBIndex With these 'extra' changes the total code (and change) is less than without. Total compiled code size reduction is about 2K.

I wish you hadn't added additional stuff since the last patch. that makes reviewing this much harder.

Comment on attachment 205709 [details] [diff] [review] Updated to current tree, and removed the redundant parameters nsGIFDecoder2.cpp: I think you should zero out mGIFState in the constructor rather than in Init. it'd also make me happier if you initialized mGIFData to null in the ctor. + NS_ASSERTION(mGIFData, "Allocation of mGIFData failed"); ew. please remove this assertion. OOM is a failure case that needs to be handled, not something you can assert won't happen. (I know this was there before, but still) - if(decoder->mGIFStruct->global_colormap && - decoder->mGIFStruct->screen_bgcolor < cmapsize) { + if (mGIFState.screen_bgcolor < cmapsize) { OK, why don't you need to test global_colormap anymore? + if (mGIFState.progressive_display && mGIFState.interlaced && (mGIFState.ipass < 4)) { please limit lines to 80 characters. Please put copyright information at the top of the file, not somewhere in the middle. + HaveDecodedRow(mGIFState.rowbuf, // Pointer to single scanline temporary buffer + drow_start, // Row number + drow_end - drow_start + 1); // Number of times to duplicate the row? 80 chars would be nice, but if not, please do align the comments the same way +nsGIFDecoder2::gif_write(const PRUint8 *buf, PRUint32 len) +{ + if (!buf || !len) do you need to check buf? the old code didn't. + if (mGIFState.screen_height < mGIFState.height) + mGIFState.screen_height = mGIFState.height; + if (mGIFState.screen_width < width) { this used to be in the if, why move it out? GIF2.h: +typedef struct gif_state { while you're here.. wanna make this use C++ syntax? (i.e. remove the "typedef") nsGIFDecoder2.h: + /* + * These are the APIs that the client calls to intialize, + * push data to, and shut down the GIF decoder. + */ that sounds wrong. there's no method to shut down the gif decoder anymore, right?

I'd also make an inline helper function for calculating clear_code.

>Please put copyright information at the top of the file, not somewhere in the >middle. and the includes and macro definitions too, for that matter...

Comment on attachment 205709 [details] [diff] [review] Updated to current tree, and removed the redundant parameters Finally: there are still two comments that refer to gif_struct.

(And, sorry for taking so long :( )

nsGIFDecoder2.cpp: I think you should zero out mGIFState in the constructor rather than in Init. * Done it'd also make me happier if you initialized mGIFData to null in the ctor. * Done + NS_ASSERTION(mGIFData, "Allocation of mGIFData failed"); ew. please remove this assertion. OOM is a failure case that needs to be handled, not something you can assert won't happen. (I know this was there before, but still) * Done - if(decoder->mGIFStruct->global_colormap && - decoder->mGIFStruct->screen_bgcolor < cmapsize) { + if (mGIFState.screen_bgcolor < cmapsize) { OK, why don't you need to test global_colormap anymore? * Not needed, as global_colormap is now a fixed array of mGIFStruct itself + if (mGIFState.progressive_display && mGIFState.interlaced && (mGIFState.ipass < 4)) { please limit lines to 80 characters. * Done Please put copyright information at the top of the file, not somewhere in the middle. * Done + HaveDecodedRow(mGIFState.rowbuf, // Pointer to single scanline temporary buffer + drow_start, // Row number + drow_end - drow_start + 1); // Number of times to duplicate the row? 80 chars would be nice, but if not, please do align the comments the same way * Done +nsGIFDecoder2::gif_write(const PRUint8 *buf, PRUint32 len) +{ + if (!buf || !len) do you need to check buf? the old code didn't. * Better safe than crashy. + if (mGIFState.screen_height < mGIFState.height) + mGIFState.screen_height = mGIFState.height; + if (mGIFState.screen_width < width) { this used to be in the if, why move it out? * Because prev. screen_height was only checked if screen_width was wrong. * Now it is always checked. GIF2.h: +typedef struct gif_state { while you're here.. wanna make this use C++ syntax? (i.e. remove the "typedef") * Done nsGIFDecoder2.h: + /* + * These are the APIs that the client calls to intialize, + * push data to, and shut down the GIF decoder. + */ that sounds wrong. there's no method to shut down the gif decoder anymore, right? * Done I'd also make an inline helper function for calculating clear_code. * Done >Please put copyright information at the top of the file, not somewhere in the >middle. and the includes and macro definitions too, for that matter... * Done Finally: there are still two comments that refer to gif_struct. * Done So, I have incorporated the comments, except for the nullcheck for buf, and for the screen_height check.

Working on a update for this patch to do this before the Frame/Container/Image stuff, to make this code smaller and cleaner...

Note, this patch results in 4K codesize savings.

After bug 317748 is done, I have an updated patch for this one.

(In reply to comment #33) > Created an attachment (id=267697) [details] > Updated again after bug 317748 Is the reason you're not setting the background color in HasDecodedRow() because gfx/imglib don't appear to use that information? Unless there is a strong reason not to, the methods gif_write, output_row, and do_lzw should be renamed to conform with the mixed-caps mozilla naming style.

Note, background color is really nowhere used, and will never be so.

Comment on attachment 269179 [details] [diff] [review] V8: Fixed functionnames to Mozilla style In general, once you ask for review on a patch, you should refrain from doing major changes to it unless requested by the reviewer. That will result in getter reviews quicker, and lessening the frustration of reviewer and reviewie alike. >+//****************************************************************************** >+// Send the data to the display front-end. >+PRUint32 nsGIFDecoder2::OutputRow() >+{ ... >+ switch (mGIFStruct.ipass) { >+ case 1: >+ row_dup = 7; >+ row_shift = 3; >+ break; >+ case 2: >+ row_dup = 3; >+ row_shift = 1; >+ break; >+ case 3: >+ row_dup = 1; >+ row_shift = 0; >+ break; >+ default: >+ break; >+ } If you're going switch similar code later on in this method to table driven (see below), why not this bit as well? ... >+ static const PRUint8 kjump[5] = { 1,8,8,4,2 }; Nit - use spaces after commas.

Replaced the switch/case with: + /* ipass = 1,2,3 results in resp. row_dup = 7,3,1 and row_shift = 3,1,0 */ + const PRUint32 row_dup = 15 >> mGIFStruct.ipass; + const PRUint32 row_shift = row_dup >> 1; and added the spaces between 1,8,8,4,2.

Checked in for Alfred.

It seems that this might have slowed down animated GIFs considerably; noticed particularly with https://bugzilla.mozilla.org/skins/custom/images/mozchomp.gif .

hmm, I don't remember seeing slowdowns like that with earlier revisions of the patch (even V8). Of course, my memory might be fuzzy. But is it possible that something in V9 slowed it down?

It has to do something with the delay timing. It is apparently not a CPU usage issue... Found it: The following statements used to be after the EndImageFrame call. /* An image can specify a delay time before which to display subsequent images. */ if (gs->delay_time < MINIMUM_DELAY_TIME) gs->delay_time = MINIMUM_DELAY_TIME; (MINIMUM_DELAY_TIME = 100) Making them effectively useless as the delay_time is stored in the frame in the EndImageFrame call. Note that in http://lxr.mozilla.org/mozilla/source/gfx/src/shared/gfxImageFrame.cpp#494 467 /* attribute long timeout; */ 468 NS_IMETHODIMP gfxImageFrame::GetTimeout(PRInt32 *aTimeout) The timeout is value is clipped to 100 when it is between 0 and 10. The fix is thus simple: remove the useless delay_time clipping. With that applied mozilla is chomping is fast as it used to be.

Ah, that would do it - don't you hate it when you accidentally fix old code? :) I noticed that IE and Opera were also throttling that image, while Safari was rendering at the specified framerate. I filed a bug against webkit, and they are resolving it by using the IE behavior: delays <= 50 ms are mapped to 100ms. We might want to do the same, though as a seperate bug to provide better tracking. http://bugs.webkit.org/show_bug.cgi?id=14413

Bug 386269 filed as a follow-up on the the delay threshold.

Do we have any provisions for automated regression tests for the GIF decoder? Particularity concerned here since crashes in decoders can be security issues. Some ideas of way's to implement: http://www.thinkingphp.org/2006/09/10/test-driven-development-in-real-world-apps/ Compare to known good decoded info, decode than re-encode and compare results, etc.

I filed bug 386651 for adding image decoding regression tests.

Comment on attachment 269740 [details] [diff] [review] Patch to remove the redundant delay_time clipping Bug 386269 has addressed this issue, so marking this one obsolete...