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
dnfield merged 2 commits into from
Mar 8, 2023
Merged

Fix gif frame reuse #40125

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7520088
Fix gif frame reuse
dnfield Mar 7, 2023
0e17897
Fixes
dnfield Mar 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added lib/ui/fixtures/four_frame_with_reuse.gif
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
26 changes: 13 additions & 13 deletions lib/ui/painting/multi_frame_codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@bdero bdero Mar 7, 2023

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

@dnfield dnfield Mar 7, 2023

Choose a reason for hiding this comment

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

Done

} else if (lastRequiredFrameIndex_ != requiredFrameIndex) {
FML_DLOG(INFO) << "Required frame " << requiredFrameIndex
<< " is not cached. Using blank slate instead.";
Copy link
Member

@bdero bdero Mar 7, 2023

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

@dnfield dnfield Mar 7, 2023

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

@dnfield dnfield Mar 7, 2023

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.

} else {
// Copy the previous frame's output buffer into the current frame as the
// starting point.
if (lastRequiredFrame_->getPixels() &&
CopyToBitmap(&bitmap, lastRequiredFrame_->colorType(),
*lastRequiredFrame_)) {
prior_frame_index = requiredFrameIndex;
}
}
// Copy the previous frame's output buffer into the current frame as the
// starting point.
if (lastRequiredFrame_->getPixels() &&
Copy link
Member

@bdero bdero Mar 7, 2023

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

@dnfield dnfield Mar 7, 2023

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

CopyToBitmap(&bitmap, lastRequiredFrame_->colorType(),
*lastRequiredFrame_)) {
prior_frame_index = requiredFrameIndex;
}
}

Expand Down Expand Up @@ -161,9 +161,9 @@ sk_sp<DlImage> MultiFrameCodec::State::GetNextFrameImage(
gpu_disable_sync_switch->Execute(
fml::SyncSwitch::Handlers()
.SetIfTrue([&skImage, &bitmap] {
// Defer decoding until time of draw later on the raster thread. Can
// happen when GL operations are currently forbidden such as in the
// background on iOS.
// Defer decoding until time of draw later on the raster thread.
// Can happen when GL operations are currently forbidden such as
// in the background on iOS.
skImage = SkImage::MakeFromBitmap(bitmap);
})
.SetIfFalse([&skImage, &resourceContext, &bitmap] {
Expand Down Expand Up @@ -257,8 +257,8 @@ Dart_Handle MultiFrameCodec::getNextFrame(Dart_Handle callback_handle) {
}));

return Dart_Null();
// The static leak checker gets confused by the control flow, unique pointers
// and closures in this function.
// The static leak checker gets confused by the control flow, unique
// pointers and closures in this function.
// NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
}

Expand Down
Loading