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

#6198 Fix missing transcoding speed info #6199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marissa999
Copy link

I noticed that the MediaInfoItem

if ((stream.AverageFrameRate || stream.RealFrameRate) && stream.Type === 'Video') {
attributes.push(createAttribute(globalize.translate('MediaInfoFramerate'), (stream.AverageFrameRate || stream.RealFrameRate)));
}
uses stream.AverageFrameRate || stream.RealFrameRate. However in the Playback Info Widget only videoStream.AverageFrameRate is used. This results in the Playback Info Widget not showing how much faster the transcoding process is compared to the original frames per seconds.

Changes
To also use the RealFrameRate in getDisplayTranscodeFps, in case the AverageFrameRate is null.

Issues
#6198

@marissa999 marissa999 requested a review from a team as a code owner October 14, 2024 12:42
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Oct 14, 2024

Cloudflare Pages deployment

Latest commit 0a7379f
Status ✅ Deployed!
Preview URL https://0c691533.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Copy link

sonarcloud bot commented Oct 14, 2024

@@ -200,7 +200,7 @@ function getDisplayTranscodeFps(session, player) {
const mediaSource = playbackManager.currentMediaSource(player) || {};
const videoStream = (mediaSource.MediaStreams || []).find((s) => s.Type === 'Video') || {};

const originalFramerate = videoStream.AverageFrameRate;
const originalFramerate = videoStream.AverageFrameRate || videoStream.RealFrameRate;
Copy link
Member

Choose a reason for hiding this comment

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

Better to use videoStream.ReferenceFrameRate || videoStream.RealFrameRate?

See also jellyfin/jellyfin#12603

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I have mostly only looked at jellybin-web, not at jellyfin-server.

I assume that ReferenceFrameRate could only possibly exist with newer Jellyfin-Server releases. Otherwise, if you have a newer web but older server then nothing would change.

I guess it depends on if web releases should work with older server releases or not. If yes then maybe it is needed to do
videoStream.ReferenceFrameRate || videoStream.AverageFrameRate || videoStream.RealFrameRate
But if older server releases dont matter, then I currently see no way how ReferenceFrameRate could ever be null, in that case maybe even const originalFramerate = videoStream.ReferenceFrameRate is enough 🤔

Though videoStream.ReferenceFrameRate || videoStream.RealFrameRate is probably also a healthy middleground. Something that ensures backwards compatibility (without fixing the bug) but something that also works for the future 🤔

Copy link
Member

Choose a reason for hiding this comment

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

We try to support one "major" version back when possible.

@thornbill thornbill added the bug Something isn't working label Oct 15, 2024
@thornbill thornbill added this to the v10.10.0 milestone Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants