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.

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.

3 participants