Closed Bug 1132427 Opened 5 years ago Closed 5 years ago

Badly rendered GIF on imgur

Categories

(Core :: ImageLib, defect, P4)

x86_64
All
defect

Tracking

()

VERIFIED FIXED
mozilla38
Tracking Status
firefox36 --- unaffected
firefox37 + verified
firefox38 --- verified

People

(Reporter: stevee, Assigned: tnikkel)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150211222327 CSet: 3094601af679

1. New profile, start firefox.
2. Navigate to http://imgur.com/gallery/rZ4FQoK

Expected:
- Properly rendered GIF

Actual:
- Malrendered GIF. After last frame the GIF doesn't reset back to the first frame cleanly.

Strangely if you view the image directly at http://i.imgur.com/rZ4FQoK.gif then it looks fine, it is just misrendered when viewing it at http://imgur.com/gallery/rZ4FQoK

Fanolian's capture shows the problem with the rendering if you are unable to observe it: http://gfycat.com/BelovedTameAcornbarnacle

Not sure if this is a regression..
[Tracking Requested - why for this release]:

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b3a1efe7900a&tochange=59ceeca79341

Regressed by: Bug 1130328
Blocks: 1130328
Flags: needinfo?(seth)
I can reproduce on windows7 without HWA. And I can also reproduce on ubuntu with blocked HWA.
Keywords: regression
OS: Windows 7 → All
I can also reproduce with regardless of HWA on/off.
(In reply to Steve England [:stevee] from comment #0)
> Strangely if you view the image directly at http://i.imgur.com/rZ4FQoK.gif
> then it looks fine, it is just misrendered when viewing it at
> http://imgur.com/gallery/rZ4FQoK

The version embedded by BugzillaJS also exhibits the problem.
Jet, AFAIK, Seth is in PTO.
We are in a bad position right now. Bug 1130328 was supposed to fix a bunch of regression but introduced this bug.

Matt, as you did the review of bug 1130328, can you help with this? Thanks
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugs)
I'll look at this today.
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Assignee: matt.woodrow → tnikkel
Attached patch patchSplinter Review
The first frame refresh area is just being calculating wrong. It only gets updated for the second frame because that code was wrongly inside the "this image has become animated" if. We need to update it for every frame of the image, not just the second.
Flags: needinfo?(seth)
Flags: needinfo?(bugs)
Attachment #8563748 - Flags: review?(jmuizelaar)
Comment on attachment 8563748 [details] [diff] [review]
patch

Review of attachment 8563748 [details] [diff] [review]:
-----------------------------------------------------------------

Add a test please.
Attachment #8563748 - Flags: review?(jmuizelaar) → review+
Working on a test now, it's a little tricky because exposing this bug requires a looping gif. But given the time pressure on getting this fix into beta I pushed it now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/109c66867717
Indeed! thanks guys for the reactivity.
Timothy, could you fill the uplift request to aurora & beta right now? Thanks
Flags: needinfo?(tnikkel)
Comment on attachment 8563748 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1130328 exposed this problem. However http://hg.mozilla.org/mozilla-central/rev/d5462de77e9f bug 1079627 added the incorrect code.
[User impact if declined]: looping animated gifs will draw incorrectly
[Describe test coverage new/current, TreeHerder]: none yet, writing a test as I write this
[Risks and why]: this should be very low risk, the code is clearly wrong.
[String/UUID change made/needed]: none
Flags: needinfo?(tnikkel)
Attachment #8563748 - Flags: approval-mozilla-beta?
Attachment #8563748 - Flags: approval-mozilla-aurora?
Attachment #8563748 - Flags: approval-mozilla-beta?
Attachment #8563748 - Flags: approval-mozilla-beta+
Attachment #8563748 - Flags: approval-mozilla-aurora?
Attachment #8563748 - Flags: approval-mozilla-aurora+
Oddly, I do not see this problem on Firefox 36 beta 8 or the 2015-02-13 Aurora build.

On Aurora, the image either does not animate at all or *doesn't loop* (Hawking shows up, then never goes away). I can fix this by going to the direct link and force reloading (Ctrl + F5). After that, the image animates correctly both directly and viewed through imgur, and does not exhibit this problem. I'm guessing the animation problems have to do with bug 1119938 and the patches fixing it that are still waiting for uplift.
https://hg.mozilla.org/mozilla-central/rev/109c66867717
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
This needs rebasing for uplift to both branches.
Flags: needinfo?(tnikkel)
So it turns out the buggy code isn't on beta or aurora. It's only on nightly, since the buggy code came from bug 1079627, which has target milestone of 38.

No comments in this bug indicate that they can see the bug in aurora or beta. So this is probably a nightly only bug. If anyone can see this bug on beta please speak up, but it will most likely have a different cause and require a different patch.
Flags: needinfo?(tnikkel)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150214030238 CSet: a7c177546ca0

Now WFM.
Status: RESOLVED → VERIFIED
I can confirm this works fine now in the latest Firefox 38 Nightly (Windows 7 x64). It was reproducible in Firefox 38 Nightly from February 11th. 

Also the issue does not show for the latest Firefox 37 Aurora (had to Ctrl+Shift+R for the gif to animate) and 36 Beta 9. I'm adjusting the flags accordingly if no one minds.
Attachment #8563748 - Flags: approval-mozilla-beta+
Attachment #8563748 - Flags: approval-mozilla-aurora+
Comment on attachment 8563748 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: 1079627
[User impact if declined]: Issues with animated GIF rendering.
[Describe test coverage new/current, TreeHerder]: We have a test.
[Risks and why]: Low risk - this is a very small fix.
[String/UUID change made/needed]: None.
Attachment #8563748 - Flags: approval-mozilla-aurora?
Comment on attachment 8563748 [details] [diff] [review]
patch

Trivial fix for a regression that simply corrects how the if statement is enclosed. Aurora+
Attachment #8563748 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8563748 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Bug fix for bug 1079627.
[User impact if declined]: Incorrect animated GIF rendering.
[Describe test coverage new/current, TreeHerder]: In 38 for a long time. Has a test.
[Risks and why]: Very low risk; in 38 for a long time. Was already approved for uplift to 37 when 37 was Aurora, but was backed out as collateral damage from an unrelated backout, even though there were no problems with this patch.
[String/UUID change made/needed]: None.
Attachment #8563748 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Comment on attachment 8563748 [details] [diff] [review]
patch

OK. Let's try this again. Beta+ this time.
Attachment #8563748 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Contact: florin.mezei
A quick check on Windows 7 x64 using Firefox 37 Beta 6 does not show this issue.
You need to log in before you can comment on or make changes to this bug.