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

Website "Attributes" page now describes all attributes #1511

Merged
merged 4 commits into from
Aug 14, 2023

Conversation

cary-ilm
Copy link
Member

This gathers all the attribute definitions onto a single page, both required and optional, and organizes them into logical groups. It also documents each attribute's type. The file is called StandardAttributes.rst now instead of StandardOptionalAttributes.rst. No C++ source changed, just the website documentation.

Preview the page here: https://cary-ilm-openexr.readthedocs.io/en/website-attributes/StandardAttributes.html

This also fixes some minor unrelated documentation formatting errors.

This gathers all the attribute definitions onto a single page, both
required and optional, and organizes them into logical groups. It also
documents each attribute's type.

Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>
<td style="vertical-align: top; width:100px"> required </td>
</tr>
<tr>
<td style="vertical-align: top; width:150px"> <tt> <b> originalDataView </b> </tt> </td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be originalDataWindow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, fixed.

<td style="vertical-align: top; width:500px">
<p style="padding-bottom:15px">
Indicates the number of chunks in this part. Required if the
multipart bit (12) is set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also worth adding that it is created automatically by the library: the user does not need to compute it

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

<td style="vertical-align: top; width:150px; font-weight:bold"> attribute name</td>
<td style="vertical-align: top; width:100px; font-weight:bold"> type </td>
<td style="vertical-align: top; width:500px; font-weight:bold"> definition </td>
</tr>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the missing 'status' column deliberate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped the "status" column where everything is optional, because the stupid sphinx formatting limits with width of the table and the long definitions can use all the width they can get. I mention in the sentence above the tables that they're all optional. If it's confusing, it's easy to add the column back in. I just preferred to reclaim the extra width in place off an "optional" on every line.

I'm totally open to redoing this entirely, but we definitely need something to distinguish required vs. optional attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided it would work better without the "status" column, and I added required underneath the attribute name where appropriate. How does that look?

</tr>

<tr>
<td style="vertical-align: top; width:150px"> <tt> <b> longitude lattitude altitude </b> </tt>
Copy link
Contributor

Choose a reason for hiding this comment

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

typo for latitude?
This might be clearer to do as three separate attribute/type pairs, repeating 'float' each time, and having the definition with a rowspan="3" to make it common to all three. That would also clarify that latitude and longitude are stored as a singe float for degrees, not with minutes and seconds

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that idea, I'll fiddle with it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured out the rowspan, works much better, thanks. I also replaced the other instances where multiple attributes have a grouped definition.

</p>
<p style="padding-bottom:15px">
Uniqueness could be accomplished with, e.g., a MAC address, a
concatenation of cameraMake, cameraModel, cameraSerialNumber, etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

The string can have arbitrary format, right? It doesn't need to follow the UUID 128 bit string format, even though that's implied by the name? It may be worth clarifying

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

attribute is consistent with the actual state of the pixels.
Application software may assume that the attribute is valid, as long
as the software will not crash or lock up if any pixels are
inconsistent with the <tt>deepImageState</tt> attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to link to InterpretingDeepPixels here? That defines the state of the library and describes how to convert image states

Copy link
Member Author

Choose a reason for hiding this comment

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

Formatting of sphinx-based tables is really limited, and I couldn't get a layout I was happy with, so I switched to embedded html. Unfortunately, I'm not sure how to embed a link to the sphinx-generated content from inside the raw html. I'll look into it later.

Stores the maximum number of samples used by any single pixel within a
deep image. If this number is small, it may be appropriate to read the
deep image into a fix-sized buffer for processing. However, this number
may be very large.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add that the library never enforced the correctness of this value, so if it shows up in legacy files it should not be trusted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>
@cary-ilm
Copy link
Member Author

Updated preview is here: file:///home/cary/builds/src/cary-ilm/openexr/website-attributes-Release/website/sphinx/StandardAttributes.html

Copy link
Contributor

@peterhillman peterhillman left a comment

Choose a reason for hiding this comment

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

Looks good!

