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

Fix image loop counter on iOS 14 #30744

Closed
wants to merge 2 commits into from

Conversation

comvenger-brandon
Copy link
Contributor

Summary

Animated gifs, which do not loop, currently animate twice on iOS 14.
See: #30147

Changelog

[iOS] [Fixed] - Animated images without loop no longer animate twice

Test Plan

Run the example app with any animated gif. I attached a gif, which is affected.

checkmark

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 15, 2021
@analysis-bot
Copy link

analysis-bot commented Jan 15, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 65975dd

@analysis-bot
Copy link

analysis-bot commented Jan 15, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,898,018 -1,550
android hermes armeabi-v7a 8,402,326 -605
android hermes x86 9,384,334 -1,491
android hermes x86_64 9,329,699 -1,614
android jsc arm64-v8a 10,349,881 -1,548
android jsc armeabi-v7a 9,837,272 -624
android jsc x86 10,397,699 -1,497
android jsc x86_64 10,983,322 -1,603

Base commit: 65975dd

@mrousavy
Copy link
Contributor

"repeat twice" is misleading imo, it doesn't "repeat" twice, it "plays" twice - right?

@comvenger-brandon
Copy link
Contributor Author

"repeat twice" is misleading imo, it doesn't "repeat" twice, it "plays" twice - right?

Yup, correct. Did I write „repeat twice“ somewhere? :o

@mrousavy
Copy link
Contributor

@comvenger-brandon no, I'm just referring to this comment, maybe you can change that as well :)

@comvenger-brandon
Copy link
Contributor Author

@comvenger-brandon no, I'm just referring to this comment, maybe you can change that as well :)

Ah, you're right. Let's just change that as well, while we're at it.

@PeteTheHeat
Copy link
Contributor

Just so I understand clearly, previously:

A gif that should loop once would go twice.
A gif that should loop twice would go three times.

With this fix, infinite loops still work, and gifs that go once/twice/thrice go the correct number of times.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@PeteTheHeat has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@comvenger-brandon
Copy link
Contributor Author

Just so I understand clearly, previously:

A gif that should loop once would go twice.
A gif that should loop twice would go three times.

With this fix, infinite loops still work, and gifs that go once/twice/thrice go the correct number of times.

Correct.

@facebook-github-bot
Copy link
Contributor

@PeteTheHeat merged this pull request in 17aa1e3.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jan 21, 2021
@DheerajGussain
Copy link

if(@available(iOS 14, *)) {
} else {
// A loop count of 1 means it should repeat twice, 2 means, thrice, etc.
if (loopCount != 0) {
loopCount++;
}
}

after using the above code, still gif animating twice

@henninghh990
Copy link

if(@available(iOS 14, *)) {
} else {
// A loop count of 1 means it should repeat twice, 2 means, thrice, etc.
if (loopCount != 0) {
loopCount++;
}
}

after using the above code, still gif animating twice

Same for me

@comvenger-brandon
Copy link
Contributor Author

Care to share the gif file in question? I will have a look.
And just to be sure: you guys are testing on iOS14?

@henninghh990
Copy link

henninghh990 commented Feb 16, 2021

Care to share the gif file in question? I will have a look.
And just to be sure: you guys are testing on iOS14?

iOS 14.4

confetti

@DheerajGussain
Copy link

DheerajGussain commented Feb 16, 2021

Screen.Recording.2021-02-16.at.8.24.35.PM.mov

Care to share the gif file in question? I will have a look.
And just to be sure: you guys are testing on iOS14?

yes it's 14.4
attaching the gif file which used antimate once only before ios 14 and there is another video which shows the current behaviour on app

vid-7

@comvenger-brandon
Copy link
Contributor Author

Care to share the gif file in question? I will have a look.
And just to be sure: you guys are testing on iOS14?

iOS 14.4

The shared image only plays once when I test it on iOS 14.4 (iPhone 12).

Are you sure you correctly applied the patch (via patch-package for example)? This change is not yet live in RN stable.

@DheerajGussain
Copy link

Care to share the gif file in question? I will have a look.
And just to be sure: you guys are testing on iOS14?

iOS 14.4

The shared image only plays once when I test it on iOS 14.4 (iPhone 12).

Are you sure you correctly applied the patch (via patch-package for example)? This change is not yet live in RN stable.

hey can you explain how to do that, iam using expo on my project

@henninghh990
Copy link

henninghh990 commented Feb 18, 2021

Care to share the gif file in question? I will have a look.
And just to be sure: you guys are testing on iOS14?

iOS 14.4

The shared image only plays once when I test it on iOS 14.4 (iPhone 12).

Are you sure you correctly applied the patch (via patch-package for example)? This change is not yet live in RN stable.

Yes, I applied it.
But whatever I do in this file, Expo doesn't "Refresh" automaticly, and even if I write something that would crash the app, nothig happens. It's like this file isn't included or something.

EDIT: I'm using managed workflow, if that can affect this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants