Skip to content

Conversation

YanNoun
Copy link
Contributor

@YanNoun YanNoun commented Aug 21, 2025

This PR fixes the 35mm focal <-> ratio focal conversion :

  • 3/2 image ratio uses 36mm filback width
  • 4/3 uses a 34mm one

Branch is on top of optim-large

@meta-cla meta-cla bot added the cla signed label Aug 21, 2025
Copy link
Member

@paulinus paulinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR seem to have mixed the actual change with other unrelated commits.

Comment on lines +71 to +74
image_ratio = float(max(width, height)) / min(width, height)
is_32 = math.fabs(image_ratio - 3.0 / 2.0)
is_43 = math.fabs(image_ratio - 4.0 / 3.0)
if is_32 < is_43:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wikipedia article mentions a definition that works for any aspect ratio in the "Calculation" section

"Converted focal length into 35 mm camera" = (Diagonal distance of image area in the 35 mm camera (43.27 mm) / Diagonal distance of image area on the image sensor of the DSC) × focal length of the lens of the DSC.

Should we use that instead so that we support any aspect ratio rather than picking between these two?
I guess the question is which definition do camera manufacturers use to fill the EXIF tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but in the end, everywhere else in the code, we use a ratio of focal / width (and not focal / diagonal), so we'll have to specify this width anyway.

@YanNoun YanNoun force-pushed the fix-35mm-conversion branch from 7ac8902 to 2f540d6 Compare August 28, 2025 12:54
@YanNoun
Copy link
Contributor Author

YanNoun commented Aug 28, 2025

Just rebased on master.

@facebook-github-bot
Copy link
Contributor

@paulinus has imported this pull request. If you are a Meta employee, you can view this in D81898826.

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

Successfully merging this pull request may close these issues.

3 participants