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

Add constant to parse Gif Loop Count and apply to repeatCount #2654

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

Conversation

tgyuuAn
Copy link

@tgyuuAn tgyuuAn commented Nov 7, 2024

Key Changes

  • Loop Count Handling: The loop count is a feature present exclusively in the GIF format among the three formats being considered (GIF, WebP, HEIF). This distinction is important because it affects how animations are processed and repeated. Among these formats, only GIF includes the concept of a loop count.

You can use this feature in your code as follows:

@Preview
@Composable
fun Test() {
    val customImageLoader = ImageLoader.Builder(LocalContext.current)
        .components {
            if (Build.VERSION.SDK_INT >= 28) {
                add(AnimatedImageDecoder.Factory())
            } else {
                add(GifDecoder.Factory())
            }
        }
        .build()

    val imageRequest = ImageRequest.Builder(LocalContext.current)
        .data(sample.common.R.drawable.ic_change_loop_count)
        .repeatCount(GIF_LOOP_COUNT)  // <---- This Constant
        .build()

    AsyncImage(
        imageLoader = customImageLoader,
        model = imageRequest,
        contentDescription = null,
        modifier = Modifier.size(100.dp),
    )
}



Parsing Logic for GIF

During the GIF parsing process, we utilized source.peek() to read the data again. This was necessary because the loop count needs to be accessed without consuming the original BufferedSource, ensuring that we can still read the data needed for further processing. Unfortunately, this reliance on peek() is a limitation and feels like a workaround in the parsing logic.

Handling Loop Count in AnimatedImageDecoder

In the AnimatedImageDecoder, the logic that applies the loop count is separate from the logic that consumes the ImageSource. Because of this separation, the wrapDrawable function now includes an additional parameter, loopCount. This design choice was unavoidable, as adding a setter to Options.repeatCount would not have sufficed for properly applying the loop count without compromising the integrity of the original image data.

If you have a better opinion, please tell me, and I will reflect it.


GIF Format Overview

The Graphics Interchange Format (GIF) consists of a structured file format that is primarily used for graphics and animations. Here’s a breakdown of its structure and the significance of the Application Extension block, particularly regarding the parsing of the loop count.

GIF Structure in Bytes

  1. Header (6 bytes):

    • The GIF file begins with a header that indicates the format version:
      • GIF87a (6 bytes)
      • GIF89a (6 bytes) <--- Loop Count exists only in this format.
  2. Logical Screen Descriptor (7 bytes):

    • This section provides information about the logical screen, including dimensions and color table flags.
  3. Global Color Table (if present):

    • This table can contain up to 256 colors and is 3 bytes per color, resulting in a maximum size of 765 bytes (256 colors).
  4. Image Descriptor (10 bytes):

    • Each image within the GIF is described by an Image Descriptor that includes its position and size on the logical screen.
  5. Image Data:

    • This section contains the actual pixel data for the image, which may also include local color tables if specified.
  6. Extension Blocks:

    • GIF files can include several extension blocks, which provide additional functionality.

Application Extension Block

The Application Extension Block is crucial for animated GIFs as it defines the animation properties, including the loop count. The structure of the Application Extension Block is as follows:

  • Block Introducer (1 byte): Always 0x21 to indicate an extension block.
  • Label (1 byte): Indicates the type of extension, 0xFF for application extension.
  • Block Size (1 byte): Indicates the size of the application identifier (usually 0x0B for 11 bytes).
  • Application Identifier (8 bytes): Typically "NETSCAPE".
  • Application Authentication Code (3 bytes): Usually "2.0".
  • Data Block Size (1 byte): Indicates the size of the data block that follows (typically 0x03 for three bytes).
  • Loop Count Indicator (1 byte): Always 0x01, indicating the presence of loop count data.
  • Loop Count (2 bytes): The actual loop count, specified in little-endian format. A value of 0 indicates infinite looping.
  • Block Terminator (1 byte): Always 0x00 to indicate the end of the extension.

In our parsing logic, we focus on the Application Extension to extract the loop count, which is critical for controlling the animation behavior of the GIF.

internal fun extractLoopCountFromGif(source: BufferedSource): Int? {
    return try {
        val byteArray = source.use { it.readByteArray() } // Copy data
        val bufferedSource = byteArray.inputStream().source().buffer() // Create BufferedSource from the copy
        val loopCount = parseLoopCount(bufferedSource)
        loopCount
    } catch (e: Exception) {
        null
    }
}

