1634839 - Off-screen decode() not resolving for cached >12 frame GIFs

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[strager.nds+firefox]

Attachment: firefox-bug.zip

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

Sometimes, the promise returned by
HTMLImageElement.prototype.decode does not resolve until the
HTMLImageElement is added to the document. If the image is
never added to the document, decode()'s promise never
resolves.

I think this buggy behavior in decode() occurs only if all
of the following is true:

• The image is a GIF animation with at least 12 frames.
• The server responded with HTTP status code 304 (Not
  Modified) and the image is loaded from Firefox' cache.

Steps to reproduce:

1. Download the attached .zip file and and extract the
   contained .html and .gif files.
2. Start a static web server for the files:
   python3 -m http.server 8001
3. Open http://localhost:8001/repro.html in Firefox.
4. Wait six seconds.
5. Click the browser's refresh button (without holding the
   SHIFT key).
6. Wait six seconds.

Actual results:

• For blab-10.gif, blab-11.gif, blab-11-200ms.gif, and
  blab-12.gif, .decode() resolves promptly, so those images
  get a green background.
• For blab-13.gif, blab-14.gif, blab-15.gif, and
  blab-15-50ms.gif:
  • The promises returned by .decode() for these images do
    not resolve within 5 seconds.
  • The promises returned by .decode() for these images
    resolve after adding the image elements to the
    document.
  • These images get a red background indicating that
    .decode() did not resolve within 5 seconds.

I observed these results on Mozilla Firefox for Mint 71.0
mint - 1.0 with a fresh Firefox profile on a Linux machine.

Name: Firefox
Version: 71.0
Build ID: 20191210124657
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0
OS: Linux 5.0.0-25-generic

Expected results:

• The promise returned by .decode() for each .gif image
  resolves promptly.
• Each image gets a green background indicating that
  .decode() resolved within 5 seconds.

Related observations:

• If I refresh the page by clicking the refresh button with
  the SHIFT key held, I always get the expected results.
• If I refresh the page by clicking on the URL bar then
  pressing the ENTER key, the behavior depends on how I
  loaded the page previously.
  • If I previously loaded the page by clicking the refresh
    button with the SHIFT key held, pressing the ENTER key
    in the URL bar always gives the expected results.
  • If I previsouly loaded the page by clicking the refresh
    button without the SHIFT key held, pressing the ENTER
    key in the URL bar always gives the bad/observed
    results.
• If I open the page directly in Firefox without the web
  server (i.e. using a file:// URL), I always get the
  expected results.

Comment 1

[strager.nds+firefox]

Possibly related issue: #1565542

Comment 2

[Alice0775 White]

Yep
Before bug 1565542 : all red
After landing bug 1565542: same results of comment #0

Comment 3

[Timothy Nikkel (:tnikkel)]

Thanks for the very clear testcase!

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1634839. Fix img.decode for animated images that aren't in the document. r?aosmond

The img.decode promise for animated images that aren't in the document might not resolve in common cases. Specifically for animated images that have been decoded and then discarded, so that mCompositedFrameInvalid is true.

mCompositedFrameInvalid is cleared by either:

1. the image is fully decoded and NotifyDecodeComplete is called.
2. RequestRefresh is able to advance to the current frame.

We don't call RequestRefresh on animated images that aren't in the document and we only decode a certain number of frames ahead of the current frame (even for fully retained animation buffers), so neither of these will happen with long enough animated images.

So we need to make sure that mCompositedFrameInvalid eventually gets cleared any time a decode is requested (which the FrameAnimator gets notified about via GetCompositedFrame). We do that be calling UpdateState in GetCompositedFrame whenever mCompositedFrameInvalid is true and the image is in the surface cache.

This runs into a second problem: UpdateState uses |mIsCurrentlyDecoded = aResult.Surface().IsFullyDecoded()| to determine when to clear mCompositedFrameInvalid. As mentioned above, we don't fully decode animated images. Further, for animated images that are large enough we use a discarding animation buffer that never keeps around all the frames and so IsFullyDecoded always returns false. So we replace that with a check that we can seek to the current animated frame index.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1634839. Teach RasterImage::NotifyDecodeComplete to deal with getting notified about new frames. r?aosmond

It takes a frame count argument but probably never gets a value we haven't already seen in a NotifyProgress call, so this doesn't fix anything as far as I know.

Depends on D73564

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1634839. Call UpdateState on the animation state in RasterImage::NotifyProgress. r?aosmond

Since AnimationState::UpdateState now depends on a specific frame of the animation being in the surface cache we need to call UpdateState whenever we add a new frame.

Depends on D73583

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/471ad96ddc3e
Fix img.decode for animated images that aren't in the document. r=aosmond
https://hg.mozilla.org/integration/autoland/rev/d1ceba243d8f
Teach RasterImage::NotifyDecodeComplete to deal with getting notified about new frames. r=aosmond
https://hg.mozilla.org/integration/autoland/rev/2c28e44f5823
Call UpdateState on the animation state in RasterImage::NotifyProgress. r=aosmond

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/471ad96ddc3e
https://hg.mozilla.org/mozilla-central/rev/d1ceba243d8f
https://hg.mozilla.org/mozilla-central/rev/2c28e44f5823