Skip to content

Conversation

@vernalchen
Copy link
Contributor

Description

Fill in OpenEXR lineOrder attribute when parsing the header in OpenEXRCoreInput::PartInfo::parse_header()

Tests

Checklist:

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable. (Check if there is no
    need to update the documentation, for example if this is a bug fix that
    doesn't change the API.)
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • If I added or modified a C++ API call, I have also amended the
    corresponding Python bindings (and if altering ImageBufAlgo functions, also
    exposed the new functionality as oiiotool options).
  • My code follows the prevailing code style of this project. If I haven't
    already run clang-format before submitting, I definitely will look at the CI
    test that runs clang-format and fix anything that it highlights as being
    nonconforming.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 7, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

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.

I think you'll need to make the equivalent change in exrinput.cpp.

Also, in both of those files, there's a line that purports to ignore such lines (just search for "lineorder" and you'll see it), which should probably be removed.

case EXR_ATTR_ENVMAP:
case EXR_ATTR_COMPRESSION:
case EXR_ATTR_CHLIST:
case EXR_ATTR_LINEORDER:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove ignore line for lineOrder

@vernalchen vernalchen requested a review from lgritz February 7, 2025 05:45
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.

Code LGTM, now you just need to fix all the tests so CI passes.

@lgritz
Copy link
Collaborator

lgritz commented Feb 8, 2025

I think the clang-format test caught some nonstandard formatting. Is this easily fixable on your end? If not, I can do it from my end as well.

@vernalchen
Copy link
Contributor Author

I think the clang-format test caught some nonstandard formatting. Is this easily fixable on your end? If not, I can do it from my end as well.

It will be very helpful if you could help fix test failures.

@lgritz
Copy link
Collaborator

lgritz commented Feb 8, 2025

Hi, I'll take care of the test failures. Also, I'm not sure why you changed the name from "openexr:lineOrder" to "lineOrder", but it should have been the first way.

I'm in the middle of this now, please don't update until after I've done so.

@vernalchen
Copy link
Contributor Author

vernalchen commented Feb 8, 2025

Hi, I'll take care of the test failures. Also, I'm not sure why you changed the name from "openexr:lineOrder" to "lineOrder", but it should have been the first way.

I'm in the middle of this now, please don't update until after I've done so.

HI, I have problems running tests locally, so I did the change according to the CI logs, such as below.

comparing out.txt to ref/out.txt
Diff out.txt vs ref/out.txt was:
-------
--- out.txt	Sat Feb  8 01:15:48 2025
+++ ref/out.txt	Sat Feb  8 01:06:08 2025
@@ -142,7 +142,6 @@
 src/tiny-az.exr :     full/display origin: 1, 1
 src/tiny-az.exr :     compression: "zip"
 src/tiny-az.exr :     DateTime: "2022:11:23 15:17:24"
-src/tiny-az.exr :     lineOrder: "increasingY"
 src/tiny-az.exr :     PixelAspectRatio: 1
 src/tiny-az.exr :     screenWindowCenter: 0, 0
 src/tiny-az.exr :     screenWindowWidth: 1
@@ -150,6 +149,7 @@
 src/tiny-az.exr :     Exif:ImageHistory: "oiiotool -pattern constant:color=0.25,0.5,0.75 4x4 3 --origin +2+2 --fullsize 20x20+1+1 -o tiny-az.exr"
 src/tiny-az.exr :     oiio:ColorSpace: "lin_rec709"
 src/tiny-az.exr :     oiio:subimages: 1
+src/tiny-az.exr :     openexr:lineOrder: "increasingY"
 src/tiny-az.exr :     Stats Min: 0.250000 0.500000 0.750000 (float)
     Stats Max: 0.250000 0.500000 0.750000 (float)
     Stats Avg: 0.250000 0.500000 0.750000 (float)

It's glad to hear that you are helping with the test failures, thanks so much!

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.

I fixed up the test output and attribute name. All CI seems to pass now, LGTM!

@lgritz lgritz merged commit 3a9cf59 into AcademySoftwareFoundation:main Feb 8, 2025
28 checks passed
lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request Feb 8, 2025
…ftwareFoundation#4628)

Fill in OpenEXR lineOrder attribute when parsing the header in
OpenEXRCoreInput::PartInfo::parse_header()

---------

Signed-off-by: vernalchen <[email protected]>
scott-wilson pushed a commit to scott-wilson/OpenImageIO that referenced this pull request May 17, 2025
…ftwareFoundation#4628)

Fill in OpenEXR lineOrder attribute when parsing the header in
OpenEXRCoreInput::PartInfo::parse_header()

---------

Signed-off-by: vernalchen <[email protected]>
Signed-off-by: Scott Wilson <[email protected]>
scott-wilson pushed a commit to scott-wilson/OpenImageIO that referenced this pull request May 18, 2025
…ftwareFoundation#4628)

Fill in OpenEXR lineOrder attribute when parsing the header in
OpenEXRCoreInput::PartInfo::parse_header()

---------

Signed-off-by: vernalchen <[email protected]>
Signed-off-by: Scott Wilson <[email protected]>
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