private fun parseLoopCount(source: BufferedSource): Int? {
    // Read GIF header
    val headerBytes = ByteArray(6)
    if (source.read(headerBytes) != 6) return null

    // Read Logical Screen Descriptor
    val screenDescriptorBytes = ByteArray(7)
    if (source.read(screenDescriptorBytes) != 7) return null

    // Skip Global Color Table
    if ((screenDescriptorBytes[4] and 0b10000000.toByte()) != 0.toByte()) {
        val colorTableSize = 3 * (1 shl ((screenDescriptorBytes[4].toInt() and 0b00000111) + 1)) // Calculate color table size
        source.skip(colorTableSize.toLong()) // Skip Global Color Table
    }

    // Handle Application Extension Block
    while (!source.exhausted()) {
        val blockType = source.readByte().toInt() and 0xFF
        if (blockType == 0x21) { // Extension Introducer
            val label = source.readByte().toInt() and 0xFF
            if (label == 0xFF) { // Application Extension
                val blockSize = source.readByte().toInt() and 0xFF
                val appIdentifier = source.readUtf8(8)
                val appAuthCode = source.readUtf8(3)

                // Check for NETSCAPE block and parse loop count
                if (appIdentifier == "NETSCAPE" && appAuthCode == "2.0") {

                    // Read data block size
                    val dataBlockSize = source.readByte().toInt() and 0xFF
                    // Read loop count indicator byte
                    val loopCountIndicator = source.readByte().toInt() and 0xFF
                    // Read low and high bytes for loop count
                    val loopCountLow = source.readByte().toInt() and 0xFF // Read low byte
                    val loopCountHigh = source.readByte().toInt() and 0xFF // Read high byte
                    val loopCount = (loopCountHigh shl 8) or loopCountLow // Combine high and low bytes

                    // Read block terminator byte
                    val blockTerminator = source.readByte().toInt() and 0xFF
                    return if (loopCount == 0) Int.MAX_VALUE else loopCount // 0 means infinite loop
                } else {
                    skipExtensionBlock(source) // Skip if not NETSCAPE
                }
            } else {
                skipExtensionBlock(source) // Skip other extension blocks
            }
        }
    }
    return null
}

// Function to skip Extension Blocks
// Extension Blocks always terminate with a zero
private fun skipExtensionBlock(source: BufferedSource) {
    while (true) {
        val size = source.readByte().toInt() and 0xFF
        if (size == 0) break
        source.skip(size.toLong())
    }
}

For further reading, check out:


If I need to write test code, please let me know where I can write it.

@colinrtwhite

@@ -54,10 +56,22 @@ class AnimatedImageDecoder(

override suspend fun decode(): DecodeResult {
var isSampled = false

val loopCount = if (options.repeatCount == GIF_LOOP_COUNT) {
extractLoopCountFromGif(source.source().peek()) ?: REPEAT_INFINITE
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.

You mean we need to skip over all image data for application extension block to parse loop count?
You use a peek buffer so almost all file content will be read into okio Buffer(art heap) (i.e. Buffer size will enlarged to file length, we cannot reuse Segment).
This may cause OOM and huge performance regression, we need to warn user that trade-off.

Copy link
Author

Choose a reason for hiding this comment

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

@revonateB0T

Ideally, it would be best to parse the Loop Count directly during the initial parsing of the GIF. However, currently, I have concluded that we need to use either peek() or clone() of BufferedSource.

Between these two methods, would it be better to use clone() to allow for garbage collection of the memory, rather than using peek()?

Or is the approach itself flawed?

I am quite concerned that the current implementation may not be the most efficient in terms of performance and memory management. I would appreciate any appropriate advice you could provide.

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.

You cannot clone a BufferedSource to another BufferedSource, as Source is one-shot.
It's hard to optimize it currently if we consist on that BufferedSource abstraction.
We need to break that, consider a ByteBuffer source which is seekable & rewindable, a File is seekable & rewindable, so I suggest you warn user we may OOM, let they know the trade-off.

Copy link
Author

@tgyuuAn tgyuuAn Nov 8, 2024

Choose a reason for hiding this comment

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

Thank you so much for your advice. Let's think more about this !!

@@ -34,6 +34,13 @@ class GifDecoder(
) : Decoder {

override suspend fun decode() = runInterruptible {
val loopCount = if (options.repeatCount == MovieDrawable.GIF_LOOP_COUNT) {
source.source().peek().let { extractLoopCountFromGif(it) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if the source is gif before we peek it.

fun DecodeUtils.isGif(source: BufferedSource): Boolean {

Copy link
Author

Choose a reason for hiding this comment

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

I appreciate your insight!

@colinrtwhite
Copy link
Member

@tgyuuAn Thank you for taking the time to write this! That said, I think this can be implemented without a custom GIF parser. The AnimatedImageDrawable docs mention that by default the repeat count in the encoded data is respected. As such I think all we need to do to support using the encoded repeat count is to not call animatedDrawable.repeatCount = if options.repeatCount == GIF_LOOP_COUNT. Also I would rename GIF_LOOP_COUNT to ENCODED_REPEAT_COUNT to indicate it's encoded in the data.

For GifDecoder I don't think we can support repeatCount without reading the data, but we can just mark ENCODED_REPEAT_COUNT as API >= 28. I'd prefer to avoid adding custom GIF parsing code since it would only be used on older devices and it could be tough to maintain long term.

@tgyuuAn
Copy link
Author

tgyuuAn commented Nov 8, 2024

@tgyuuAn Thank you for taking the time to write this! That said, I think this can be implemented without a custom GIF parser. The AnimatedImageDrawable docs mention that by default the repeat count in the encoded data is respected. As such I think all we need to do to support using the encoded repeat count is to not call animatedDrawable.repeatCount = if options.repeatCount == GIF_LOOP_COUNT. Also I would rename GIF_LOOP_COUNT to ENCODED_REPEAT_COUNT to indicate it's encoded in the data.

For GifDecoder I don't think we can support repeatCount without reading the data, but we can just mark ENCODED_REPEAT_COUNT as API >= 28. I'd prefer to avoid adding custom GIF parsing code since it would only be used on older devices and it could be tough to maintain long term.

I feel a weight lifted off my shoulders !!!!!!!

@tgyuuAn tgyuuAn force-pushed the gifAutoLoopCount branch 2 times, most recently from ce13939 to ce1c822 Compare November 8, 2024 08:13
@tgyuuAn
Copy link
Author

tgyuuAn commented Nov 8, 2024

@colinrtwhite

If it is over 28, there is an exception that causes infinite repetition if you use GifDecoder.

Comment on lines 21 to 28
fun ImageRequest.Builder.repeatCount(repeatCount: Int) = apply {
require(repeatCount >= REPEAT_INFINITE) { "Invalid repeatCount: $repeatCount" }
if (SDK_INT >= 28) {
require(repeatCount >= ENCODED_LOOP_COUNT) { "Invalid repeatCount: $repeatCount" }
} else {
require(repeatCount >= REPEAT_INFINITE) { "Invalid repeatCount: $repeatCount" }
}
extras[repeatCountKey] = repeatCount
}
Copy link
Author

Choose a reason for hiding this comment

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

In this part, the lint was broken because the version branch for ENCODED_LOOP_COUNT was not given, but this was fixed.

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.

You'll have to run ./gradlew apiDump to update the latest API files since the change updates the public API.

@@ -151,7 +153,11 @@ class MovieDrawable @JvmOverloads constructor(
* Default: [REPEAT_INFINITE]
*/
fun setRepeatCount(repeatCount: Int) {
require(repeatCount >= REPEAT_INFINITE) { "Invalid repeatCount: $repeatCount" }
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 want to leave this check as is. ENCODED_LOOP_COUNT shouldn't be passed to MovieDrawable.setRepeatCount.

@@ -122,7 +124,9 @@ class AnimatedImageDecoder(
return baseDrawable
}

baseDrawable.repeatCount = options.repeatCount
if (options.repeatCount != ENCODED_LOOP_COUNT) {
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 add handling to GifDecoder.Factory to ensure we don't set the repeat count there either when options.repeatCount == ENCODED_LOOP_COUNT.

Copy link
Author

Choose a reason for hiding this comment

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

We'll need to add handling to GifDecoder.Factory to ensure we don't set the repeat count there either when options.repeatCount == ENCODED_LOOP_COUNT.

I'm trying to add check() to the Factory as shown below. What do you think about this?

class Factory(
    val enforceMinimumFrameDelay: Boolean = true,
) : Decoder.Factory {

    override fun create(
        result: SourceFetchResult,
        options: Options,
        imageLoader: ImageLoader,
    ): Decoder? {
        if (Build.VERSION.SDK_INT >= 28) {
            check(options.repeatCount == ENCODED_LOOP_COUNT) { 
                "ENCODED_LOOP_COUNT cannot be used with GifDecoder. Please use the AnimatedImageDecoder if you need to use the encoded loop count."
            }
        }

        if (!DecodeUtils.isGif(result.source.source())) return null
        return GifDecoder(result.source, options, enforceMinimumFrameDelay)
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

We only want to guard against it here and ignore the parameter instead of throwing: https://github.com/coil-kt/coil/blob/main/coil-gif/src/main/java/coil3/gif/GifDecoder.kt#L52

* This only applies when using [AnimatedImageDecoder].
*/
@RequiresApi(28)
const val ENCODED_LOOP_COUNT = -2
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 should move this constant to AnimatedImageDecoder.Companion.ENCODED_LOOP_COUNT because it's only supported there and can't be used by MovieDrawable.

Copy link
Author

@tgyuuAn tgyuuAn Nov 11, 2024

Choose a reason for hiding this comment

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

Thx, that's a really good idea.

@colinrtwhite
Copy link
Member

@tgyuuAn You need to run ./gradlew spotlessApply to fix the style check.

* Pass this to [ImageRequest.Builder.repeatCount] to repeat according to encoded LoopCount metadata.
* This only applies when using [AnimatedImageDecoder].
*/
@RequiresApi(28)
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this annotation as AnimatedImageDecoder already has it.

companion object {
/**
* Pass this to [ImageRequest.Builder.repeatCount] to repeat according to encoded LoopCount metadata.
* This only applies when using [AnimatedImageDecoder].
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
* This only applies when using [AnimatedImageDecoder].

Copy link
Author

Choose a reason for hiding this comment

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

I updated it !

@colinrtwhite
Copy link
Member

@tgyuuAn Thanks for implementing this - no more changes needed! I'll merge this once we're done with 3.0.x bug fix releases and include it in the 3.1.0 release.

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