Closed
Bug 196295
Opened 22 years ago
Closed 17 years ago
Move (merge) GIF2.cpp into nsGIFDecoder2
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
VERIFIED
FIXED
mozilla1.8beta3
People
(Reporter: paper, Assigned: alfredkayser)
References
Details
Attachments
(1 file, 8 obsolete files)
79.07 KB,
patch
|
tor
:
superreview+
|
Details | Diff | Splinter Review |
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•22 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•21 years ago
|
Target Milestone: mozilla1.5alpha → Future
Assignee | ||
Comment 2•20 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 | ||
Comment 3•20 years ago
|
||
Some of the struct cleanup is in bug 274391.
Assignee | ||
Comment 4•20 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•20 years ago
|
||
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•20 years ago
|
Attachment #169629 -
Flags: review?(paper)
Assignee | ||
Comment 6•20 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).
Reporter | ||
Comment 7•20 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•20 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•20 years ago
|
||
Fair enough, let first finish the review of bug 274391 ...
Assignee | ||
Comment 10•20 years ago
|
||
bug 285872 added to be handled first before the merge.
Depends on: 285872
Reporter | ||
Comment 11•20 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•20 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•20 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);
Reporter | ||
Comment 14•20 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•20 years ago
|
Attachment #179928 -
Flags: review?(cbiesinger)
Reporter | ||
Comment 15•20 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 16•20 years ago
|
||
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•20 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•20 years ago
|
Severity: normal → minor
Priority: P5 → --
Reporter | ||
Comment 18•20 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 19•19 years ago
|
||
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•19 years ago
|
Attachment #179928 -
Flags: review?(pavlov)
Assignee | ||
Comment 20•19 years ago
|
||
Note, because of bug 274391 and bug 295872, the current patch is 'bitrotted' and
needs to be updated.
Assignee | ||
Comment 21•19 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•19 years ago
|
Attachment #205709 -
Flags: review?(cbiesinger)
Comment 22•19 years ago
|
||
I wish you hadn't added additional stuff since the last patch. that makes reviewing this much harder.
Comment 23•19 years ago
|
||
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?
Comment 24•19 years ago
|
||
I'd also make an inline helper function for calculating clear_code.
Comment 25•19 years ago
|
||
>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 26•19 years ago
|
||
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-
Comment 27•19 years ago
|
||
(And, sorry for taking so long :( )
Updated•19 years ago
|
Summary: Move GIF2.cpp into nsGIFDecoder2 → Move (merge) GIF2.cpp into nsGIFDecoder2
Assignee | ||
Comment 28•19 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)
Updated•19 years ago
|
Attachment #212376 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #212376 -
Flags: superreview?(tor)
Assignee | ||
Comment 29•18 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 | ||
Comment 30•18 years ago
|
||
Attachment #212376 -
Attachment is obsolete: true
Attachment #250974 -
Flags: superreview?(tor)
Attachment #212376 -
Flags: superreview?(tor)
Assignee | ||
Comment 31•18 years ago
|
||
Note, this patch results in 4K codesize savings.
Assignee | ||
Comment 32•17 years ago
|
||
After bug 317748 is done, I have an updated patch for this one.
Depends on: 317748
Assignee | ||
Comment 33•17 years ago
|
||
Attachment #250974 -
Attachment is obsolete: true
Attachment #267697 -
Flags: superreview?(tor)
Attachment #250974 -
Flags: superreview?(tor)
Comment 34•17 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•17 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•17 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•17 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)
Attachment #269417 -
Flags: superreview?(tor) → superreview+
Comment 38•17 years ago
|
||
Checked in for Alfred.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 39•17 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 .
Comment 40•17 years ago
|
||
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•17 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•17 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
Comment 43•17 years ago
|
||
Bug 386269 filed as a follow-up on the the delay threshold.
Comment 44•17 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.
Comment 45•17 years ago
|
||
I filed bug 386651 for adding image decoding regression tests.
Attachment #269740 -
Flags: superreview?(tor)
Assignee | ||
Comment 46•17 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
Updated•17 years ago
|
Flags: in-testsuite-
Assignee | ||
Updated•17 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•