274244 - fails to render gif on both mac and windows; safari and ie both render fine

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Dave]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

The gif at http://twadzilla.com/gfx/arrows/l.gif fails to render both on mac and
windows firefox (1.0). However this same image displays fine in safari and
internet explorer, even ms paint.

Reproducible: Always
Steps to Reproduce:
1. Load http://twadzilla.com/gfx/arrows/l.gif in browser

Alternatively,
1. Save this image to disk.
2. Open the image using Firefox locally.

Actual Results:  
The browser displays a placeholder image indicating it failed to display.

Expected Results:  
A left-arrow image should be displayed.

This bug was repro'd on both Mac OS 10.3 (Safari 1.2.4; IE 5.2; Firefox 1.0) and
Windows XP SP2 (IE 6; Firefox 1.0).

Comment 1

[Boris Zbarsky [:bzbarsky]]

When I try to open this GIF in GIMP, I get a "GIF: ran off the end of my bits"
error message, and the GIF does not open....

Comment 2

[ArronM (:paper)]

Taking

Gif supplies the Explicit EOS Code, but rows still remain without data. We dump
the whole frame in that case, where we could display the partial image.  

Looks like what's needed is yet another "ignore the error" hack.

Comment 3

[ArronM (:paper)]

regression from fix of Bug 235298.  I'm working on a way to properly fix the
other bug, which will fix this one too.  Right now, nsCSSRendering relies on
whether the image has an alpha in order to determine whether to draw a
background for a NS_STYLE_BG_REPEAT_XY tiled image.  Bug 235298 "fixed" this by
throwing the corrupt image out, which causes this bug.

The patch I'm writing up introduces a GetNeedsBackgroundColor to gfxIImageFrame.
 gfxImageFrame first checks if the image has an alpha, and if it does, it
returns true.  Otherwise, it checks if the image is complete, and if it is,
returns true.  To check if the image is complete, it calls a new function
nsIImage::GetIsImageComplete(), which has platform specific code to check if all
the frame has data yet.  On most platforms, it's the same code.

Performance shouldn't be impacted.

This may bring out existing bugs (unfinished code) in the platform specific
DrawTile() code.  It does for Windows, which doesn't handle partially decoded
images perfectly in ProgressiveDoubleBlit (but does in fall-back routine in
DrawTile, so if all else fails, I can use the fall-back routine when the image
is not complete).  GTK appears to be ok, and I'm scared of Mac.  I'm assuming
Cairo is incomplete?

Comment 4

[ArronM (:paper)]

(In reply to comment #3)
> Otherwise, it checks if the image is complete, and if it is, returns true.

heh. what I really meant was if the image is _not_ complete, it returns true
(saying that someone has to draw the BG for it)

Comment 5

[ArronM (:paper)]

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

Comment 6

[ArronM (:paper)]

Attachment: Display more broken GIFs

What this patch does:

1) Rolls back part of Bug 235298, allowing broken images to be flushed and
marked as complete again (nsGIFDecoder.cpp).  This alone causes tiled partially
completed images to display like crap because no background is drawn for the
incomplete part.

2) Make gfxIImageFrame determine whether an image needs its background painted.
  Make nsCSSRendering call gfxIImageFrame when tiling and wanting to know if it
has to paint the background.

3) Add a nsIImage::GetIsImageComplete(..), needed for
gfxImageFrame::GetNeedsBackground()

4) Add platform specific code for GetIsImageComplete()

5) Fix up nsImageWin::ProgressiveDoubleBlit(..) so that it tiles partially
complete images properly

6) Force nsImageMac to tile slowly if image is partially complete

I think that's it.  It only looks like a big patch because of adding almost the
same code to all the platforms nsIImage implementation.

The results is that images with at least some data decoded will be displayed. 
The rest of the area defined by the image will be transparent.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

please change the IID of interfaces you change. that way, code compiled for the
old interface can handle the interface change gracefully (QI will fail) instead
of crashing.

Comment 8

[ArronM (:paper)]

Thanks beisi, I had no idea until now.

For anyone else interested, and for my future reference, a good URL describing
why one changes the UUID is here:
http://www.mozilla.org/projects/xpcom/book/cxc/html/weblock_finish.html

I won't attach a patch with the new UUID until more changes are required, or
checkin time, or upon request.

Comment 9

[ArronM (:paper)]

Attachment: Display more broken GIFs

Update to trunk.  Includes new UUID for gfxIImageFrame.

As per Bug 245407, Comment 71, I've allowed DrawTileQuickly() to be called for
incomplete images on Macs.

Comment 10

[ArronM (:paper)]

Comment on attachment 173342 [details] [diff] [review]
Display more broken GIFs

removing r/sr requests from old patch (I was hoping creating adding a new patch
would have done that for me)

Comment 11

[ArronM (:paper)]

Javier, this is regarding Bug 245407, Comment 71.  The patch in this bug enables
tiling of partially decoded (corrupt mid-stream) images.  If you have time,
could you test out the patch? I've set up a testcase at
http://mozilla.animecity.nu/IMGTest/BrokenBGImage01.html .  The page should tile
part of a gray-ish image.  The incomplete part of the image should be
transparent, and displayed as green, since the BODY's BGCOLOR is set to that.

For current mozilla builds,
http://mozilla.animecity.nu/IMGTest/BrokenBGImage01.html displays no background
image at all, just the green background.

Comment 12

[jhp (no longer active)]

ArronM, you're using the "nsImageMac" class in nsImageBeOS.cpp.

Comment 13

[jhp (no longer active)]

The patch seems to work fine for me on Mac OS X.

Comment 14

[ArronM (:paper)]

Attachment: Display More Broken GIFs

Double Thanks, Javier.	Patch updated to fix class name in BeOS

Comment 15

[ArronM (:paper)]

Attachment: Display more broken GIFs

-> Update to trunk

Stuart, any chance of getting this in before 1.8b2?

Comment 16

[ArronM (:paper)]

Checked In