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

8169501: GIF animation is too fast #221

Closed
wants to merge 1 commit into from
Closed

Conversation

@bhaweshkc
Copy link
Collaborator

@bhaweshkc bhaweshkc commented May 15, 2020

issue is caused by the threshold value for frame duration used by javaFx before it gets normalized. JavaFx is using threshold value 10 while other browser (Safari, Firefox) is using 50 due to which, value between 10 and 50 don't get normalized and animation runs at faster speed. To fix the issue change frame duration normalization value to <= 50.
Safari : https://bugs.webkit.org/show_bug.cgi?id=14413
Firefox : https://bugzilla.mozilla.org/show_bug.cgi?id=386269


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Download

$ git fetch https://git.openjdk.java.net/jfx pull/221/head:pull/221
$ git checkout pull/221

bhawesh
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 15, 2020

👋 Welcome back bhaweshkc! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label May 15, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 15, 2020

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented May 15, 2020

While it does seem like the fix will make things better, the logic is a bit surprising to me. What it means is that very small values in the range [0,50] (currently [1,10] before your fix) will get forced to 100, but moderately small values, in the range [51,99] will be used directly. I wonder what the rationale was for this behavior in Safari is? I would have thought just a simple clamp (either to 50 or 100) would make more sense, but perhaps matching the popular browsers is better.

What I would like to see, though, is a comparison of behavior between:

  • WebView (both before and after your proposed fix)
  • JavaFX Image in an ImageView
  • Firefox
  • Safari
  • Chrome

Can you find two images (the one in the bug report can be one of them) with a frame delay value in the 11-50 range and also something in the 51-70 range and run the tests?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jun 30, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth requested review from kevinrushforth and removed request for kevinrushforth Jul 23, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

This is pending response to comments above.

@bhaweshkc bhaweshkc marked this pull request as draft Jul 23, 2020
@openjdk openjdk bot removed the rfr label Jul 23, 2020
@bhaweshkc
Copy link
Collaborator Author

@bhaweshkc bhaweshkc commented Sep 15, 2020

10ms10ms                   11ms11ms                  12ms12ms                  15ms15ms


19ms19ms                  20ms20ms                  21ms21ms                  40ms40ms                  


75ms75ms                  Originalgif

Without the fix, gif animation speed matches for all interval gifs with all other browser (Which includes firefox, safari, chrome).
Only animation speed for gif file which is attached in issue doesn't match. javafx webkit plays it faster.
Imageview plays everything at its original speed. No clamping happens here.

@bhaweshkc bhaweshkc marked this pull request as ready for review Sep 15, 2020
@openjdk openjdk bot added the rfr label Sep 15, 2020
@nlisker
Copy link
Collaborator

@nlisker nlisker commented Sep 15, 2020

@bhaweshkc
Copy link
Collaborator Author

@bhaweshkc bhaweshkc commented Sep 16, 2020

Is this related to https://bugs.openjdk.java.net/browse/JDK-8209560?

it seems not.

@kevinrushforth kevinrushforth self-requested a review Sep 19, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 19, 2020

Using the images you posted above, it appears that the browsers (Firefox and Edge on Windows, Firefox and Safari on Mac) have a threshold of 20ms, not 51 ms. Looks like the formula should be

delay < 20 : set to 100
delay >= 20 : use the value as is

Your fix does make the 19 ms image match the browsers (whereas existing WebView is too fast), but the 20ms, 21ms, and 40ms images no longer do (they are fine in the existing WebVIew and too slow with your patch).

So I recommend changing the value to 20 and then re-testing.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 30, 2020

This PR is on hold. It can be reopened or a new PR can be sent as and when it is ready to proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants