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

support for returning combined channel list of multi-part files #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tiltX
Copy link

@tiltX tiltX commented Nov 18, 2021

Currently, read_exr_header() will only read the first header of a multi-part OpenEXR file which results in an incomplete channel list. This patch adds a mechanism to read all headers separated by null bytes if the exr's flags indicate that a multi-part file is being processed. The returned dict will contain a "channels" attribute that is a combination of all channels across all the parts.

This isn't full multi-part support since according to the OpenEXR standard, the same attribute may have different values in different parts of the file (the part's "name" attribute being the most obvious one). But supporting this would require much more profound changes to the returned dict.

@vvzen
Copy link
Owner

vvzen commented Nov 20, 2021

Thanks a lot for your contribution, @tiltX !
I will have a deeper read later, but at a first look I feel that it might be very useful to actually store the multipart information so that it's and easy to understand that we're actually dealing with a multipart exr. I would be a quite radical change yes, but it would be quite useful and it would improve some of it the naive decisions of the original design.
Maybe instead of returning a dictionary we can return a list of dictionaries? Or instead, we could return a dictionary with keys "0", "1", etc. to indicate the different parts.
How do you feel about that?

Thanks!

vv

@tiltX
Copy link
Author

tiltX commented Nov 23, 2021

Yes, I've shied away from more disruptive changes that would be necessary for full multi-part support. My use-case was just to know which passes there are in a rendered EXR.

I've played around with your suggestion of using a list of dicts with numeric keys. That seems more suitable than a list although I don't know how often it happens in real life that attributes are present in several parts but not all of them. The multi-part exrs I've encountered had most of their metadata in the first part and every subsequent part just had the bare minimum of attributes (name, display windows, ...).

I'm currently testing the following method: if the header indicates a multi-part exr every attribute's value will be a dict with numeric keys instead of the value itself.

One could also iterate over this result and turn every dict that only has one element into that one value. This way the returned dict still allows direct access to all metadata via its keys and it would also allow reading most metadata straight from the dictionary:

{
    "name": {0: "diffuse", 1: "reflection", 2: "refraction"},
    'pixelAspectRatio': 1.0,
    ....
}

An alternative could be to return a custom data class derived from dict that allows naive access to all metadata keys regardless of whether it's a multi-part or single part EXR (by just reading the first part's value). If the calling code knows how to deal with multi-part data and is interested in it, there could be a custom getter function that returns all values for an attribute or the value of a specific part:

meta["name"]          # returns "diffuse"
meta.get("name")      # returns {0: "diffuse", 1: "reflection", 2: "refraction"}
meta.get("name", 2)   # returns "refraction

@vvzen
Copy link
Owner

vvzen commented Jan 3, 2022

Hi @tiltX !
I've recently added automatic tests to be run via GitHub actions on the opening (and updating) of PRs.
It seems that the current changes in your branch are passing the tests for python2.7, but not in 3.7 . Do you mind having a look?

Regarding the 2 alternatives you were suggesting, I feel that at this point your second proposal might be better. We might just have reached the time for us to wrap everything into a simple custom class that makes it easy to query metadata for both multipart and single part EXRs.
The internal data structure used by the class could be the dictionary that you proposed:

every attribute's value will be a dict with numeric keys instead of the value itself

but that would be an implementation detail.

For the API, I think this would be a perfect case of starting with a TDD workflow and first model how the API would be used, then use that to drive the implementation. Maybe something like this could make sense (the >>> notation is to simulate an interactive prompt, so statements without a >>> are the return of the previous code snippet) :

# For a moment, lets ignore the fact that in practice I don't think anyone would have
# a multipart image with different Aspect Ratios per part

# Return all parts
>>>  metadata['pixelAspectRatio']
(1, 2, 1)
>>>  metadata.get('pixelAspectRatio')
(1, 2, 1)
# Specify which part to retrieve (does it make sense to start from 0 ?)
>>>  metadata.get_part('pixelAspectRatio', part=0)
1
>>>  metadata.get_part('pixelAspectRatio', 0)
1
>>>  metadata.get_part('pixelAspectRatio', part=1)
2
>>>  metadata.get_part('pixelAspectRatio', part=2)
1

if we have a single part, the API would act mostly similar, but throw errors if you try to access a part != 0.

# Return all parts (in this case, we have a single part EXR
>>>  metadata['pixelAspectRatio']
(1,)
>>>  metadata.get('pixelAspectRatio')
(1,)
# Specify which part to retrieve (does it make sense to start from 0 ?)
>>>  metadata.get_part('pixelAspectRatio', part=0)
1
>>>  metadata.get_part('pixelAspectRatio', part=1)
ValueError: EXR file has no part 1
>>>  metadata.get_part('pixelAspectRatio', part=2)
ValueError: EXR file has no part 2

Thanks!

@tiltX
Copy link
Author

tiltX commented Jan 4, 2022

Hi, yeah I've noticed. It might have to do with dict order. I'll take a look although I've mainly just updated the pull request to get the hang of the corresponding github workflow.

I don't know if/when I'll find the time to explore your proposed API (but I think it looks good).
So feel free to close this request, maybe this thread should be an issue instead?

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.

2 participants