@cary-ilm cary-ilm merged commit 69b1297 into AcademySoftwareFoundation:main Aug 14, 2023
27 checks passed
@JGoldstone
Copy link
Contributor

Late to the game as usual, I notice that in the description of screenWindowCenter, we have "not be able so generate" when we should have "not be able to generate".

@JGoldstone
Copy link
Contributor

Neither in this file nor in ImfStandardAttributes.h do we define the point on the z-axis from which we measure focus distance. The measurement should be from the image plane. For the digital cinema cameras (and I would think DSLRs, etc as well), this is neither the front surface of "the sensor" nor the front of the photosite layer but is somewhere in-between. Back to front I think the order is photosite array, CFA material, micro-lens array (so that one doesn't lose photos that otherwise would hit the interstitial, non-photosensitive areas), and then either IR and then OLPF (Optical Low-Pass Filter) or OLPF and then IR. Is it worth trying to get more clarity on this?

@kmilos
Copy link

kmilos commented Sep 11, 2023

Can someone please advise which of the three would be the equivalent of the Exif FocalLength (see CIPA DCG-001 and ISO 517):

nominalFocalLength
pinholeFocalLength
effectiveFocalLength

@JGoldstone
Copy link
Contributor

JGoldstone commented Sep 11, 2023 via email

@kmilos
Copy link

kmilos commented Sep 11, 2023

From the definition here (in section “Tags used buy Exif SubIFD”) it sounds to me as if the corresponding tag is nominalFocalLength.

Thanks for confirming - I was suspecting as much since the definition says "focused at infinity" rather than exact focus at shot.

@cary-ilm
Copy link
Member Author

@JGoldstone, where's the "here" you're referring to?

@lgritz
Copy link
Contributor

lgritz commented Sep 11, 2023

@JGoldstone While we're both here, I'm going to give my headache about this topic to you.

If an application that may convert between formats reads a JPEG or camera raw image containing (to pick an example) Exif FocalLength and is asked to convert to an OpenEXR image, what metadata should it write to the exr file?

  1. Write the Exif FocalLength as exr "nominalFocalLength" because we think that's the approved exr metadata name for the same thing?
  2. Write it as "Exif:FolcalLength" to keep it clear that it came originally from Exif data, and don't set nominalFocalLength even though it's the same concept?
  3. Write both? And what should you do if you come across a file that has both, but they contain different values? Which one "wins" when you really need to know the focal length?

@JGoldstone
Copy link
Contributor

@cary-ilm my bad for not copying the URL: https://www.media.mit.edu/pia/Research/deepview/exif.html

@lgritz sorry to make your headache worse but say you do a merge where two inputs are abutted (so there are no mixed pixels) and then the result of the merge is written out; and suppose that the two inputs have different focal lengths. What is the focal length of the merged result?

@lgritz
Copy link
Contributor

lgritz commented Sep 11, 2023

@JGoldstone Yeah, I have that headache, too, but didn't want to burden you with it yet. There's a general problem of what to do with conflicting metadata any time you combine images.

There's no correct answer, I think we just need a policy/heuristic that everybody can understand. Some possibilities include (I'm just throwing out possibilities that may or may not be mutually exclusive):

  • As soon as you alter pixel values in any way, camera-related metadata should be understood as historical information about the original capture and do not describe any computed result image.
  • When combining multiple images, "first one" wins.
  • When combining multiple images, camera metadata should be discarded.
  • When combining multiple images, somehow save them all? (I'm sorry, I think it was a colossal mistake for the SMPTE metadata to demand never to be namespaced; if they had, we could also add use namespacing to document the camera data from multiple source images in a file.)

In the more pedantic world of my dreams, our "specification" documenting standard metadata would say for each one:

  • Is it describing THIS image, or documenting a historical source image?
  • Should it be invalidated if pixel values change?
  • What should happen if multiple images are combined into one? (Possibilities might include invalidate, concatenate, store all separately, first one wins, error if they don't match, etc.)
  • If this metadata is altered, are there other metadata that should be invalidated?

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