55997 - Image library causing unnecessary timers. Many unnecessary timers.

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Mike Kaply [:mkaply]]

Check out :

http://lxr.mozilla.org/seamonkey/source/modules/libimg/gifcom/gif.cpp#1517

The problem is that 0 length delays are always being set to MINIMUM_DELAY_TIME 
(10). They should actually be going down the GETN(1, gif_image_start); path.

This is causing tons of unecessary timers to be created.

Here is the correct code:

                if (gs->delay_time){
                  if(gs->delay_time < MINIMUM_DELAY_TIME )
                      gs->delay_time = MINIMUM_DELAY_TIME;
                    if(ic->imgdcb){
                          gs->delay_timeout = (void *)
                                
ic->imgdcb->ImgDCBSetTimeout(gif_delay_time_callback, gs->ic, gs->delay_time);
                     }
                    /* Essentially, tell the decoder state machine to wait
                       forever.  The timeout callback routine will wake up the
                       state machine and force it to decode the next image. */
                    GETN(1L<<30, gif_image_start);
                    gs->state = gif_delay;
                } else {
                    GETN(1, gif_image_start);
                }


The if check for MINIMUM_DELAY_TIME should be in the second if.

Comment 1

[Mike Kaply [:mkaply]]

I'm putting an RTM+ in here because someone in Netscape land needs to 
look at this before shipping Netscape 6.

This is a genuine performance bug.

Comment 2

[tor]

Attachment: patch version of mkaply's change

Comment 3

[tor]

Removing {rtm+] from status - that should only be added once you have r/sr.
Looks right, and hasn't caused problems for the couple days I've had it in
my tree.

pnunn, could you review this?

Comment 4

[pnunn]

At one time there was a Good Reason for the delay time
to always be set at the minimum value. But that was the old
code base. Time(rs) have changed. I'll need to pound on this
one alittle to verify its goodness.
-p

Comment 5

[Brendan Eich [:brendan]]

mkaply mailed me enquiring about when this might get r=/sr= (I think he composed
his mail to me before pnunn's update of today).  I don't know, but I thought I'd
ask in the bug.

/be

Comment 6

[selmer (gone)]

How bad is the performance problem?  Can we go ahead with reviews?  (If this
doesn't happen ASAP, it's going to be minus by default because the RTM will have
shipped.)

Comment 7

[Phil Peterson]

PDT marking [rtm-]. Looks like this patch has been languishing.

Comment 8

[Brendan Eich [:brendan]]

Yes, the patch is languishing.  Pam, can we check it into the trunk so that 
everyone can pound on it?

/be

Comment 9

[pnunn]

I've been testing the patch in my local tree and
it works fine (linux & NT). There was a particular
reason why a minimun delay time was _always_ set before
the timer code was changed. It appears a minimum of
zero is now ok.

This patch also appears to help fix or alleviate #50786,
where the clean up after end of stream wins (or loses ) a race with the
clean up after a bad image and crashes.

r:pnunn for trunk
who gives an sr: ?

Comment 10

[Brendan Eich [:brendan]]

I reviewed this earlier, and while reviewing bug 53597, and even whined about 
the non-four-space indentation at the deepest nesting level shown in the patch. 
Anyway, sr=[e-mail].

/be

Comment 11

[pnunn]

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

Comment 12

[pnunn]

Make that bug#58071 (not 50786).

(so thats what that loud, constant background noise is...)

Comment 13

[pnunn]

My trunk build isn't up to date.
Anyone have a fresh tree want to check this in?
-p

Comment 14

[Mike Kaply [:mkaply]]

Since it was originally mine, I'll check it in.

Comment 15

[pnunn]

much thanks.

Comment 16

[Mike Kaply [:mkaply]]

Fix checked in.

And I fixed a couple of the bad indentations that brendan was talking about.

Comment 17

[Terri Preston]

Coding issue, marking verified

Comment 18

[pnunn]

 
MK:
would you back out this patch for the minimum timer delay?
See bug#59494.
-pn

Comment 19

[pnunn]

All pnunn bugs reassigned to Pav, who is taking over
the imglib.

Comment 20

[Stuart Parmenter]

New imagelib doesn't use timers for anything except animations.
old code no longer used.  marking fixed.

Comment 21

[Terri Preston]

Verified