Improve Transcode Quality #832
Conversation
video/profiles.go
Outdated
| @@ -12,6 +12,7 @@ const ( | |||
| MaxVideoBitrate = 288_000_000 | |||
| TrackTypeVideo = "video" | |||
| TrackTypeAudio = "audio" | |||
| defaultCRF = 27 | |||
There was a problem hiding this comment.
Why 27? I think ffmpeg default is 23 actually.
There was a problem hiding this comment.
Just from some tests. Please check this part of design doc for details. 23 will give way bigger segment size.
|
@mjh1 After my change, all the transcode profiles JSON tests are failing. Do you have any way to re-generate them or something? How did they got created? |
video/profiles.go
Outdated
| // relativeBitrate needs to be slightly higher than the proportional average bitrate of the source video. | ||
| // Livepeer network uses bitrate to set max bitrate for encoding, so for the video to look good, we multiply | ||
| // it by a factor of 1.2. | ||
| relativeBitrate := 1.2 * float64(profile.Width*profile.Height) * (float64(videoBitrate) / float64(video.Width*video.Height)) |
There was a problem hiding this comment.
how did we determine the 1.2 multiplier?
There was a problem hiding this comment.
I didn't really. I just know that what we currently have is too low, because we're setting: max bitrate = source avg bitrate which gives poor results.
As described in the doc, I plan to not set it at all, when the Os start to support the quality param (Phase 2).
video/profiles.go
Outdated
| profiles := make([]EncodedProfile, 0) | ||
| var CRF uint = defaultCRF | ||
| if videoProfile.CRF != 0 { | ||
| CRF = videoProfile.CRF |
There was a problem hiding this comment.
CRF = 0 is still a valid value (meaning lossless). Are we explicitly saying crf = 0 should never be allowed or should this check for values between 0 and 51?
There was a problem hiding this comment.
This is a good point. The problem with 0 is that everywhere in go and protobuf, it's the default => so, the assumption is that 0 is a not set value.
Anyway, since as you know, there is no direct correspondence of the the quality value and the crf / cq, let's maybe not think about this corner case too much.
…into rafal/transcode-quality-params
|
@emranemran I also renamed the |
Codecov Report
@@ Coverage Diff @@
## main #832 +/- ##
===================================================
- Coverage 52.16754% 52.15693% -0.01061%
===================================================
Files 63 63
Lines 6136 6143 +7
===================================================
+ Hits 3201 3204 +3
- Misses 2675 2679 +4
Partials 260 260 see 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -134,7 +142,10 @@ func GetDefaultPlaybackProfiles(video InputTrack) ([]EncodedProfile, error) { | |||
| // check it here. | |||
| lowerQualityThanSrc := profile.Height < video.Height && profile.Bitrate < video.Bitrate | |||
| if lowerQualityThanSrc { | |||
There was a problem hiding this comment.
Don't we also want this for the rendition that's the same resolution as the source?
There was a problem hiding this comment.
My understanding is that no, because for the same resolution as the source we create a profile in line 157. So, this logic here is (and always was) for the profile resolution lower than source.
There was a problem hiding this comment.
Ohh wait, maybe you're right that we should also increase the bitrate in line 157. Will update it there.
Phase 1 as described in the related Design Doc.
Related to livepeer/lpms#371 and livepeer/go-livepeer#2848
Before most Os update the go-livepeer version to support the CRF parameter, I suggest to do the following in order to improve slightly the transcode quality:
Rationale from Design Doc: