Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make coil-gif multiplatform #2594

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

outadoc
Copy link

@outadoc outadoc commented Oct 26, 2024

This PR intends to bring animated GIF support to iOS and other non-Android platforms. (see #2347)

A new AnimatedSkiaImageDecoder has been integrated in nonAndroidMain, largely based on @colinrtwhite's POC, with added support for looping GIFs.

Since Skia natively supports animated WebP, this new decoder also supports them. I've validated that with a test image in the sample app, but I haven't committed it. I'm not sure if other formats (HEIF?) could be supported; I couldn't find proper images to test, so for now the decoder won't try to decode them.

  • The coil-gif artifact has been converted to Kotlin Multiplatform
  • decodeUtils.kt has been moved to commonMain, while the rest of the code stays in androidMain
  • apiDump has been run
  • Tests have been run
  • The documentation has been updated
  • The changes have been tested on Android, iOS and Desktop in the sample app
  • Buffer next frames in the background
  • Support AnimatedTransformation in AnimatedSkiaImage and update docs
  • Support extra options in AnimatedSkiaImage
  • Write new Skia tests
iOS preview
ios.mp4
Desktop preview
Enregistrement.de.l.ecran.2024-10-26.a.17.11.45.mov

@outadoc outadoc marked this pull request as ready for review October 26, 2024 15:05
@outadoc
Copy link
Author

outadoc commented Oct 26, 2024

@colinrtwhite Hi! Feel entirely free to make suggestions or even entirely take over the PR if you feel like it. As you can see, it's based on your POC, so it should be familiar. I'm a graphics newbie, so there's probably a lot to improve still -- but hopefully this helps the project 😄

Copy link
Member

@colinrtwhite colinrtwhite left a comment

Choose a reason for hiding this comment

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

Thanks! Added some comments that we'll need to tackle before merging. I can take those on after the Coil 3.1 release or feel free to take a stab at the changes if you'd like. We'll also need to add tests for this before we can merge it.


Optionally, you can manually add the decoder to your component registry when constructing your `ImageLoader`:

```kotlin
// For Android
val imageLoader = ImageLoader.Builder(context)
Copy link
Member

@colinrtwhite colinrtwhite Oct 26, 2024

Choose a reason for hiding this comment

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

We should introduce a common function like AnimatedImageDecoderFactory (not sure about the naming) that automatically returns the right decoder factory implementation for the platform/API level. That way we could recommend:

val imageLoader = ImageLoader.Builder(context)
    .components {
        add(AnimatedImageDecoderFactory())
    }
    .build()

in the readme.


!!! Note
Coil includes two separate decoders to support decoding GIFs. `GifDecoder` supports all API levels, but is slower. `ImageDecoderDecoder` is powered by Android's [ImageDecoder](https://developer.android.com/reference/android/graphics/ImageDecoder) API which is only available on API 28 and above. `ImageDecoderDecoder` is faster than `GifDecoder` and supports decoding animated WebP images and animated HEIF image sequences.
On Android, to transform the pixel data of each frame of a GIF, see [AnimatedTransformation](/coil/api/coil-gif/coil3.gif/-animated-transformation).
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 we can support this on all platforms by replacing android.graphics.Canvas with coil3.Canvas in AnimatedTransformation.

* fully buffered, but will also play more smoothly.
*/
class AnimatedSkiaImageDecoder(
private val source: ImageSource,
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to pass through Options so we can add support for all the properties in coil3/gif/imageRequest.kt. I don't think we should ship this without adding support for those properties first.

*/
class AnimatedSkiaImageDecoder(
private val source: ImageSource,
private val prerenderFrames: Boolean = true,
Copy link
Member

@colinrtwhite colinrtwhite Oct 26, 2024

Choose a reason for hiding this comment

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

I think we'll need to tweak this to something like bufferedFramesCount (pass 0 for disabled, Int.MAX_VALUE to buffer everything) since we'll need to update the implementation to buffer a few frames up front then buffer the rest just in time before we need to draw them.

We'll want to buffer X frames during the decode process then figure out how to get a CoroutineScope so we can decode the rest right after AnimatedSkiaImage.draw is called.

@outadoc
Copy link
Author

outadoc commented Oct 31, 2024

@colinrtwhite Thank you for your feedback. I'll try to address it as much as I can 🙂

@outadoc outadoc force-pushed the feat/coil-gif-multiplatform branch 3 times, most recently from 2b50e0b to a9aa4d9 Compare November 4, 2024 22:43
@outadoc outadoc marked this pull request as draft November 5, 2024 21:01
@outadoc
Copy link
Author

outadoc commented Nov 5, 2024

I've written JVM tests and rewritten some of the code to fix issues. I'm pretty satisfied with how the PR is turning out, even if we could still add more tests, I'm sure.

@outadoc outadoc marked this pull request as ready for review November 5, 2024 22:46
drawImage(
image = org.jetbrains.skia.Image.makeRaster(
imageInfo = codec.imageInfo,
bytes = frame,
Copy link
Contributor

@revonateB0T revonateB0T Nov 8, 2024

Choose a reason for hiding this comment

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

Is there any possibility to use natively allocated pixels buffer?
Although desktop jvm has larger heap size, I don't think store pixels in jvm heap is a good idea.
Moreover, buffer size is not changed, we could just use two or three buffer to do something like decode-render-swap, just like vulkan swapchain, this help us avoid allocating extra memory.

Copy link
Member

Choose a reason for hiding this comment

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

@revonateB0T Agreed, but let's tackle this as a follow up.

For buffer size we need to consume/buffer the ImageSource entirely inside Decode.decode. This is to prevent leaking underlying references (mostly to Coil's disk cache). Would that still work with decode-render-swap?

Copy link
Contributor

Choose a reason for hiding this comment

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

For buffer size we need to consume/buffer the ImageSource entirely inside Decode.decode. This is to prevent leaking underlying references (mostly to Coil's disk cache).

That's not optimal, ImageDecoder will not comsume source fully as well for animated image, it requires the source is seekable instead.

And that's why it requires a ByteArray/File/ByteBuffer, they are both seekable & rewindable, ImageDecoder takes ownership(for fd, it calls dup) and requires source must not be modified during AnimatedImageDrawable's lifecycle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Load frames totally to memory at once is terrible, we can easily expect a OOM occurs(If we use jvm heap) or operating system not response(If we use native heap) or process get killed for some webp/gif have long durations & frames.
That's extremely terrible idea...

Copy link
Member

Choose a reason for hiding this comment

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

@revonateB0T Ah I see what you mean. I was talking about copying the undecoded data in Decode.decode so we can read it outside of that scope (similar to what we do in AnimatedImageDecoder. How do we solve this in a common way across platforms?

Copy link
Contributor

@revonateB0T revonateB0T Nov 29, 2024

Choose a reason for hiding this comment

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

@revonateB0T Ah I see what you mean. I was talking about copying the undecoded data in Decode.decode so we can read it outside of that scope (similar to what we do in AnimatedImageDecoder. How do we solve this in a common way across platforms?

Then yes, decode-render-swap works, but it's still not optimal to buffer whole source in memory if source is too large, we still need a file-backed source to reduce memory pressure on low end machine.

The problem is, dup a fd on Linux then close DiskCache Editor is fine, we can read the content of file even diskcache entry is deleted(See https://man7.org/linux/man-pages/man2/unlink.2.html)

If the name was the last link to a file but any processes still
have the file open, the file will remain in existence until the
last file descriptor referring to it is closed.

But things is different on Windows, we are not allowed to delete a file if it's opened?

Copy link
Member

Choose a reason for hiding this comment

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

@revonateB0T We can't use a file backed source as we don't know when the animated GIF is no longer used and we can't expect users to manually call a close method.

Copy link
Contributor

Choose a reason for hiding this comment

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

@revonateB0T We can't use a file backed source as we don't know when the animated GIF is no longer used and we can't expect users to manually call a close method.

On Android, we use file source if maybeRewriteGifSource is disabled, the file is in diskcache. When limit exceeded, file will be automatically deleted w/o affect decoder, but it only works on Linux platform.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, we can't rely on that behavior on all supported platforms. Also Skiko doesn't support streaming - all bytes need to be in memory.

@outadoc outadoc force-pushed the feat/coil-gif-multiplatform branch from dadc9bb to 02c3af0 Compare November 8, 2024 17:58
public static final fun getRepeatCount (Lcoil3/Extras$Key$Companion;)Lcoil3/Extras$Key;
public static final fun getRepeatCount (Lcoil3/request/ImageRequest;)I
public static final fun getRepeatCount (Lcoil3/request/Options;)I
public static final fun onAnimationEnd (Lcoil3/request/ImageRequest$Builder;Lkotlin/jvm/functions/Function0;)Lcoil3/request/ImageRequest$Builder;
Copy link
Member

@colinrtwhite colinrtwhite Nov 28, 2024

Choose a reason for hiding this comment

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

@outadoc Removing these functions breaks binary compatibility. We'll have to keep them and mark them as hidden and delegate to the non-hidden versions similar to here. When users update Kotlin will automatically compile against the non-hidden functions.

@@ -57,7 +28,7 @@ val Extras.Key.Companion.animatedTransformation: Extras.Key<AnimatedTransformati
private val animatedTransformationKey = Extras.Key<AnimatedTransformation?>(default = null)

/**
* Set the callback to be invoked at the start of the animation if the result is an animated [Drawable].
* Set the callback to be invoked at the start of the animation if the result is an animated drawable.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Set the callback to be invoked at the start of the animation if the result is an animated drawable.
* Set the callback to be invoked at the start of the animation if the result is an animated image.

@@ -75,7 +46,7 @@ val Extras.Key.Companion.animationStartCallback: Extras.Key<(() -> Unit)?>
private val animationStartCallbackKey = Extras.Key<(() -> Unit)?>(default = null)

/**
* Set the callback to be invoked at the end of the animation if the result is an animated [Drawable].
* Set the callback to be invoked at the end of the animation if the result is an animated drawable.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Set the callback to be invoked at the end of the animation if the result is an animated drawable.
* Set the callback to be invoked at the end of the animation if the result is an animated image.

}
}

private const val DEFAULT_BUFFERED_FRAMES_COUNT = 5
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this constant to AnimatedSkiaImageDecoder.Factory.Companion and make it public

}

class Factory(
private val bufferedFramesCount: Int = DEFAULT_BUFFERED_FRAMES_COUNT,
Copy link
Member

Choose a reason for hiding this comment

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

Can we assert bufferedFramesCount is >= 0 (or maybe >= 1)?

abstract val decoderFactory: Decoder.Factory

@Test
fun eachFrameIsDisplayedCorrectlyWithExpectedTimingForOneFullIteration() = runTest {
Copy link
Member

Choose a reason for hiding this comment

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

This is awesome! Thanks for adding such high quality tests.

internal class AnimatedSkiaImage(
private val codec: Codec,
private val coroutineScope: CoroutineScope,
private val timeSource: TimeSource,
Copy link
Member

Choose a reason for hiding this comment

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

Great idea to pass a time source here. Copied it CrossfadePainter as well.

}
named("nonAndroidMain") {
dependencies {
implementation(projects.coilComposeCore)
Copy link
Member

Choose a reason for hiding this comment

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

Can we import only compose.runtime here?


// Buffer the next frames in the background.
if (bufferFramesJob == null || bufferFramesJob?.isCancelled == true) {
bufferFramesJob = coroutineScope.launch(Dispatchers.Default) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add an option for decoderCoroutineContext to this class' constructor. We can pass the value from options.decoderCoroutineContext from our Decoder.

private fun Canvas.drawFrame(frameIndex: Int) {
animatedTransformation?.transform(this)

frames[frameIndex]?.let { frame ->
Copy link
Member

@colinrtwhite colinrtwhite Nov 28, 2024

Choose a reason for hiding this comment

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

We should probably block if the current frame we're trying to draw isn't available yet. We don't need to wait for the whole decode job to complete, however. We can use runBlocking on non-JS, but might need to spin in a while loop on JS to block.

Skipping drawing the current frame if it's not available yet could make it appear like the GIF is disappearing randomly on slower devices. It'll be up to the library user to set bufferedFramesCount high enough that we don't end up blocking.

Coil includes two separate decoders to support decoding GIFs. `GifDecoder` supports all API levels, but is slower. `ImageDecoderDecoder` is powered by Android's [ImageDecoder](https://developer.android.com/reference/android/graphics/ImageDecoder) API which is only available on API 28 and above. `ImageDecoderDecoder` is faster than `GifDecoder` and supports decoding animated WebP images and animated HEIF image sequences.
| Decoder | Supported Platforms | Supported Formats | Notes |
|----------------------------|---------------------|-------------------|-----------------------------------------------------------------------------------------------------------------|
| `AnimatedImageDecoder` | Android (API 28+) | GIF, WebP, HEIF | Powered by Android's [ImageDecoder](https://developer.android.com/reference/android/graphics/ImageDecoder) API. |
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for documenting this 🙏🏻

@colinrtwhite
Copy link
Member

colinrtwhite commented Nov 28, 2024

@outadoc This excellent! The implementation looks great. Left a few more comment + we'll need to rebase. Afterwards we should be good to merge!

@outadoc
Copy link
Author

outadoc commented Jan 2, 2025

Thank you for the feedback Colin, sorry I haven't been able to work on this for a while due to some personal stuff. I'll try to make progress on it soon-ish, but if someone wants to make updates in the mean time, feel free.

@masterQian
Copy link

masterQian commented Jan 3, 2025

Because I was in a hurry, I used your code, which can compile and run normally. However, I found that testing on Desktop (Windows/JVM) showed that WEBP animation consumes a lot of CPU and memory. I am a beginner and cannot solve it, but through Profile monitoring, I saw that the program only used 200M of memory but applied for 5-8G of memory. I don't think this is a memory leak, but I don't know what the reason is. Thank you!

version: coil3-3.0.4 + coil-gif-multiplatform

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.

4 participants