Closed rupinderjeet closed 3 years ago
Hey @rupinderjeet, @colinrtwhite I poked around with this issue (have a little bit of free time on my hands and decided to see if there are OS projects that might need help). From my research, it seems that this particular GIF has 0 delay time in-between frames @rupinderjeet. The reason it plays so differently on the web is that major browsers handle this case by replacing delay with a default value if it is lesser then cut-off. ImageDecoder plays it as received (really fast) and Movie just shows the first frame. When I replaced delay time with 15 it seems to be loading fine by Coil with both decoders. Now the question to the @colinrtwhite: I presume that pre-processing GIF or writing custom decoder, which allows replacing delay with minimal one, is way beyond the scope of that library, so what do we do with that issue?
@IljaKosynkin Thanks for investigating this! Do you know how tough it would be to pre-process the GIF as it's being read? I'd be open to a solution that uses ForwardingSource
to replace the GIF frame delay similar to web browsers depending the performance impact + how much code it would require.
@colinrtwhite I can't tell it from the top of my head regarding complexity and performance impact, to be honest, but I'm going to poke around to tell estimation, possibly with PR, if it is not too bad in my opinion.
Also, on a side note, from my investigation it seems that all files in gifs.json
are actually WEBP files, maybe I should add some actual GIFs here?
@IljaKosynkin Are you sure? They appear to be encoded as GIFs when I download them locally. Though, feel free to add more sample gifs to that file.
@colinrtwhite oh ok, seems that it might be an issue with browser. Because it defaults to WEBP for me apparently, but I can manually change link to get GIF version of the same animation. I wonder what version does Android receive tho, ImageDecoderDecoder checks for both GIF and WEBP, so either can get fetched. I would change all examples to hardcode extension, tbh, half for GIF half for WEBP.
As for the ForwardingSource, it seems that delay time can be found by searching for 21 F9 04 sequence in the byte buffer and changing uint16_t after first byte after that sequence. However, I'm not sure that it won't mess with the internal structure of the GIF, introducing potential artefacts in the image. On the other hand, that it is not likely and proper solution would require writing like good 30-40% of a GIF decoder, to make sure we're changing data in proper offsets. Thoughts?
@IljaKosynkin I don't think we can use animated WebP in the sample since those only work on API 28+.
Interesting! Do you know what 21 F9 04
represents in the GIF spec? Is it a magic number? I'd want to avoid anything that could introduce artifacts (unless that number is safe according to the GIF spec) and instead opt for a mini GIF parser.
@colinrtwhite It is not a magic number, well at least not sequence itself.
21 signals that extension will follow, F9 signals type of graphic control extension, 04 signals how many bytes are there before end of the block.
While probability of introducing artefacts is low, I believe it is still possible since image data is LZW compressed and there is no guarantee this sequence of bytes will not appear here, AFAIK.
We can extend the sequence to regex 00 21 F9 04 XX XX XX XX 00 which would lower chances of corrupting data further, but won't really make it impossible.
As for the decoder, I'm not sure writing it in Kotlin won't hit performance significantly. And writing native code might be not a best idea from the point of view of maintainability.
Or, alternatively, we can submit feature request to Android team to add minimal delay time for GIFs in ImageDecoder itself (since apparently it is not part of AOSP) and mark this issue as "won't fix".
So it's "pick your poison" kind of situation.
@IljaKosynkin Yep, we definitely don't want to add native code and I don't think requesting a feature for the next platform release would help much. In terms of performance I'm not too worried about adding some pre-processing of the input data using Kotlin code. Glide's GIF parser is 100% Java and it's pretty quick. As long as we're not allocating class instances I think we should be good.
Correct me if I'm wrong (not super familiar with the GIF technical specs), but maybe we could do something like this:
00
(the termination flag).Is that something you're interested in working on? Else I'm happy to take it on.
@colinrtwhite tbh I'm not sure how much overhead processing of binary files in pure Java/Kotlin would add compared to native languages, but I feel like it might be substantial. But we can try and profile code to see how bad it actually is. Anyway, regarding issue itself:
@IljaKosynkin Sounds good. I'll probably have time to try to prototype this this weekend.
Ok, so I guess I better pickup something else, right? @colinrtwhite
@IljaKosynkin Sure, if you could investigate + fix https://github.com/coil-kt/coil/issues/539 that would be awesome.
Sure, gonna take a look @colinrtwhite
Hi @colinrtwhite - Do you have any update or workaround on this issue? On some gifs I'm also noticing ImageDecoderDecoder
playing very fast and GifDecoder
just shows the first frame.
Edit: To add more context, I am migrating to Coil from Glide. On Glide, the gifs were all playing at the same speed. With Coil, I'm seeing this issues depending on the type of the decoder.
@seankim-android Ah yep this is a longstanding issue, unfortunately due to how Movie
handles a 0 time delay in between frames. I think your best bet to work around this is apply the fix mentioned here https://github.com/coil-kt/coil/issues/540#issuecomment-715489993. You can wrap a GifDecoder
and look for the 00 21 F9 04 XX XX XX XX 00
sequence to replace the frame delay with 15. I'll try and figure out same sample code for this this weekend.
Since this is taking a while to fix using a full GIF parser, it probably makes sense to include this work-around in GifDecoder
itself (disabled by default). cc @IljaKosynkin if you're still interested in implementing this.
@colinrtwhite Thank you very much for looking into this :pray:. I'm new to GIF decoders and having that sample code will be super helpful.
@colinrtwhite sorry for late response, got caught up in stuff. Yes, I'll do it, but can't promise any fast results. It is not exactly a one-line fix. Will check out Glide to see how they did it.
@IljaKosynkin I ended up taking a stab at it this weekend here. It uses the less cumbersome solution, but I haven't seen any artifacts using it yet.
@seankim-android If you want to use it asap in your project you can copy the FrameDelayRewritingSource
from the linked PR and create a decoder that wraps GifDecoder
/ImageDecoderDecoder
like so:
class DelegateGifDecoder(private val delegate: Decoder): Decoder by delegate {
override suspend fun decode(
pool: BitmapPool,
source: BufferedSource,
size: Size,
options: Options
) = delegate.decode(pool, FrameDelayRewritingSource(source).buffer(), size, options)
}
Though, keep in mind that it isn't fully tested and may have bugs!
@colinrtwhite Thank you very much for the PR and the workaround. I'm trying out the FrameDelayRewritingSource
with GifDecoder
and ImageDecoderDecoder
on different devices. The min frame delay rewrite seems to work very well so far!
@seankim-android Glad to hear. This fix will be included in the next release of the library in a few weeks (though disabled by default to let it incubate).
![Uploading Media_230914_004726.gif…]()
![Uploading 170828_095722_what-is-poweriq1-2.gif…]()
Plays really fast than how it is shown in Android Gallery and Chrome. Issue occurs while using
ImageDecoderDecoder
. If onlyGifDecoder
is used, gif doesn't play at all.How do I load this gif? Checkout #539
This is how it looks when I load it:
record.mp4.zip