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.

Sign up for GitHub

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

Jump to bottom

Improve Transcode Quality #2848

Merged
merged 9 commits into from
Sep 14, 2023
Merged

Improve Transcode Quality #2848

merged 9 commits into from
Sep 14, 2023

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Aug 29, 2023

Related to livepeer/lpms#371

@leszko leszko marked this pull request as ready for review September 6, 2023 14:49
@leszko leszko mentioned this pull request Sep 6, 2023
Merged
emranemran
emranemran reviewed Sep 8, 2023
View reviewed changes
net/lp_rpc.proto Outdated
@@ -301,6 +301,7 @@ message VideoProfile {
CHROMA_444 = 2;
}
ChromaSubsampling chromaFormat = 27;
uint32 crf = 28;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the default? I'd suggest setting this lower since it translates to a pretty high CQ.

Copy link
Contributor

@emranemran emranemran Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK i see now after reading your comment in the other PR. But should this be 27 like the other PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a value, just the field number in the probuf. So, in short, in just needs to be the next number after the line above.

@leszko
Copy link
Contributor Author

leszko commented Sep 8, 2023

@emranemran as discussed, I renamed crf to quality everywhere. PTAL.

@codecov
Copy link

codecov bot commented Sep 8, 2023 •
edited
Loading

Codecov Report

Merging #2848 (8e486a1) into master (4a4a956) will increase coverage by 0.02012%.
Report is 2 commits behind head on master.
The diff coverage is 100.00000%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2848         +/-   ##
===================================================
+ Coverage   56.48625%   56.50637%   +0.02012%     
===================================================
  Files             88          88                 
  Lines          19233       19235          +2     
===================================================
+ Hits           10864       10869          +5     
+ Misses          7770        7767          -3     
  Partials         599         599
Files Changed Coverage Δ
common/util.go 60.41667% <100.00000%> (+0.11816%) ⬆️
server/segment_rpc.go 78.26725% <100.00000%> (+0.03196%) ⬆️

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a4a956...8e486a1. Read the comment docs.

Files Changed Coverage Δ
common/util.go 60.41667% <100.00000%> (+0.11816%) ⬆️
server/segment_rpc.go 78.26725% <100.00000%> (+0.03196%) ⬆️

... and 1 file with indirect coverage changes

@leszko leszko merged commit aa81378 into master Sep 14, 2023
17 checks passed
@leszko leszko deleted the rafal/transcode-quality-params branch September 14, 2023 10:21
eliteprox pushed a commit to eliteprox/go-livepeer that referenced this pull request Feb 21, 2024
@leszko @eliteprox
Improve Transcode Quality (livepeer#2848)
9b1ed7e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomshutt thomshutt thomshutt approved these changes

@emranemran emranemran Awaiting requested review from emranemran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@leszko @thomshutt @emranemran