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 the raw:flip attribute #2572

Merged
merged 2 commits into from May 5, 2020
Merged

Add the raw:flip attribute #2572

merged 2 commits into from May 5, 2020

Conversation

ghost
Copy link

@ghost ghost commented Apr 29, 2020

Description

Adding the "raw:flip" attribute.

This field is set by LibRaw when the image has been rotated internally.
From my understanding, this is an equivalent to the exif:orientation (Don't know why it is not in this exif).

Tests

I don't think a test is needed as it is only an export of a known LibRaw structure's field.

Checklist:

  • I have read the contribution guidelines.
  • If this is more extensive than a small change to existing code, I
    have previously submitted a Contributor License Agreement
    (individual, and if there is any way my
    employers might think my programming belongs to them, then also
    corporate).
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • [ X My code follows the prevailing code style of this project.

@lgritz
Copy link
Collaborator

lgritz commented Apr 29, 2020

Most of the CI tests are failing, because all the raw-related tests now have additional "raw:flip" data in their metadata printing output. Maybe you should only set the attribute if the field is set to a non-default value?

But I'm also wondering if instead of returning this value as a separate field -- which would go unrecognized by any app that wasn't intentionally looking for it -- maybe a better approach would be to simply incorporate it into the "Orientation" that is reported for the image? In other words, the raw file may be in some sense have both the exif orientation and the flip value, and the combination of those tells you how to view the image, but from OIIO's perspective it can fold both into the orientation and then any app that already responds to orientation will automatically also understand the raw flip.

@ghost
Copy link
Author

ghost commented Apr 29, 2020

Definitely a good idea to merge it with orientation. However i am not confident currently with the "universal" meaning of the flip values. I only have Canon cameras and their values seems very similar to the exif orientation possible values.

@lgritz
Copy link
Collaborator

lgritz commented Apr 29, 2020

Usually with libraw, if the field is not in one of the "maker" structures, it tends to mean the same thing for all cameras.

@lgritz
Copy link
Collaborator

lgritz commented Apr 29, 2020

https://www.libraw.org/docs/API-datastruct-eng.html That says it's the same as orientation? Or does it override orientation?

https://www.libraw.org/node/2445 (read the whole thread) that's also a little alarming, it may be the 1-8 orientation tag, or it may be a degree value?

@ghost
Copy link
Author

ghost commented Apr 30, 2020

@lgritz
Copy link
Collaborator

lgritz commented Apr 30, 2020

OK, so our code should look at the flip value, if it's a multiple of 90 then we convert to a proper orientation. And then... put the results into Orientation, overriding any Exif value?

@ghost
Copy link
Author

ghost commented May 5, 2020

I am not sure you're right.

For me, orientation describe a rotation to be done by the viewer. That is, the array described by the image is still the "sensor array".

Flip describe a rotation already applied to the image. The image is not to be done by the viewer. However, it may still be a valuable information for some applications.

@lgritz
Copy link
Collaborator

lgritz commented May 5, 2020

OK, in that case, then let's go ahead and set "raw:flip" without altering "Orientation." But I think you should only set the flip attribute when it's a non-default value. There's no need to clutter the metadata (and alter all the unit test reference output) for an attribute that is almost never used, except in cases where it is.

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM. I will push a separate fix that updates the testsuite reference output.

@lgritz lgritz merged commit 61327af into AcademySoftwareFoundation:master May 5, 2020
lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request May 5, 2020
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.

None yet

2 participants