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

Add sensorCenterOffset, sensorOverallDimensions, sensorPhotositePitch… #1489

Merged

Conversation

JGoldstone
Copy link
Contributor

… and sensorAcquisitionRectangle optional standard attributes

Also removed a redundant and non-convention-following IMATH_NAMESPACE:: from the data type of the cameraColorBalance optional standard attribute.

… and sensorAcquisitionRectangle optional standard attributes

Signed-off-by: Joseph Goldstone <jgoldstone@arri.com>
Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

lgtm, and addresses my struggle with understanding the meaning of photosite :)

@JGoldstone
Copy link
Contributor Author

Cary asked me the other day to update the documentation for the optional standard attributes. That really should be done in this pull request for the sensor* additions and I might as well get the doc in for the others as well. Can we hold off on actually merging this for 2-3 days?

…orPhotositePitch, and sensorAcquisitionRectangle to doc page for standard optional attributes

Signed-off-by: Joseph Goldstone <jgoldstone@arri.com>
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM, just need blank lines around the list entries in the .rst to make sphinx happy.

…rd optional attributes doc

Signed-off-by: Joseph Goldstone <jgoldstone@arri.com>
@JGoldstone
Copy link
Contributor Author

@cary-ilm , does that address your problem? It's saying "all checks have passed", presumably after having built everything, including documentation. And is there a way for me to see build artifacts so that, e.g., I can actually look at the text on the now-modified page?

@cary-ilm
Copy link
Member

@JGoldstone, yes, that worked, thanks. You can validate yourself by building with BUILD_DOCS=ON, which will generate the html for the website, then you can open the html in your local browser, although it's sort of a pain because you first have to make sure sphinx and other requirements are installed, described here.

Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@cary-ilm cary-ilm merged commit ae32ed6 into AcademySoftwareFoundation:main Jul 18, 2023
@JGoldstone JGoldstone deleted the feature/sensor_attributes branch July 18, 2023 19:49
@@ -65,6 +65,10 @@ OPENEXR_IMF_INTERNAL_NAMESPACE_SOURCE_ENTER
IMF_STD_ATTRIBUTE_IMP (originalDataWindow, OriginalDataWindow, Box2i)
IMF_STD_ATTRIBUTE_IMP (worldToCamera, WorldToCamera, M44f)
IMF_STD_ATTRIBUTE_IMP (worldToNDC, WorldToNDC, M44f)
IMF_STD_ATTRIBUTE_IMP (sensorCenterOffset, SensorCenterOffset, V2f)
IMF_STD_ATTRIBUTE_IMP (sensorOverallDimensions, SensorOverallDimensions, V2f)
IMF_STD_ATTRIBUTE_IMP (sensorPhotositePitch, SensorPhotositePitch, float)
Copy link

@kmilos kmilos Jul 19, 2023

Choose a reason for hiding this comment

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

Although it is most common, perhaps there are cases where the pitch is not identical horizontally and vertically? Or there is even perhaps no simple horizontal/vertical definition as in the case of slanted/irregular/non-square grid arrays like Fujifilm Super CCD (and more modern small/large pixel HDR equivalents), or Fujifilm EXR, and other novel patterns?

Perhaps a added note that this is applicable only to a Cartesian (square) grid of pixels is at least in order here if there is no easy way to make this more general?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add a note when I go back in to add content on the .rst page for CCT and tint and all that.

In all the SMPTE RIS discussions on metadata for camera tracking, no one has ever asked to support, e.g., the sensor on the Sony F65, which is at a 45º angle. And the last time I saw non-square pixels in acquisition was pre-Rec. 709 HDTV cameras (the Wikipedia article for "Grand Alliance (HDTV)" supports this).

While I understand that OpenEXR use is not limited to the entertainment industry, AFAIK it is by far the dominant use case, and we should not let the perfect be the enemy of the good.

Copy link

Choose a reason for hiding this comment

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

While I understand that OpenEXR use is not limited to the entertainment industry, AFAIK it is by far the dominant use case, and we should not let the perfect be the enemy of the good.

Of course. Listing the known assumptions/"limitations" is part of this good practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's really needed a sensorPhotositeAspectRatio can be added later, I suppose.

Perhaps more relevant is clarifying what to do about anamorphic lenses. In that case photosites don't have the same field of view horizontally and vertically due to the lens optics, rather than their dimensions. pixelAspectRatio defines how the image should be displayed, not what the camera lens actually did, so is generally only indicative of how much anamorphic squeeze happened at capture time. It seems like all the lens attributes assume non-anamorphic lenses

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

Successfully merging this pull request may close these issues.

5 participants