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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b8b120
Add constant to parse Gif Loop Count and apply to repeatCount
tgyuuAn Nov 7, 2024
5b302b6
Removed gif parsing and changed encoded loop count to be available on…
tgyuuAn Nov 8, 2024
6ed462e
Remove unnecessary code and ENCODED_LOOP_COUNT not to be applied when…
tgyuuAn Nov 11, 2024
769d0b4
Remove unnecessary code
tgyuuAn Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion coil-gif/src/main/java/coil3/gif/AnimatedImageDecoder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import coil3.decode.Decoder
import coil3.decode.ImageSource
import coil3.decode.toImageDecoderSourceOrNull
import coil3.fetch.SourceFetchResult
import coil3.gif.MovieDrawable.Companion.ENCODED_LOOP_COUNT
import coil3.gif.internal.animatable2CallbackOf
import coil3.gif.internal.asPostProcessor
import coil3.gif.internal.maybeWrapImageSourceToRewriteFrameDelay
Expand Down Expand Up @@ -54,6 +55,7 @@ class AnimatedImageDecoder(

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

val drawable = runInterruptible {
maybeWrapImageSourceToRewriteFrameDelay(source, enforceMinimumFrameDelay).use { source ->
val imageSource = source.toImageDecoderSourceOrNull(options, animated = true)
Expand Down Expand Up @@ -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

baseDrawable.repeatCount = options.repeatCount
}

// Set the start and end animation callbacks if any one is supplied through the request.
val onStart = options.animationStartCallback
Expand Down
15 changes: 14 additions & 1 deletion coil-gif/src/main/java/coil3/gif/MovieDrawable.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import android.graphics.PorterDuff
import android.graphics.Rect
import android.graphics.drawable.AnimatedImageDrawable
import android.graphics.drawable.Drawable
import android.os.Build.VERSION.SDK_INT
import android.os.SystemClock
import androidx.annotation.RequiresApi
import androidx.core.graphics.createBitmap
import androidx.core.graphics.withSave
import androidx.vectordrawable.graphics.drawable.Animatable2Compat
Expand Down Expand Up @@ -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.

if (SDK_INT >= 28) {
require(repeatCount >= ENCODED_LOOP_COUNT) { "Invalid repeatCount: $repeatCount" }
} else {
require(repeatCount >= REPEAT_INFINITE) { "Invalid repeatCount: $repeatCount" }
}
this.repeatCount = repeatCount
}

Expand Down Expand Up @@ -284,5 +290,12 @@ class MovieDrawable @JvmOverloads constructor(
companion object {
/** Pass this to [setRepeatCount] to repeat infinitely. */
const val REPEAT_INFINITE = -1

/**
* Pass this to [setRepeatCount] to repeat according to encoded LoopCount metadata.
* 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 •
edited
Loading

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.

}
}
8 changes: 7 additions & 1 deletion coil-gif/src/main/java/coil3/gif/imageRequests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ package coil3.gif
import android.graphics.ImageDecoder
import android.graphics.drawable.AnimatedImageDrawable
import android.graphics.drawable.Drawable
import android.os.Build.VERSION.SDK_INT
import coil3.Extras
import coil3.annotation.ExperimentalCoilApi
import coil3.getExtra
import coil3.gif.MovieDrawable.Companion.ENCODED_LOOP_COUNT
import coil3.gif.MovieDrawable.Companion.REPEAT_INFINITE
import coil3.request.ImageRequest
import coil3.request.Options
Expand All @@ -17,7 +19,11 @@ import coil3.request.Options
* @see AnimatedImageDrawable.setRepeatCount
*/
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.


Expand Down
Loading