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

A gif is playing very fast #540

Closed
rupinderjeet opened this issue Sep 24, 2020 · 25 comments · Fixed by #783
Closed

A gif is playing very fast #540

rupinderjeet opened this issue Sep 24, 2020 · 25 comments · Fixed by #783
Labels
bug Something isn't working help wanted Issues that are up for grabs + are good candidates for community PRs

Comments

@rupinderjeet
Copy link

rupinderjeet commented Sep 24, 2020

tumblr_ku2pvuJkJG1qz9qooo1_r1_400

Plays really fast than how it is shown in Android Gallery and Chrome. Issue occurs while using ImageDecoderDecoder. If only GifDecoder 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

@rupinderjeet rupinderjeet added the bug Something isn't working label Sep 24, 2020
@colinrtwhite colinrtwhite added the help wanted Issues that are up for grabs + are good candidates for community PRs label Sep 26, 2020
@IljaKosynkin
Copy link
Contributor

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.
ezgif-6-53c89e37efaa
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?

@colinrtwhite
Copy link
Member

@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.

@IljaKosynkin
Copy link
Contributor

@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.

@IljaKosynkin
Copy link
Contributor

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?

@colinrtwhite
Copy link
Member

@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.

@IljaKosynkin
Copy link
Contributor

IljaKosynkin commented Oct 21, 2020

@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?

@colinrtwhite
Copy link
Member

@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.

@IljaKosynkin
Copy link
Contributor

@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.

@colinrtwhite
Copy link
Member

@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:

  1. Skip the Header (6 bytes)
  2. Skip the Logical Screen Descriptor (7 bytes)
  3. Skip the Global Color Table (3*2^(N+1) bytes where N is the number given by the last 3 bytes of the Logical Screen Descriptor)
  4. Modify the delay time in the Graphics Control Extension (if present).
  5. Skip the Image Descriptor (10 bytes)
  6. Skip the Local Color Table (same way we skipped the Global Color Table)
  7. Skip through the image data. It gives us repeating sizes to skip through for each image block.
  8. Handle skipping the Application and Plain Text and extensions (both list their size at the beginning of the block)
  9. Go back to step 4 if we aren't at 00 (the termination flag).

Is that something you're interested in working on? Else I'm happy to take it on.

@IljaKosynkin
Copy link
Contributor

@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:

  • Yes, that exactly what I meant by 40% of decoder. Because you're still parsing pretty much everything, just skipping most of the data.
  • Who picks the issue largely depends on how much priority it has IMHO. I don't have great familiarity with either Coil internal structure or GIF decoder implementation, so if this issue is urgent you're probably better person to pick it up. Otherwise I can implement it, it just will take some while.

@colinrtwhite
Copy link
Member

@IljaKosynkin Sounds good. I'll probably have time to try to prototype this this weekend.

@IljaKosynkin
Copy link
Contributor

Ok, so I guess I better pickup something else, right? @colinrtwhite

@colinrtwhite
Copy link
Member

@IljaKosynkin Sure, if you could investigate + fix #539 that would be awesome.

@IljaKosynkin
Copy link
Contributor

Sure, gonna take a look @colinrtwhite

@seankim-android
Copy link

seankim-android commented Jun 16, 2021

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.

@colinrtwhite
Copy link
Member

@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 #540 (comment). 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.

@seankim-android
Copy link

@colinrtwhite Thank you very much for looking into this 🙏. I'm new to GIF decoders and having that sample code will be super helpful.

@IljaKosynkin
Copy link
Contributor

@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.

@colinrtwhite
Copy link
Member

colinrtwhite commented Jun 21, 2021

@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!

@seankim-android
Copy link

seankim-android commented Jun 21, 2021

@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!

@colinrtwhite
Copy link
Member

colinrtwhite commented Jun 22, 2021

@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).

@Guaxi1227
Copy link

Uploading Media_230914_004726.gif…

@Guaxi1227
Copy link

@Guaxi1227
Copy link

Media_230914_004726

@starryintegrated
Copy link

Uploading 170828_095722_what-is-poweriq1-2.gif…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Issues that are up for grabs + are good candidates for community PRs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@colinrtwhite @IljaKosynkin @rupinderjeet @seankim-android @Guaxi1227 @starryintegrated and others