-
Notifications
You must be signed in to change notification settings - Fork 662
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
base: main
Are you sure you want to change the base?
Conversation
@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 😄 |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
coil-gif/README.md
Outdated
|
||
!!! 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). |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
@colinrtwhite Thank you for your feedback. I'll try to address it as much as I can 🙂 |
2b50e0b
to
a9aa4d9
Compare
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. |
drawImage( | ||
image = org.jetbrains.skia.Image.makeRaster( | ||
imageInfo = codec.imageInfo, | ||
bytes = frame, |
There was a problem hiding this comment.
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.
dadc9bb
to
02c3af0
Compare
This PR intends to bring animated GIF support to iOS and other non-Android platforms. (see #2347)
A new
AnimatedSkiaImageDecoder
has been integrated innonAndroidMain
, 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.
coil-gif
artifact has been converted to Kotlin MultiplatformdecodeUtils.kt
has been moved tocommonMain
, while the rest of the code stays inandroidMain
apiDump
has been runAnimatedTransformation
inAnimatedSkiaImage
and update docsAnimatedSkiaImage
iOS preview
ios.mp4
Desktop preview
Enregistrement.de.l.ecran.2024-10-26.a.17.11.45.mov