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

Reduce GIF frame window from 30ms to 20ms #3907

Merged
merged 5 commits into from Sep 4, 2022

Conversation

TroyKomodo
Copy link
Contributor

@TroyKomodo TroyKomodo commented Aug 6, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Currently the check for the next animated frame is every 30ms which happens to be too slow for the gif standard, the maximum speed of a gif is 20ms per frame.

An edge case would be when a gif only has 2 frames

Frame Number Frame delay
1 20ms
2 20ms

The total frame duration is 40ms

In the current implementation, if we start at a 0 offset

Offset Offset mod 40 Time Passed Frame Displayed
0 0 0ms 1
33 33 30ms 2
66 26 60ms 2
99 19 90ms 1
132 12 120ms 1

As you can see the frames are duplicated when they shouldn't be.

Its like a train station with trains ever 30ms and we are only ready to board every 20ms, and since we are impatient we miss our train.

Perhaps a smarter way of detecting which frame to select is a more performant approach however I haven't thought of how to do that. Or we could just say that 30ms is the min frame timing instead of 20ms

fixes SevenTV#85

This one works and has been tested on windows by myself, I haven't tested osx because i dont have a mac.

Please can someone else confirm that this works on windows, linux and osx.
https://stackoverflow.com/questions/19958788/qt-5-1-1-and-opengl-rendering-speed

@Felanbird
Copy link
Collaborator

Seems fine on Windows, I can slightly see the speed difference on old emotes, but without lag I doubt others will be able to notice

will need the changelog again

@TroyKomodo
Copy link
Contributor Author

TroyKomodo commented Aug 6, 2022

The slight difference is likely actually lag on the state before this pr on windows (33ms + non precise timer). Since the faster emote timing is now closer to how browsers are implemented. Try comparing the emotes in chatterino vs the raw url in a browser tab. @Felanbird

@TroyKomodo
Copy link
Contributor Author

Perhaps @pajlada we can write a test to test the interval at which the timer fires. Which can help mitigate this problem. I would do it but i dont know how the testing works here.

@TroyKomodo
Copy link
Contributor Author

The vcpkg config change is because that dep is missing when you try install via vcpkg. Probs unrelated to this pr.

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

This change makes sense to me, to ensure this is tested appropriately could you provide a list of a couple of emotes + a channel where they can be used in so we can crowd-test on different systems?

Something like this:

| Twitch Channel | Emote Name | Link to emote |
|----------------|------------|---------------|
| A              | B          | C             |
| A              | B          | C             |
Twitch Channel Emote Name Link to emote
A B C
A B C

vcpkg.json Outdated Show resolved Hide resolved
@TroyKomodo
Copy link
Contributor Author

Twitch Channel Emote Name Link to emote
pajlada WAYTOODANK https://betterttv.com/emotes/5ad22a7096065b6c6bddf7f3
pajlada Ninja https://betterttv.com/emotes/5e7f2cc0d6581c3724c16b84
pajlada pajaAAAAAAAAAAA https://betterttv.com/emotes/5e8b989d09989e5cdff54e2e
pajlada pajaHop https://betterttv.com/emotes/56eddabbf854c2297286e5e8
pajlada AlienPls https://betterttv.com/emotes/5805580c3d506fea7ee357d6
pajlada ppHopper https://betterttv.com/emotes/5b0da25016252035ad06900e
pajlada pajaSWA https://betterttv.com/emotes/5738dc6ad7a0b60d147270a0

When comparing timings please compare between this commit and the master commit and also look at the browser since this change should try and make it as close to how its implemented in the browser as possible. (the master commit might be wrong)

@pajlada
Copy link
Member

pajlada commented Sep 3, 2022

@TroyKomodo Merge the latest main branch into your branch

@Felanbird
Copy link
Collaborator

Does this actually fix the chatterino7 issue? It seems a bit faster but is still not the required speed to match (the thumbnail/the website listing)

I can't show it in a gif b/c the thumbnail is 2 fast 2 furious

@TroyKomodo
Copy link
Contributor Author

Currently the min allowed is 20ms on our fork people have been asking me to allow less than 20 because of webp allowing 1ms frame timings and stuff like that. I haven't allowed it yet. However we might revisit this change when we get round to the c2 c7 merge.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -5,7 +5,7 @@

namespace chatterino {

constexpr long unsigned gifFrameLength = 33;
constexpr long unsigned gifFrameLength = 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for global variable 'gifFrameLength' [readability-identifier-naming]

Suggested change
constexpr long unsigned gifFrameLength = 20;
constexpr long unsigned GIF_FRAME_LENGTH = 20;

Copy link
Collaborator

Choose a reason for hiding this comment

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

good idea clang-tidy but we will do that tomorrow if its correct

@Felanbird Felanbird merged commit bc38d69 into Chatterino:master Sep 4, 2022
@TroyKomodo TroyKomodo deleted the troy-fix-frame-timings branch September 4, 2022 16:17
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.

Some emotes are different speed in Chatterino and on 7TV
3 participants