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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Mayank Bansal]

Attachment: 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.

Comment 1

[Mayank Bansal]

the flicker doesnt start till the image gets loaded verticlly. This could initially take 3-4 seconds, before the black flicker starts.

Comment 2

[Alice0775 White]

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

Comment 3

[Seth Fowler [:seth] [:s2h]]

Thanks for the regression range, Alice0775. Investigating.

Comment 4

[Seth Fowler [:seth] [:s2h]]

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

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.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

[Tracking Requested - why for this release]: Flickering regression

Comment 7

[Seth Fowler [:seth] [:s2h]]

This ended up in 43, so the fix will need to be uplifted.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: If we detect animation during a full decode, drop the results of the full decode on the floor

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.

Comment 9

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15e3266a32d4

Comment 10

[Timothy Nikkel (:tnikkel)]

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?

Comment 11

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: If we detect animation during a full decode, drop the results of the full decode on the floor

This version of the patch uses the same surface flags as the old decoder for the
new decoder.

Comment 13

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecd96cf530ac

Comment 14

[Seth Fowler [:seth] [:s2h]]

That try job looks good to me. Seems like service workers are having a rough time right now...

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0530d695b6

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f0530d695b6

Comment 17

[Seth Fowler [:seth] [:s2h]]

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.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e921bf3f704