A large GIF on slow network with an invalid frame delay on the first frame will flicker black till it loads completely

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mayankleoboy1, Assigned: seth)

Tracking

({regression})

43 Branch
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ fixed, firefox44 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 1 obsolete attachment)

Posted file aboutsupport.txt
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20150818030209

Steps to reproduce:


1.Take a slow intenet connection. 50KB/s-100KB/s should repro this beautifully.

2. Take a large GIF image : http://ctho.org/tmp/graphics/raytracer/movie/out.gif

3. Open the URL


3. Open the URL


Actual results:

Till the GIF loads completely, the GIF will violently flicker black. 


Expected results:

Probably shouldnt.
the flicker doesnt start till the image gets loaded verticlly. This could initially take 3-4 seconds, before the black flicker starts.
Reproduced, ADSL 12M

https://hg.mozilla.org/mozilla-central/rev/90d9b7c391d38ae118865bd87b5d011feee6dded
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150818030209


Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=54d0fc9946067a995cc49326fd3861303cd1f105&tochange=c035b63e1b81

Regressed by: Bug 1194059
Blocks: 1194059
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(seth)
Version: Trunk → 43 Branch
Thanks for the regression range, Alice0775. Investigating.
Flags: needinfo?(seth)
So this GIF has a zero frame delay on the first frame, despite being animated. That causes us to hit the RecoverFromInvalidFrames() fallback case in RasterImage::FinalizeDecoder(). Presumably something is going wrong during the recovery attempt.
Summary: A large GIF on slow network will flicker black till it loads completely → A large GIF on slow network with an invalid frame delay on the first frame will flicker black till it loads completely
Hmm, I bet the reason the slow network is relevant is that RecoverFromInvalidFrames() fails to do a sync decode, because we had enough data to decode the header of the second frame and discover the animation, but not enough data to complete the decode of the entire image synchronously.

I can definitely see that wreaking some havoc, because it means that we'll set RasterImage::mHasBeenDecoded to |true| before the image actually gets decoded completely, and that probably ends up breaking all sorts of stuff.

We need to handle this recovery in a more robust way, clearly.
[Tracking Requested - why for this release]: Flickering regression
Whiteboard: [gfx-noted]
This ended up in 43, so the fix will need to be uplifted.
Here's the patch. The crucial difference is that when we detect that we need to
drop all surfaces and recover, we drop the results of the full decode on the
floor, so that we don't end up setting |mHasBeenDecoded| to true and telling
observers that the image has been fully decoded.

In local testing this corrects the problem.
Attachment #8665181 - Flags: review?(tnikkel)
Assignee: nobody → seth
Status: NEW → ASSIGNED
Comment on attachment 8665181 [details] [diff] [review]
If we detect animation during a full decode, drop the results of the full decode on the floor

>+  bool metadataOK = SetMetadata(aDecoder->GetImageMetadata(), wasMetadata);
>+  if (!metadataOK) {
>+    // This indicates a serious error that requires us to discard all existing
>+    // surfaces and redecode to recover. We'll drop the results from this
>+    // decoder on the floor, since they aren't valid.
>+    aDecoder->TakeProgress();
>+    aDecoder->TakeInvalidRect();
>+    RecoverFromInvalidFrames(mSize, DECODE_FLAGS_DEFAULT);
>+    return;
>+  }

Hmm, shouldn't we be using the decode flags that the (old) Decoder has set when calling RecoverFromInvalidFrames?
Attachment #8665181 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #10)
> Hmm, shouldn't we be using the decode flags that the (old) Decoder has set
> when calling RecoverFromInvalidFrames?

Yeah, that seems like a good idea. I'll update the patch.
This version of the patch uses the same surface flags as the old decoder for the
new decoder.
Attachment #8665181 - Attachment is obsolete: true
That try job looks good to me. Seems like service workers are having a rough time right now...
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f0530d695b6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8665188 [details] [diff] [review]
If we detect animation during a full decode, drop the results of the full decode on the floor

Approval Request Comment
[Feature/regressing bug #]: 1194059
[User impact if declined]: Extremely ugly flickering while animated GIFs load from the network.
[Describe test coverage new/current, TreeHerder]: Not clear how to write a test for this. =( On m-c.
[Risks and why]: Pretty low risk, especially at this point in the cycle. The change here is not conceptually very big.
[String/UUID change made/needed]: None.
Attachment #8665188 - Flags: approval-mozilla-aurora?
Comment on attachment 8665188 [details] [diff] [review]
If we detect animation during a full decode, drop the results of the full decode on the floor

Good to catch this early! Approved for uplift
Attachment #8665188 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.