Scaling bug fix (issue 353) #354
Open
Conversation
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
It turned out special logic that makes sure Nvidia encoder limits are respected can sometimes cause problems. More specifically: - Resolution cropping logic may generate odd resolutions for the video stream, which is not supported by h.264 and usually shows up as a "green bar" at the bottom of the video (because encoder adds a line of zero YUV values, which translated to certain shade of green) - Portrait resolutions get needlessly enlarged, ie value that was requested as a height of the picture gets used as a width, and as aspect ratio is preserved, height is rescaled and much larger Two changes were made: 1) all scaled values are now rounded to the nearest even number and 2) the whole mechanism is not applied if the resolution stays within the encoder limits
Previous commit changed the resolution clamping approach: instead of changing every resolution given, change only resolutions that are not acceptable to the encoder. To make tests pass properly, one thing needed to change. Previously, when 600x300 resolution was given to the encoder with 700x500 limits, it got changed to the 250x500. While that was an expected behaviour, I don't think it is a proper one - why change something that encoder is fully willing to accept? So the test was modified to be OK, with original 600x300
Previous changes caused limits from the array in ffmpeg.go to be strictly applied, i.e. if the resolution was fitting the limits, no change was done. This uncovered the fact that the limits themselves were bad - on my machine NVenc refused to work with the message that the frame dimensions is too low. Changing the minimum resolution to 146x145 fixed the problem. Note that is is quite likely that the limits should be different for h265 as well, but on my machine all works. Proper approach for this problem would be something along the line of dynamic discovery of these limits (via Nvidia APIs) and then applying true local HW limits, not hardcoded ones.
|
Unfortunately I am running out of time here. In my opinion the problem is that existing code changes the resolutions even if they are in the "legal" range - but that allows some tests to succeed. I fixed a few of those but not all. So I guess somebody else has to do it. Alternative option is just adopt "rounding up to the nearest even number" from my first commit, this will eradicate green bars and everything else should work - only then the problem with changing the resolutions in the legal range will remain. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
So to be honest I failed to understand completely original author's intentions. It is a complex code, and getting resolutions/aspects right is notoriously tricky. BTDT. Also, this is my last week, and I don't be there to take care of debugging in case of problems.
Here is what I did, then:
I hope this is acceptable.