Skip to content

fix: Avoid decoding static images again when a listener is re-added - #1062

Open
shihabkandil wants to merge 5 commits into
Baseflow:mainfrom
shihabkandil:fix/static-image-redecode-on-relisten
Open

shihabkandil wants to merge 5 commits into
Baseflow:mainfrom
shihabkandil:fix/static-image-redecode-on-relisten

Conversation

@shihabkandil

Copy link
Copy Markdown

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Bug fix

⤵️ What is the current behavior?

When an image stream loses all its listeners and gets one back, MultiImageStreamCompleter.addListener asks the codec for a new frame, even for a static image that was already decoded. This happens every time a route is covered and uncovered, because Image stops listening while TickerMode is off.

On native this is only wasted work. On web, in Firefox and Safari (where Flutter does not use the browser ImageDecoder), the second decode comes back blank, so images on a screen turn into empty boxes after navigating to another route and back.

🆕 What is the new behavior (if this is a feature change)?

A static image that has already emitted a frame is not decoded again when a listener is re-added; the listener gets the current image. Animated images still resume decoding. This is the same condition Flutter's MultiFrameImageStreamCompleter.addListener uses. _currentImage is private to the framework, so _framesEmitted is used instead.

💥 Does this PR introduce a breaking change?

No

🐛 Recommendations for testing

  • flutter test: the new test fails on main (numFramesAsked is 2) and passes with this change.
  • Manual: on web, open a page with a list of CachedNetworkImages in Firefox or Safari, push another route, pop back. Images stay visible with this change and go blank without it.

📝 Links to relevant issues/docs

Related: #949 replaces this completer with the framework one, which would also fix this. This PR is the minimal fix in the meantime.

🤔 Checklist before submitting

  • All projects build
  • Follows style guide lines (code style guide)
  • Relevant documentation was updated
  • Rebased onto current main

@shihabkandil

Copy link
Copy Markdown
Author

Can you please check this PR @rickdijk , i think it will solve the mentioned issue and this #1061 as well

@rickdijk rickdijk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more, outside the diff, at line 155 of multi_image_stream_completer.dart: if the last listener leaves while the first static decode is running, the decoded frame lands in _nextFrame and is dropped without being emitted. _framesEmitted stays 0, so the next addListener decodes again. On Firefox and Safari that is the blank-image path from the PR description, so navigating away mid-load and back can still blank the image. Calling setImage with the frame even when nobody is listening (it handles an empty listener list) would keep it for the next listener.

if (!hasListeners && _codec != null) _decodeNextFrameAndSchedule();
if (!hasListeners &&
_codec != null &&
(_framesEmitted == 0 || _codec!.frameCount > 1)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_framesEmitted is only reset in _switchToNewCodec, which runs on the animated path. When a static codec replaces another one through _handleCodecReady while nothing is listening, the counter keeps its old value, and this check then skips the decode.

A stale cache entry hits this. The cached file is decoded and shown (_framesEmitted is 1), the user pushes a route and Image drops its listener, and then getFileStream yields the refreshed file. _handleCodecReady sets _codec but doesn't decode because there are no listeners. On pop, this condition sees one emitted frame and frameCount == 1, so the fresh image is never decoded and the stale one stays on screen. On main the re-add would decode it.

Resetting _framesEmitted = 0 in _handleCodecReady fixes it, and makes the check mean "has the current codec produced an image yet", which is what Flutter's _currentImage == null stands for.

if (!hasListeners && _codec != null) _decodeNextFrameAndSchedule();
if (!hasListeners &&
_codec != null &&
(_framesEmitted == 0 || _codec!.frameCount > 1)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the comment above: _framesEmitted also drives the animation cycle math in _handleAppFrame, so using it as a stand-in for "we have an image" gives one counter two jobs. A field set in _emitFrame and cleared whenever _codec changes (say bool _currentCodecHasImage) would state the condition directly, and wouldn't break if another path swaps _codec later.

Comment thread cached_network_image/test/image_stream_completer_test.dart
@rickdijk

Copy link
Copy Markdown
Collaborator

Once all comments are resolved, I'd like to ask you to bump the version of the package and update CHANGELOG.md too.

@shihabkandil

Copy link
Copy Markdown
Author

Thank you for your clarification, I will work on resolving it asap and dig deep in how it handles the multi frame images

@shihabkandil

Copy link
Copy Markdown
Author

@rickdijk I have resolved the different states of listeners adding / removing and re-addition in static, animated decoding states along with tests covering the 5 cases, if anything is still needed, let me know :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants