Move (merge) GIF2.cpp into nsGIFDecoder2

VERIFIED FIXED in mozilla1.8beta3

Status

()

defect
--
minor
VERIFIED FIXED
16 years ago
12 years ago

People

(Reporter: paper, Assigned: alfredkayser)

Tracking

Trunk
mozilla1.8beta3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

Reporter

Description

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

Comment 1

16 years ago
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
Reporter

Updated

16 years ago
Target Milestone: mozilla1.5alpha → Future
Assignee

Comment 2

15 years ago
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.
Assignee

Updated

15 years ago
Depends on: 274391
Assignee

Comment 3

15 years ago
Some of the struct cleanup is in bug 274391.
Assignee

Comment 4

15 years ago
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.
Assignee

Comment 5

15 years ago
Posted 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.
Assignee

Updated

15 years ago
Attachment #169629 - Flags: review?(paper)
Assignee

Comment 6

15 years ago
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).
Assignee

Updated

15 years ago
Blocks: 92580
Reporter

Comment 7

15 years ago
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 :)
Reporter

Comment 8

15 years ago
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-
Assignee

Comment 9

15 years ago
Fair enough, let first finish the review of bug 274391 ...
Assignee

Comment 10

14 years ago
bug 285872 added to be handled first before the merge.
Depends on: 285872
Reporter

Comment 11

14 years ago
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
Reporter

Comment 12

14 years ago
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)
Assignee

Comment 13

14 years ago
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);
Assignee

Updated

14 years ago
Blocks: 285872
Reporter

Comment 14

14 years ago
(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)
Reporter

Updated

14 years ago
Attachment #179928 - Flags: review?(cbiesinger)
Reporter

Comment 15

14 years ago
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...
Reporter

Comment 17

14 years ago
(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
Reporter

Updated

14 years ago
Severity: normal → minor
Priority: P5 → --
Reporter

Comment 18

14 years ago
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+

Updated

14 years ago
Attachment #179928 - Flags: review?(pavlov)
Assignee

Comment 20

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

Comment 21

14 years ago
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)
Assignee

Updated

14 years ago
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
Assignee

Comment 28

13 years ago
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+
Assignee

Updated

13 years ago
Attachment #212376 - Flags: superreview?(tor)
Assignee

Comment 29

13 years ago
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
Assignee

Updated

13 years ago
Blocks: 366465
Assignee

Comment 30

13 years ago
Attachment #212376 - Attachment is obsolete: true
Attachment #250974 - Flags: superreview?(tor)
Attachment #212376 - Flags: superreview?(tor)
Assignee

Comment 31

13 years ago
Note, this patch results in 4K codesize savings.
Assignee

Comment 32

12 years ago
After bug 317748 is done, I have an updated patch for this one.
Depends on: 317748
Assignee

Comment 33

12 years ago
Attachment #250974 - Attachment is obsolete: true
Attachment #267697 - Flags: superreview?(tor)
Attachment #250974 - Flags: superreview?(tor)

Comment 34

12 years ago
(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.

Assignee

Comment 35

12 years ago
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 36

12 years ago
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.
Assignee

Comment 37

12 years ago
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)

Updated

12 years ago
Attachment #269417 - Flags: superreview?(tor) → superreview+

Comment 38

12 years ago
Checked in for Alfred.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 39

12 years ago
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?
Assignee

Comment 41

12 years ago
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)

Comment 42

12 years ago
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

Comment 43

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

Comment 44

12 years ago
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.

Updated

12 years ago
Attachment #269740 - Flags: superreview?(tor)
Assignee

Comment 46

12 years ago
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

Updated

12 years ago
Flags: in-testsuite-
Assignee

Updated

12 years ago
Status: RESOLVED → VERIFIED
Depends on: 413931
You need to log in before you can comment on or make changes to this bug.