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.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tdaede
Copy link
Collaborator

@tdaede tdaede commented Aug 10, 2020

Fixes #2467

@rzumer
Copy link
Collaborator

rzumer commented Aug 10, 2020

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.

src/capi.rs Outdated Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented Aug 10, 2020

Coverage Status

Coverage decreased (-0.02%) to 83.278% when pulling 7d049a9 on tdaede:max_frame_width into 54b667c on xiph:master.

@tdaede tdaede force-pushed the max_frame_width branch 2 times, most recently from 224f557 to a49ec00 Compare August 10, 2020 20:00
@tdaede
Copy link
Collaborator Author

tdaede commented Aug 10, 2020

Added some doc comments.

@lu-zero
Copy link
Collaborator

lu-zero commented Aug 26, 2020

it needs a rebase.

src/header.rs Show resolved Hide resolved
src/encoder.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@vibhoothi vibhoothi left a 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

@jamrial
Copy link
Contributor

jamrial commented Sep 23, 2020

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.

@lu-zero lu-zero added this to In progress in Release 0.5 Jan 14, 2021
@lu-zero
Copy link
Collaborator

lu-zero commented Jul 16, 2021

This patch needs are rebase, should we land it before 0.5?

@tdaede
Copy link
Collaborator Author

tdaede commented Jul 21, 2021

This has rotted pretty badly, it'll need some work but I would like it in.

@tdaede tdaede force-pushed the max_frame_width branch 2 times, most recently from 495bd04 to 3dd5ef7 Compare July 23, 2021 20:44
@tdaede
Copy link
Collaborator Author

tdaede commented Jul 23, 2021

A long time coming, but I've rebased this and fixed the nits.

src/api/config/encoder.rs Outdated Show resolved Hide resolved
@rzumer
Copy link
Collaborator

rzumer commented Jul 23, 2021

LGTM besides one comment

@jamrial
Copy link
Contributor

jamrial commented Jul 23, 2021

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:

!! Invalid configuration: invalid width 720 (expected >= 16, <= 65535)
Error: Generic { msg: "Invalid configuration", e: "invalid width 720 (expected >= 16, <= 65535)" }

Which is confusing and wrong.

@tdaede
Copy link
Collaborator Author

tdaede commented Sep 1, 2021

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.

@jamrial
Copy link
Contributor

jamrial commented Sep 1, 2021

The first option sounds good. Basically, replacing the hardcoded 65535 with something like min(65535, max_width).

@tdaede
Copy link
Collaborator Author

tdaede commented Sep 1, 2021

OK, it now displays:

!! Invalid configuration: invalid width 720 (expected >= 16, <= 720)
Error: Generic { msg: "Invalid configuration", e: "invalid width 720 (expected >= 16, <= 720)" }

@tdaede
Copy link
Collaborator Author

tdaede commented Sep 1, 2021

(we should probably remove that redundant CLI error printing)

@jamrial
Copy link
Contributor

jamrial commented Sep 1, 2021

!! Invalid configuration: invalid width 720 (expected >= 16, <= 720)

But 720 is <= 720.

Also, if i passed --max_width=64, it should report !! Invalid configuration: invalid width 720 (expected >= 16, <= 64) for an input video with width 720.

@tdaede
Copy link
Collaborator Author

tdaede commented Sep 1, 2021

Ah indeed I forgot to update a parameter of the format string. Now displays:

!! Invalid configuration: invalid width 1280 (expected >= 16, <= 720)
Error: Generic { msg: "Invalid configuration", e: "invalid width 1280 (expected >= 16, <= 720)" }

@jamrial
Copy link
Contributor

jamrial commented Sep 1, 2021

Should be good then. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 0.5
  
In progress
Development

Successfully merging this pull request may close these issues.

API to set sequence header max resolution
8 participants