Status

()

defect
P2
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: john.p.baker, Assigned: vlad)

Tracking

(Blocks 1 bug, {regression, verified1.9.1})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments)

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.
Flags: blocking1.9.1?
Keywords: regression
Version: unspecified → 1.9.1 Branch
Posted image screenshot
That's a little weird.. I can definitely see it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Jeff thinks it might be Cairo.
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?
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.
Assignee: nobody → vladimir
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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.
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.
Attachment #373180 - Flags: superreview?(roc)
Attachment #373180 - Flags: review?(joe)
Attachment #373180 - Flags: review?(joe) → review+
Attachment #373180 - Flags: superreview?(roc) → superreview+
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?
Attachment #373180 - Flags: superreview+
(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 :-)
I think we'd better bite that bullet or we're just saving up trouble for later.
(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 .
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.
OS: Windows 2000 → All
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
Checked in the ImageUpdated fix; joe, want to fix the GIF decoder too?  I'd r+ the quick gif .width change.
Flags: in-testsuite+
Keywords: fixed1.9.1
Duplicate of this bug: 489564
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Marking this fixed doesn't seem to take into account comment #9 when roc withdrew his sr+ and comment #11
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.
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
Status: RESOLVED → VERIFIED
Attachment #374441 - Flags: review? → review?(vladimir)
Attachment #374441 - Flags: review?(vladimir) → review+
Attachment #374441 - Flags: superreview?(pavlov)
Attachment #374441 - Flags: superreview?(pavlov) → superreview+
Reopened to get the patch in nsGIFDecoder2.cpp applied.
Status: VERIFIED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
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.
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)
Bug 494229 is that follow-up bug.
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
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.
Attachment #401857 - Flags: review?
Attachment #401857 - Flags: review? → review?(joe)
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?
(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?
Making the <img> display:block should make its positioning independent of font metrics.
Attachment #401857 - Flags: review?(joe) → review-
Comment on attachment 401857 [details] [diff] [review]
make the reftest more robust by forcing small font size

Can you use roc's suggestion instead?
You need to log in before you can comment on or make changes to this bug.