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

Separate bitrate control from resolution #6071

Merged
merged 9 commits into from
Sep 20, 2024

Conversation

gnattu
Copy link
Member

@gnattu gnattu commented Sep 14, 2024

Changes

  • Remove all resolution related text from bitrate control as the server does not set resolution based on that. The actual field is kept as I'm unsure if that is used anywhere
  • Only keep on entry of bitrate that is higher than original bitrate to allow using source bitrate, hide all other entries as the extremely high ones like 120Mbps is meaningless for 4Mbps source, and such option usually confuses our user.

Issues

A follow up of the server change: jellyfin/jellyfin#12644, but that not depends on that PR, this just works better with that merged.

@gnattu gnattu requested a review from a team as a code owner September 14, 2024 08:53
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.

ESLint doesn't pass. Please fix all ESLint issues.

src/components/qualityOptions.js Outdated Show resolved Hide resolved
src/components/qualityOptions.js Outdated Show resolved Hide resolved
src/components/qualityOptions.js Outdated Show resolved Hide resolved
src/components/qualityOptions.js Outdated Show resolved Hide resolved
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.

ESLint doesn't pass. Please fix all ESLint issues.

src/components/qualityOptions.js Outdated Show resolved Hide resolved
src/components/qualityOptions.js Outdated Show resolved Hide resolved
@dmitrylyzo dmitrylyzo added the enhancement Improve existing functionality or small fixes label Sep 14, 2024
Co-authored-by: Dmitry Lyzo <56478732+dmitrylyzo@users.noreply.github.com>
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

Seems to work.

  1. Assume we have HEVC/AV1 and have to transcode to H264. The next higher bitrate may not be enough for comparable quality.

  2. When we select some bitrate (say 15Mbps) and then start another video with a lower bitrate (say 4Mbps), it puts a checkmark on Auto. It worked the same way before, though. 🤷‍♂️

@gnattu
Copy link
Member Author

gnattu commented Sep 16, 2024

When we select some bitrate (say 15Mbps) and then start another video with a lower bitrate (say 4Mbps), it puts a checkmark on Auto. It worked the same way before, though. 🤷‍♂️

I noticed this as well but I decided to leave it the same way as before because it always has been like this for a few years. If we want to change this we need to think about what way is better and change that accordingly which is out of scope of this PR.

Assume we have HEVC/AV1 and have to transcode to H264. The next higher bitrate may not be enough for comparable quality.

Or do we port the scaling function into the web as well and also scale based on that?

@dmitrylyzo
Copy link
Contributor

dmitrylyzo commented Sep 16, 2024

Or do we port the scaling function into the web as well and also scale based on that?

That would be ideal in terms of math, but it will require them to be synchronized. If only we could add this function to the SDK.

As I see, we were adding bitrates for the next higher resolution. This produces more than 1 extra bitrate.
For simplicity, we can go this way.

Hmm, no - we limit it with the currently selected resolution. But using a second higher bitrate is still an option.

Since we are detaching bitrate from resolution, this becomes unnecessary:

function setSelectValue(select, value, defaultValue) {
select.value = value;
if (select.selectedIndex < 0) {
select.value = defaultValue;
}
}
function onMaxVideoWidthChange(e) {
const context = this.options.element;
const selectVideoInNetworkQuality = context.querySelector('.selectVideoInNetworkQuality');
const selectVideoInternetQuality = context.querySelector('.selectVideoInternetQuality');
const selectChromecastVideoQuality = context.querySelector('.selectChromecastVideoQuality');
const selectVideoInNetworkQualityValue = selectVideoInNetworkQuality.value;
const selectVideoInternetQualityValue = selectVideoInternetQuality.value;
const selectChromecastVideoQualityValue = selectChromecastVideoQuality.value;
const maxVideoWidth = parseInt(e.target.value || '0', 10) || 0;
fillQuality(selectVideoInNetworkQuality, true, 'Video', maxVideoWidth);
fillQuality(selectVideoInternetQuality, false, 'Video', maxVideoWidth);
fillChromecastQuality(selectChromecastVideoQuality, maxVideoWidth);
setSelectValue(selectVideoInNetworkQuality, selectVideoInNetworkQualityValue, '');
setSelectValue(selectVideoInternetQuality, selectVideoInternetQualityValue, '');
setSelectValue(selectChromecastVideoQuality, selectChromecastVideoQualityValue, '');
}

@gnattu
Copy link
Member Author

gnattu commented Sep 17, 2024

Hmm, no - we limit it with the currently selected resolution. But using a second higher bitrate is still an option.

As a simplified option we can just display the option that is first higher than 1.5x of the source bitrate when we are doing hevc/av1/vp9 to h264. This is a rough estimate but should work good enough for most of them because it is the estimated compression efficiency in common bitrate range (8-10mbit/s)

@gnattu
Copy link
Member Author

gnattu commented Sep 17, 2024

I made the tradeoff option: increase the bitrate used to filter the control options by a factor of 1.5 when it is not too high (below the 20Mbps option). Ideally we only have to do this when transcoding to h264, but unfortunately we need extra api call to get this kind of info and it is not ideal to me. Maybe we can improve the API in future releases but I think this is okay for now.

Copy link

sonarcloud bot commented Sep 17, 2024

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit d9d2f8d0be6ff06cb23dd265441dfed0761b3403
Status ✅ Deployed!
Preview URL https://e0bc0104.jellyfin-web.pages.dev
Type 🔀 Preview

@thornbill thornbill added this to the v10.10.0 milestone Sep 20, 2024
@thornbill thornbill merged commit 5e17cbe into jellyfin:master Sep 20, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or small fixes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants