Closed Bug 1634839 Opened 1 year ago Closed 1 year ago

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

Categories

(Core :: ImageLib, defect)

71 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: strager.nds+firefox, Assigned: tnikkel)

References

Details

Attachments

(4 files)

Attached file 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.

Possibly related issue: #1565542

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

Blocks: 1565542
Component: Untriaged → ImageLib
Product: Firefox → Core
Flags: needinfo?(tnikkel)

Thanks for the very clear testcase!

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(tnikkel)

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.

Assignee: nobody → tnikkel
Status: NEW → ASSIGNED

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

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

Pushed by tnikkel@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Regressions: 1676172
You need to log in before you can comment on or make changes to this bug.