Closed
Bug 143830
Opened 22 years ago
Closed 22 years ago
Garbage in transparent areas of animated gif during first loop
Categories
(Core Graveyard :: Image: Painting, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: paper, Assigned: dcone)
References
()
Details
(Keywords: perf, topembed+, Whiteboard: [adt2 RTM] [ETA 08/01])
Attachments
(2 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
dcone
:
review+
tor
:
superreview+
jud
:
approval+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
confirming, ccing dcone who should probably be following this...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•22 years ago
|
||
don, can you take a look at the issues here?
Component: ImageLib → Image: GFX
Reporter | ||
Comment 4•22 years ago
|
||
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)
Reporter | ||
Comment 5•22 years ago
|
||
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.
Attachment #83469 -
Attachment is obsolete: true
Reporter | ||
Comment 6•22 years ago
|
||
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
Reporter | ||
Updated•22 years ago
|
Updated•22 years ago
|
Keywords: mozilla1.0.1
a patch sitting in a bug for a month without review?
Reporter | ||
Comment 8•22 years ago
|
||
Same as last patch, except with fix of Bug 137128 included.
Attachment #83991 -
Attachment is obsolete: true
Reporter | ||
Comment 9•22 years ago
|
||
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)
Keywords: perf
Comment 10•22 years ago
|
||
Comment on attachment 90039 [details] [diff] [review]
Updated Patch
sr=tor
Attachment #90039 -
Flags: superreview+
Assignee | ||
Comment 11•22 years ago
|
||
This patch takes out the ability to to 8 bit alpha.. don't you think thats a
problem for things like PNG's ?
Reporter | ||
Comment 12•22 years ago
|
||
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).
Assignee | ||
Comment 13•22 years ago
|
||
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.
Attachment #90039 -
Flags: review+
Assignee | ||
Comment 14•22 years ago
|
||
Do you want me to check this in.. and watch for any bugs that may crop up?
Reporter | ||
Comment 15•22 years ago
|
||
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. :)
Assignee | ||
Comment 16•22 years ago
|
||
checked in.. and fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•22 years ago
|
Updated•22 years ago
|
Comment 17•22 years ago
|
||
tpreston: teri, can you pls verify this fix on the trunk? thanks!
Comment 18•22 years ago
|
||
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•22 years ago
|
||
This needs to get on the branch ***today***. Thanks.
Comment 20•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
addendum: fwiw, i had tested on linux rh7.2, win2k and mac 10.1.5.
Comment 23•22 years ago
|
||
Verified fixed win 98 trunk build 2002073008 (sorry was out of the office yesterday)
Status: RESOLVED → VERIFIED
Updated•22 years ago
|
Whiteboard: [adt2 RTM] [ETA 07/27] → [adt2 RTM] [ETA 07/31]
Comment 24•22 years ago
|
||
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
Updated•22 years ago
|
Attachment #90039 -
Flags: approval+
Comment 25•22 years ago
|
||
please checkin to the MOZILLA_1_0_BRANCH branch. once there, remove the
"mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Updated•22 years ago
|
Whiteboard: [adt2 RTM] [ETA 07/31] → [adt2 RTM] [ETA 08/01]
Assignee | ||
Updated•22 years ago
|
Keywords: mozilla1.0.1+ → fixed1.0.1
Comment 26•22 years ago
|
||
Verified fixed Win XP branch build 2002080608 and win ME branch build
2002080608, adding keyword verified1.0.1
Keywords: fixed1.0.1 → verified1.0.1
You need to log in
before you can comment on or make changes to this bug.
Description
•