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

Improve thumbnail in video progress bar #1218

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

NicoPennec
Copy link
Member

@NicoPennec NicoPennec commented Oct 12, 2023

Problem

  • On Edit page, there is no thumbnail on video progress bar.
  • Too much styling calculation on mouseover progress bar.
  • The tile image is not preloading of first init of component.

Solution

  • Fix missing tile on video progress for Edit page.
  • Refactor frame tile style in order to reduce compute.
  • Improve tile preloading on init component.

@frankrousseau
Copy link
Contributor

I'm a little bit concerned by this change. The natural dimensions led too many edge cases. That's why I decided to set the size on the backend size.

@NicoPennec
Copy link
Member Author

NicoPennec commented Oct 13, 2023

@frankrousseau

I'm a little bit concerned by this change. The natural dimensions led too many edge cases. That's why I decided to set the size on the backend size.

After several tests, the dimension given by the video player still seems good to me. But I understand that you prefer to keep this logic on the back-end side.

The dimension given by the Kitsu API (Zou) can be wrong on some cases. So It will be necessary to modify the endpoint[1] to return the correct resolution based on the Display aspect ratio (DAR) for each preview video. (ping @EvanBldy)

[1] GET /api/data/playlists/entities/{id}/preview-files

@NicoPennec NicoPennec changed the title [player] fix thumbnail dimension in video progress bar Improve thumbnail in video progress bar Oct 13, 2023
@frankrousseau
Copy link
Contributor

It's better to ensure a proper computation on the backend side. The solution you propose requires to load the video before displaying the thumbnail.

@NicoPennec
Copy link
Member Author

I removed the commit that was causing debate.

@NicoPennec NicoPennec merged commit 5faefd4 into cgwire:master Oct 17, 2023
4 checks passed
@NicoPennec NicoPennec deleted the fix/video-progress branch October 17, 2023 11:25
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.

2 participants