Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix gif frame reuse #40125

Merged
merged 2 commits into from
Mar 8, 2023
Merged

Fix gif frame reuse #40125

merged 2 commits into from
Mar 8, 2023

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Mar 7, 2023

Adds a golden test that hopefully works cross platform 😇

Fixes flutter/flutter#122134

See also #31098

@dnfield
Copy link
Contributor Author

dnfield commented Mar 7, 2023

This reintroduces flutter/flutter#61150 (comment)

@@ -115,17 +115,17 @@ sk_sp<DlImage> MultiFrameCodec::State::GetNextFrameImage(
<< "Frame " << nextFrameIndex_ << " depends on frame "
<< requiredFrameIndex
<< " and no required frames are cached. Using blank slate instead.";
return nullptr;
} else if (lastRequiredFrameIndex_ != requiredFrameIndex) {
FML_DLOG(INFO) << "Required frame " << requiredFrameIndex
<< " is not cached. Using blank slate instead.";
Copy link
Member

Choose a reason for hiding this comment

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

So this path ends up getting hit for the problem GIF? It would seem like we're possibly drawing the wrong frame here, and we're just getting lucky that the frame still looks OK while rendering the wrong frame?

It's common for GIFs/WEBPs/APNGs to keep using one frame as the backdrop across the whole animation. That's not really possible with the way we've written multiframecodec today.

Not sure if we should fix it in this PR, but this comment should at least be updated since it's inaccurate with this change. Maybe just drop the "Using blank slate instead" since that doesn't always happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't get hit becaues of the else if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'mg oing to remove it entirely though.

@@ -115,17 +115,17 @@ sk_sp<DlImage> MultiFrameCodec::State::GetNextFrameImage(
<< "Frame " << nextFrameIndex_ << " depends on frame "
<< requiredFrameIndex
<< " and no required frames are cached. Using blank slate instead.";
return nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

It would be more graceful to just draw the frame against the zeroed bitmap in this case instead of failing the frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
// Copy the previous frame's output buffer into the current frame as the
// starting point.
if (lastRequiredFrame_->getPixels() &&
Copy link
Member

Choose a reason for hiding this comment

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

I think adding requiredFrameIndex != SkCodec::kNoFrame to the conditional would keep both fixes working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this a bit differently by making the condition below that updates the "keep" frame every time once we first encounter a keep frame.

I think this is fine because Skia's documentation on this parameter says that the last required frame is the earliest frame, and you can use any frame from that one to the last one. See https://github.com/google/skia/blob/7b18d6c5c676abcb274a8dc6b2896c8c2f667a63/include/codec/SkCodec.h#L640

@dnfield dnfield requested review from bdero and gaaclarke March 7, 2023 23:26
@dnfield
Copy link
Contributor Author

dnfield commented Mar 7, 2023

Added a regression test for the webp issue, and have a patch that now correctly handles both the funky gif and the funky webp.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Animated gifs that want to reuse frames are failing to do so
2 participants