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

Fix for MPEG-1 video pixel aspect ratio #411

Merged
merged 5 commits into from
Feb 15, 2023

Conversation

bvibber
Copy link
Contributor

@bvibber bvibber commented Feb 15, 2023

The reciprocal of the actual value was being returned for MPEG-1 video. Files with square pixels were not affected, nor were MPEG-2 files listing a display aspect ratio.

Fixes #410

The reciprocal of the actual value was being returned for MPEG-1
video. Files with square pixels were not affected, nor were MPEG-2
files listing a display aspect ratio.

Fixes JamesHeinrich#410
This gives a nice 320px result instead of 321 point something
when producing square pixel output and is commonly used.
Getting this warning:

Binary operation "/" between 1 and 0.0|0.6735|0.7031|0.7615|0.8055|0.8437|0.8935|0.91666666666667|0.9815|1.0|1.0255|1.0695|1.1|1.1575|1.2015|1.3333|1.7778|2.21 results in an error.

Hopefully this will convince it that it won't divide by zero?
@bvibber
Copy link
Contributor Author

bvibber commented Feb 15, 2023

I don't understand why static analysis thinks the division can cause an error here -- the only possible error is divide by zero, which is guarded explicitly against.

@JamesHeinrich JamesHeinrich merged commit 5756928 into JamesHeinrich:master Feb 15, 2023
@StudioMaX
Copy link
Collaborator

StudioMaX commented Feb 15, 2023

It might be dumb, but... We have a check isset($lookup[$mpeg_version][$rawaspectratio], which checks for the presence of a key in the array, but we need to look for values instead.
UPD: forget this, I was wrong.

@JamesHeinrich
Copy link
Owner

... but we need to look for values instead.
Can you explain what you mean further, please?

@bvibber
Copy link
Contributor Author

bvibber commented Feb 15, 2023

There is a guard specifically for $ratio != 0 so I'm confused why 1 / $ratio is troublesome to the analyzer. :)

@Krinkle
Copy link

Krinkle commented Feb 15, 2023

@Brion The ratio is always casted to a float before this afaik, in which case 0.0 (as float rather than int) should work equally well, and satisfy the analyser. You could then also use strict equality, which would currently fail. I'm guessing the analyser is using strict equality to conservatively narrow down possible values.

@bvibber
Copy link
Contributor Author

bvibber commented Feb 15, 2023

@Krinkle wrote:

@Brion The ratio is always casted to a float before this afaik, in which case 0.0 (as float rather than int) should work equally well, and satisfy the analyser. You could then also use strict equality, which would currently fail. I'm guessing the analyser is using strict equality to conservatively narrow down possible values.

I'll try that in a followup and see if it helps. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPEG-1 pixel aspect ratio is reciprocal of actual value
4 participants