rav1e icon indicating copy to clipboard operation
rav1e copied to clipboard

Support max_width/max_height options.

Open tdaede opened this issue 3 years ago • 17 comments

Fixes #2467

tdaede avatar Aug 10 '20 18:08 tdaede

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.

rzumer avatar Aug 10 '20 18:08 rzumer

Coverage Status

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

coveralls avatar Aug 10 '20 19:08 coveralls

Added some doc comments.

tdaede avatar Aug 10 '20 20:08 tdaede

it needs a rebase.

lu-zero avatar Aug 26 '20 09:08 lu-zero

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.

jamrial avatar Sep 23 '20 15:09 jamrial

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

lu-zero avatar Jul 16 '21 20:07 lu-zero

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

tdaede avatar Jul 21 '21 00:07 tdaede

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

tdaede avatar Jul 23 '21 20:07 tdaede

LGTM besides one comment

rzumer avatar Jul 23 '21 21:07 rzumer

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.

jamrial avatar Jul 23 '21 21:07 jamrial

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.

tdaede avatar Sep 01 '21 13:09 tdaede

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

jamrial avatar Sep 01 '21 13:09 jamrial

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 avatar Sep 01 '21 13:09 tdaede

(we should probably remove that redundant CLI error printing)

tdaede avatar Sep 01 '21 13:09 tdaede

!! 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.

jamrial avatar Sep 01 '21 14:09 jamrial

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)" }

tdaede avatar Sep 01 '21 15:09 tdaede

Should be good then. Thanks.

jamrial avatar Sep 01 '21 18:09 jamrial