143830 - Garbage in transparent areas of animated gif during first loop

Status: VERIFIED FIXED

(Core Graveyard :: Image: Painting, defect, P2)

Description

[ArronM (:paper)]

This seems to be effecting Windows 98 only.

Build 2002051004
View URL.  On the first loop of this animation, garbage is seen.  You may need
to change your default background to a darker color to see it.

I've traced it down to the ugly thing that is called nsImageWin::DrawToImage
I've came up with two solutions.  

#1 Change 3 lines of DrawToImage.

#2 Replace DrawToImage with GTK's DrawToImage.  This essentially disables the
"Opimizations" and produces 1.9x faster rendering.

Both options have been tested and work nicely.

I'll be attaching option #2 in the next few days unless there are any objections
or concerns.

Comment 1

[Boris Zbarsky [:bzbarsky]]

confirming, ccing dcone who should probably be following this...

Comment 2

[Stuart Parmenter]

don, can you take a look at the issues here?

Comment 3

[dcone (gone)]

ok.. I will take this

Comment 4

[ArronM (:paper)]

Attachment: Replace DrawToImage with GTK's version

I've essentially replaced the DrawToImage code with the GTK's version.	The
differences from the GTK version are:

- switched code to handle Bottom-Up data storage

- One comment in the GTK version was "// fix - could speed up by gathering a
run of 0xff's and doing 1 memcpy" so I did that.  I'll make a bug for applying
the gathering speedup thing to the GTK code if this patch goes well.

- added code for the case of the image being already optimized.

This will fix the problem, and speed things up.  I speed tested this on a Win98
K6-2 400, and a Win2k PIII-933 at various video modes using Attachment 47940 [details] as
a test subject.  CPU usage dropped by about 5% on all tests.  Not as big as a
perf gain as I first thought, but it's still good.

I realize that the old code used Windows GDI calls in expectation that the
Windows code would run faster (or the code on the video board if Windows passes
control to it), but I haven't found a case yet where the Windows GDI calls are
faster.  There seems to be just too much overhead with setting up the GDI calls
or something like that.  Try out the link in this comment with a nightly build
and a build with this patch and compare for yourself (I look forward to hearing
the results)

Comment 5

[ArronM (:paper)]

Attachment: Simpler Patch

The "could speed up by gathering a run of 0xff's and doing 1 memcpy"
enhancement I added did not work.  I removed it, and now the code looks even
closer to GTK's.

Comment 6

[ArronM (:paper)]

Attachment: "diff -wbBu" on GTK's DrawToImage and the new Window's one

I took the DrawToImage function of nsImageGTK and DrawToImage of nsImageWin
(after the patch) and did a "diff -wbBu" on them.  If you are familiar with the
GTK's code, this'll make the patch easier to review.  If not, ignore it :P

Comment 7

[[e-mail] (gone - use [e-mail] instead)]

a patch sitting in a bug for a month without review?

Comment 8

[ArronM (:paper)]

Attachment: Updated Patch

Same as last patch, except with fix of Bug 137128 included.

Comment 9

[ArronM (:paper)]

By "last patch" I mean, it's the same as "Simpler Patch"

I can now prove that this patch is a huge perf gain.

View Attachment 85488 [details] with and without this patch (on Windows) with the mouse
NOT hovering over the image.  Without the patch I get 85% CPU usage.  With the
patch, it goes down to 45%.  System is Win2k, PIII-933, NVidia Vanta @ 16bit color.

If someone else can confirm this, perhaps the bug will get more attention.  You
will need a branch build from July 2nd or later, and a rather fast windows
machine.  I can probably put up a .dll with the patch in if someone wants to
test but they aren't a compiler-type person. (I won't if no one asks)

Comment 10

[tor]

Comment on attachment 90039 [details] [diff] [review]
Updated Patch

sr=tor

Comment 11

[dcone (gone)]

This patch takes out the ability to to 8 bit alpha.. don't you think thats a
problem for things like PNG's ?

Comment 12

[ArronM (:paper)]

dcone:
DrawToImage is only used by the animated gif code, which only has 0 or 1 bit
alpha.  PNGs have their own internal library for image manipulation.  I could
not be positive that the 8bit code even worked (since it never runs, nor has it
ever have run), so I took it out.  

If sometime in the future we are going to call DrawToImage with a 8 bit alpha,
then we are going to have to build code for OS/2 & GTK, since both of these do
not handle 8bit DrawToImage yet. On a side note, a good portion of platforms
that do support 8bit alpha in DrawToImage do so because with no additional code
(their native OS GDI calls just handle it).

Comment 13

[dcone (gone)]

Comment on attachment 90039 [details] [diff] [review]
Updated Patch

r=dcone.  This makes should make things fast.  I would put at the top.. that
this will not work for 8 bit alpha just in-case this needs support later.

Comment 14

[dcone (gone)]

Do you want me to check this in.. and watch for any bugs that may crop up?

Comment 15

[ArronM (:paper)]

Yes :) (once the tree is open that is).  That way you can add a comment about 
not supporting 8bit alpha if you want, and save me from making another patch. :)

Comment 16

[dcone (gone)]

checked in.. and fixed.

Comment 17

[Jaime Rodriguez, Jr.]

tpreston: teri, can you pls verify this fix on the trunk? thanks!

Comment 18

[Susie Wyshak]

Hey, the 7/26 trunk also fixes:
http://dynamic.espn.go.com/espn/chat/chatESPN?event_id=2083

(which is very choppy on 7/23 branch)

Comment 19

[Susie Wyshak]

This needs to get on the branch ***today***. Thanks.

Comment 20

[Jaime Rodriguez, Jr.]

Teri needs to very this as fixed, and that it causes no regressions, before we
can take it on the branch. It will also need Drivers approval in addition to the
ADT's approval.

Teri, can you pls verify this as fixed? thanks!

Comment 21

[sairuh (rarely reading bugmail)]

i tried to verify this by comparing what i see with both today's trunk (with
fix) and branch (without fix) builds, but i cannot tell the difference visually.
i tried both the sample URL, as well as the espn.com link in comment 18. i've
darkened my default background color, and all i see (for the test URL) is the
animation of the "113553" text --i couldn't detect any garbage kruft with the
branch build.

Comment 22

[sairuh (rarely reading bugmail)]

addendum: fwiw, i had tested on linux rh7.2, win2k and mac 10.1.5.

Comment 23

[Terri Preston]

Verified fixed win 98 trunk build 2002073008 (sorry was out of the office yesterday)

Comment 24

[Paul Wyskoczka]

Adding adt1.0.1+ on behalf of the adt for checkin to the 1.0 branch.  Please get
drivers approval before checking in. When you check this into the branch, please
add keyword fixed1.0.1

Comment 25

[Judson Valeski]

 please checkin to the MOZILLA_1_0_BRANCH branch. once there, remove the
"mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword. 

Comment 26

[Terri Preston]

Verified fixed Win XP branch build 2002080608 and win ME branch build
2002080608, adding keyword verified1.0.1