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

Incorrect aspect ratio handling #48

Closed
tvasenin opened this issue Feb 7, 2024 · 7 comments
Closed

Incorrect aspect ratio handling #48

tvasenin opened this issue Feb 7, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@tvasenin
Copy link

tvasenin commented Feb 7, 2024

Currently, ffmpeg is invoked with -aspect 4:3 or 16:9.

This is incorrect, as 4:3 is the display aspect ratio for 922x576 (for PAL), but the output video (with default padding) is 928px wide.

The fix would be to remove the -aspect option entirely, as the correct SAR is already provided in stream header by ld-chroma-decoder:

https://github.com/oyvindln/vhs-decode/blob/d4c3f8504573b0d5cc1d1777dea7c5b69be6a161/tools/ld-chroma-decoder/outputwriter.cpp#L158

@JuniorIsAJitterbug
Copy link
Owner

You are correct, this is a mistake.
I will leave -aspect 16:9 for --force-anamorphic and --letterbox but remove for everything else.

Thank you for the report. 👍️

@JuniorIsAJitterbug JuniorIsAJitterbug added the bug Something isn't working label Feb 8, 2024
@JuniorIsAJitterbug JuniorIsAJitterbug self-assigned this Feb 8, 2024
@tvasenin
Copy link
Author

tvasenin commented Feb 8, 2024

I will leave -aspect 16:9 for --force-anamorphic and --letterbox but remove for everything else.

In these cases, it's still incorrect to pass -aspect 16:9, as the resulting AR will be wrong due to the padding (and potential vertical crop overrides).

You should override SAR instead, as per the code above:
PAL Widescreen: -vf "setsar=512/461:max=1000"
NTSC Widescreen: -vf "setsar=194/171:max=1000"

(max=1000 option is required, otherwise ffmpeg will try to lossy-reduce the ratio to make numerator and denominator become <= 100)

@JuniorIsAJitterbug
Copy link
Owner

JuniorIsAJitterbug commented Feb 8, 2024

Great, thank you for the information.
I've tested the values you sent and I think the PAL values should be swapped?

setsar=194/171:max=1000 = 1052x432 2.44
setsar=171/194:max=1000 = 928x490 1.89
setsar=97/114:max=1000 = 760x394 1.93

@tvasenin
Copy link
Author

tvasenin commented Feb 8, 2024

No, it shouldn't be swapped, as this is SAR (sample aspect ratio aka 'the shape of the pixel'), not DAR.

If you want to understand how it's derived, take a look at the source code above.

@JuniorIsAJitterbug
Copy link
Owner

JuniorIsAJitterbug commented Feb 8, 2024

Understood.
Does the code above not use the following?

PAL 16:9 = 512:461
PAL 4:3 = 384:461
NTSC 16:9 = 194:171
NTSC 4:3 = 97:114

Apologies if I've misread this again. 🙂

@tvasenin
Copy link
Author

tvasenin commented Feb 8, 2024

Yes, these values are correct.
It was wrong copy-paste on my side -- updated my post.

@JuniorIsAJitterbug
Copy link
Owner

I have added the 16:9 setsar values when --force-anamorphic and --letterbox are used.
-ar usage has been removed.

I hope this is now solved. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants