-
Notifications
You must be signed in to change notification settings - Fork 71
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 Transcode Quality #371
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
e3dc8d1
Test transcoding quality
leszko b3b66ca
Fix for testing in GCP
leszko 5b6c119
Add CRF param
leszko 3a68bd2
Add preset and tier params
leszko 720e18a
Add CRF param
leszko 4f482c9
Update CQ param
leszko 8396865
Updates for testing
leszko 3948710
Updates before PR
leszko e9afe5d
Add CRF to JsonProfile
leszko 56d3191
Update how cq is calculated from crf
leszko 5432f68
Introduce range for CRF and CQ params
leszko f718efe
Update how cq is calculated from crf
leszko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you end up with this formula? Lower CQ values typically indicate better quality at expense of compressions. Also encoders typically will pick either CQ and CRF as the main control so I'm not sure if CQ here will actually make a difference. Do you see a difference if we leave out CQ changes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRF is used by CPU-based transcoding, CQ is used by GPU-based transcoding.
So, only one of these values will be used at the time.
Technically there is no direct numerical correspondence between CQ and CRF, so the other option would be to set both CRF and CQ in our API. But I think it's not a pragmatic approach right now. Most of our transcoders are GPU anyway.
How did I come with
cq := p.Profile.CRF + 7
, just experimenting with videos. But obviously not for all videos it will give the same results, since technically CRF and CQ values do not correspond to each other.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
cq := p.Profile.CRF + 7
doesn't quite make sense to me esp since they don't correspond to one another. Also if CRF is CPU only, then why do we need it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explained here: https://discord.com/channels/423160867534929930/1140578841136484402/1149041283583721634
lets discuss more in discord