-
Notifications
You must be signed in to change notification settings - Fork 665
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
base: main
Are you sure you want to change the base?
Conversation
3f37e59
to
bbfd45b
Compare
@@ -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 |
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.
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.
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.
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.
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.
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.
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.
Thank you so much for your advice. Let's think more about this !!
bbfd45b
to
9b8b120
Compare
@@ -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) } |
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.
Check if the source is gif before we peek it.
fun DecodeUtils.isGif(source: BufferedSource): Boolean { |
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 appreciate your insight!
@tgyuuAn Thank you for taking the time to write this! That said, I think this can be implemented without a custom GIF parser. The For |
I feel a weight lifted off my shoulders !!!!!!! |
ce13939
to
ce1c822
Compare
If it is over 28, there is an exception that causes infinite repetition if you use GifDecoder. |
ce1c822
to
836c5fa
Compare
…ly on SDK 28 or higher
836c5fa
to
5b302b6
Compare
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 | ||
} |
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.
In this part, the lint was broken because the version branch for ENCODED_LOOP_COUNT
was not given, but this was fixed.
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.
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" } |
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 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) { |
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 add handling to GifDecoder.Factory
to ensure we don't set the repeat count there either when options.repeatCount == ENCODED_LOOP_COUNT
.
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 add handling to
GifDecoder.Factory
to ensure we don't set the repeat count there either whenoptions.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)
}
}
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 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 |
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 should move this constant to AnimatedImageDecoder.Companion.ENCODED_LOOP_COUNT
because it's only supported there and can't be used by MovieDrawable
.
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.
Thx, that's a really good idea.
… using GifDecoder
7062567
to
6ed462e
Compare
@tgyuuAn You need to run |
* Pass this to [ImageRequest.Builder.repeatCount] to repeat according to encoded LoopCount metadata. | ||
* This only applies when using [AnimatedImageDecoder]. | ||
*/ | ||
@RequiresApi(28) |
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 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]. |
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.
* This only applies when using [AnimatedImageDecoder]. |
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 updated it !
f70ac50
to
d2ee980
Compare
d2ee980
to
769d0b4
Compare
@tgyuuAn Thanks for implementing this - no more changes needed! I'll merge this once we're done with |
Key Changes
You can use this feature in your code as follows:
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 originalBufferedSource
, ensuring that we can still read the data needed for further processing. Unfortunately, this reliance onpeek()
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 theImageSource
. Because of this separation, thewrapDrawable
function now includes an additional parameter,loopCount
. This design choice was unavoidable, as adding a setter toOptions.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
Header (6 bytes):
GIF87a
(6 bytes)GIF89a
(6 bytes) <--- Loop Count exists only in this format.Logical Screen Descriptor (7 bytes):
Global Color Table (if present):
Image Descriptor (10 bytes):
Image Data:
Extension Blocks:
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:
0x21
to indicate an extension block.0xFF
for application extension.0x0B
for 11 bytes).0x03
for three bytes).0x01
, indicating the presence of loop count data.0
indicates infinite looping.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.
For further reading, check out:
If I need to write test code, please let me know where I can write it.
@colinrtwhite