Closed Bug 196295 Opened 19 years ago Closed 15 years ago

Move (merge) GIF2.cpp into nsGIFDecoder2

Categories

(Core :: ImageLib, defect)

defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla1.8beta3

People

(Reporter: paper, Assigned: alfredkayser)

References

Details

Attachments

(1 file, 8 obsolete files)

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
Priority: -- → P5
Target Milestone: --- → mozilla1.5alpha
Target Milestone: mozilla1.5alpha → Future
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.
Depends on: 274391
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.
Attached patch A 'proof-of-concept' version (obsolete) — Splinter Review
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.
Attachment #169629 - Flags: review?(paper)
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).
Blocks: 92580
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)
Attachment #169629 - Flags: review?(paper) → review-
Fair enough, let first finish the review of bug 274391 ...
bug 285872 added to be handled first before the merge.
Depends on: 285872
removing bug 285872 from depends list.  They are not dependant.  Bug 285872 is
an code modification/enhancement bug, this one isn't.
No longer depends on: 285872
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)
Attachment #179928 - Flags: review?(cbiesinger)
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);
Blocks: 285872
(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)
Attachment #179928 - Flags: review?(cbiesinger)
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.
Attachment #179928 - Flags: review?(cbiesinger)
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
Severity: normal → minor
Priority: P5 → --
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.
Target Milestone: Future → mozilla1.8beta3
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.
Attachment #179928 - Flags: review?(cbiesinger) → review+
Attachment #179928 - Flags: review?(pavlov)
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.
Attachment #169629 - Attachment is obsolete: true
Attachment #179928 - Attachment is obsolete: true
Attachment #179928 - Flags: review?(pavlov)
Attachment #205709 - Flags: review?(cbiesinger)
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.
Attachment #205709 - Flags: review?(cbiesinger) → review-
(And, sorry for taking so long :( )
Summary: Move GIF2.cpp into nsGIFDecoder2 → Move (merge) GIF2.cpp into nsGIFDecoder2
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.
Attachment #205709 - Attachment is obsolete: true
Attachment #212376 - Flags: review?(cbiesinger)
Attachment #212376 - Flags: review?(cbiesinger) → review+
Attachment #212376 - Flags: superreview?(tor)
Working on a update for this patch to do this before the Frame/Container/Image stuff, to make this code smaller and cleaner...
Status: NEW → ASSIGNED
Blocks: 366465
Attachment #212376 - Attachment is obsolete: true
Attachment #250974 - Flags: superreview?(tor)
Attachment #212376 - Flags: superreview?(tor)
Note, this patch results in 4K codesize savings.
After bug 317748 is done, I have an updated patch for this one.
Depends on: 317748
Attached patch Updated again after bug 317748 (obsolete) — Splinter Review
Attachment #250974 - Attachment is obsolete: true
Attachment #267697 - Flags: superreview?(tor)
Attachment #250974 - Flags: superreview?(tor)
(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.
Attachment #267697 - Attachment is obsolete: true
Attachment #269179 - Flags: superreview?(tor)
Attachment #267697 - Flags: superreview?(tor)
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.
Assignee: paper → alfredkayser
Attachment #269179 - Attachment is obsolete: true
Attachment #269417 - Flags: superreview?(tor)
Attachment #269179 - Flags: superreview?(tor)
Attachment #269417 - Flags: superreview?(tor) → superreview+
Checked in for Alfred.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Attachment #269740 - Flags: superreview?(tor)
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
Depends on: 386268
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.
Attachment #269740 - Flags: superreview?(tor)
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...
Attachment #269740 - Attachment is obsolete: true
Depends on: 388174
Flags: in-testsuite-
Status: RESOLVED → VERIFIED
Depends on: 413931
You need to log in before you can comment on or make changes to this bug.