-
Notifications
You must be signed in to change notification settings - Fork 252
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
Support max_width/max_height options. #2490
base: master
Are you sure you want to change the base?
Conversation
d8898c6
to
9531e27
Compare
It could be useful to state in the doc comments what implications these options have on the decoded bitstream. Without reading the spec it's hard to tell what they might be used for. |
9531e27
to
65fc871
Compare
224f557
to
a49ec00
Compare
Added some doc comments. |
it needs a rebase. |
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.
Now it requires some rebase as #2520 is merged
Also, the spec states that if reduced_still_picture_header is enabled (currently always forced for still_picture), dimensions in the sequence header and the frame header must be the same, so you should check the user provided arguments to ensure this is the case. |
This patch needs are rebase, should we land it before 0.5? |
This has rotted pretty badly, it'll need some work but I would like it in. |
495bd04
to
3dd5ef7
Compare
A long time coming, but I've rebased this and fixed the nits. |
LGTM besides one comment |
If i set a max_width value lower than the width of the actual frames (e.g. a 720x480 y4m source encoded with --max-width=64) i get this error:
Which is confusing and wrong. |
Would you prefer that the error message for width/height be updated to indicate that it should be less than 65535 or max_width, or that a new error message for explicitly max_width be created? (and if so, we could technically error either for the width or max_width setting?) FWIW for the S-Frame content that this is useful for right now, the muxing happens outside of rav1e, so it itself doesn't have to see the width/height change dynamically. |
The first option sounds good. Basically, replacing the hardcoded 65535 with something like min(65535, max_width). |
02fde85
to
276aae3
Compare
276aae3
to
202afe1
Compare
OK, it now displays:
|
202afe1
to
1165103
Compare
(we should probably remove that redundant CLI error printing) |
1165103
to
8227deb
Compare
But 720 is <= 720. Also, if i passed --max_width=64, it should report |
8227deb
to
d2e0838
Compare
Ah indeed I forgot to update a parameter of the format string. Now displays:
|
d2e0838
to
7d049a9
Compare
Should be good then. Thanks. |
Fixes #2467