488685 - Cruft on GIF image

Status: RESOLVED FIXED

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

Description

[John P Baker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre
Build Identifier: 

At certain zoom levels there is cruft on the right hand side of GIF image - this has appeared, on branch, in last day or two.

Reproducible: Always

Steps to Reproduce:
1. Go to data:image/gif;base64,R0lGODlhEAAQAPAAALuWAAAAACH5BAUIAAEALAMAAwAKAAoAAAIMDI5nye0Po5xutVAAADs=
2. Change zoom if required


Expected Results:  
square with rounded corners

There is (should be) a transparent region round the gif data. I seem to remember this happening before - a year or two back.

Comment 1

[John P Baker]

Attachment: screenshot

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That's a little weird.. I can definitely see it.

Comment 3

[Joe Drew (not getting mail)]

Jeff thinks it might be Cairo.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=abad3a76f2d4&tochange=9bda3eab3b2d

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Then it's almost certainly bug 479852; I'm not sure why we're triggering any of that code when we're not tiling, though, and since the testcase is a gif image itself then we can't be.  Is there anything special about this gif?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This looks to be an image with a 3x3 padding around a central 10x10 area.

When we first enter into Draw, we have:

subimage.X  0.000000 subimage.XMost  16.000000
imageRect.X 0.000000 imageRect.XMost 16.000000
padding: 1 [3,3]
decoded: 0 0 16 10

^ the decoded width looks weird to me, not sure why it's not 10?

After we do the if (doPadding) block, we end up with
subimage  X..XMost 0..13
imageRect X..XMost 0..10

And so we trigger the subimage.xmost > imagerect.xmost check which sets doTileX.

Then right before we do the drawing, we have:

mWidth: 10 mHeight: 10 doTileX: 1 doTileY: 0

This behaviour changed because we used to compute doTileX/doTileY before we munged stuff for padding, and now we compute it later on.  I can move the computation earlier in the function, but I'm not sure if there isn't something else going on here -- we have a 3px padding on all sides, but we seem to be trusting the 'available' rect to take care of the bottom/right padding... but here, the decoded width is (I think) bogus, and so it breaks down.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is exposing a whole bunch of fun bugs.  I think there are two:

1) nsThebesImage::ImageUpdated blindly assumes that the passed-in rect is sane at http://hg.mozilla.org/mozilla-central/file/tip/gfx/src/thebes/nsThebesImage.cpp#l246 .  In this case, mWidth/mHeight = 10, but we're happily setting mDecoded.width to 16 with that UnionRect.  This has the side effect of GetIsImageComplete() right below never returning TRUE.

2) the GIF decoder calls ImageUpdated with the bad rect here: http://hg.mozilla.org/mozilla-central/file/93bc0760d0d2/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp#l205 .  I think it should be using mGIFStruct.width (the width of the current frame) as opposed to screen_width.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add bounds check in ImageUpdated

Just the change in nsThebesImage::ImageUpdated is sufficient to fix this bug, but I would like to see the gif decoder bug fixed as well.

I'll put together a reftest based on the gif here.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 373180 [details] [diff] [review]
add bounds check in ImageUpdated

Actually, wouldn't it make more sense to just assert that boundsRect contains mDecoded here, and fix the decoder(s) so that assert doesn't fire?

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #9)
> (From update of attachment 373180 [details] [diff] [review])
> Actually, wouldn't it make more sense to just assert that boundsRect contains
> mDecoded here, and fix the decoder(s) so that assert doesn't fire?

It might, though that requires someone to understand the gif decoder :-)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we'd better bite that bullet or we're just saving up trouble for later.

Comment 12

[John P Baker]

(In reply to comment #7)
> 2) the GIF decoder calls ImageUpdated with the bad rect here:
> http://hg.mozilla.org/mozilla-central/file/93bc0760d0d2/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp#l205
> .  I think it should be using mGIFStruct.width (the width of the current frame)
> as opposed to screen_width.

[In case it saves someone a few minutes of archaeology] That code was introduced in bug 366465 .

Comment 13

[Alfred Kayser]

Indeed, the issue is that the image is one frame of the 'animation', and can be smaller than the 'screen'. 
Replacing:
   205   nsIntRect r(0, fromRow, mGIFStruct.screen_width, rows);
with:
   205   nsIntRect r(0, fromRow, mGIFStruct.width, rows);
should do the correct thing.

The rectangle 'r' is passed to the imgFrame, which is also sized to the frame width and height.

This has been not much of a problem until now. screen_width is always bigger or equal to width, so that the update rect is always large enough. 
But now with different rendering options in Cairo having a too big rectangle causes the edge effect.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in the ImageUpdated fix; joe, want to fix the GIF decoder too?  I'd r+ the quick gif .width change.

Comment 15

[Daniel Holbert [:dholbert]]

This landed on 1.9.1 as: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b9f7eb42b83
I renamed the reftests to fix the orange as: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/11c7c5ac23bc

Comment 17

[John P Baker]

Marking this fixed doesn't seem to take into account comment #9 when roc withdrew his sr+ and comment #11

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I do think that on trunk we should back this patch out, replace it with an assertion, and fix the GIF decoder so the assertion doesn't fire.

Comment 19

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090423 Minefield/3.6a1pre ID:20090423040020

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090423 Shiretoko/3.5b4pre ID:20090423040926

Comment 20

[Alfred Kayser]

Attachment: Patch to do the correct thing in nsGIFDecoder2.cpp

Comment 21

[Alfred Kayser]

Reopened to get the patch in nsGIFDecoder2.cpp applied.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in the nsGIFDecoder2 patch.
http://hg.mozilla.org/mozilla-central/rev/e50fc11c203a

I think we should take another patch in this bug to backout Vlad's change to ImageUpdated and instead add an assertion that boundsRect contains mDecoded, then we can close this out.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Could you guys please file a follow-up bug for the nsGIFDecoder2 patch and leave this one FIXED? It's pretty tricky to track bugs that have different fixes on different branches (though admittedly, yes, we do it for branch-safe versions and applicability changes)

Comment 24

[Joe Drew (not getting mail)]

Bug 494229 is that follow-up bug.

Comment 25

[Jonathan Kew [:jfkthame]]

Attachment: make the reftest more robust by forcing small font size

The reftest here is fragile if the browser font size is increased, or if the default font is changed to one with larger ascent/descent metrics such as Charis SIL. This causes breakage because the position of the <img> in the reftest changes as the default line height increases, whereas the background-image rendering in the reference stays in a fixed position.

The attached patch forces a very small font size in the test file, to make it more robust in the face of different font settings in the profile.

Comment 26

[Joe Drew (not getting mail)]

This would apply to all of our <img>-based reftests. Shouldn't we find some way to fix all of them? Could we convert them all to background-image?

Comment 27

[Bobby Holley (:bholley)]

(In reply to comment #26)
> This would apply to all of our <img>-based reftests. Shouldn't we find some way
> to fix all of them? Could we convert them all to background-image?

We still need coverage for <img> as well, no?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Making the <img> display:block should make its positioning independent of font metrics.

Comment 29

[Joe Drew (not getting mail)]

Comment on attachment 401857 [details] [diff] [review]
make the reftest more robust by forcing small font size

Can you use roc's suggestion instead?