hioOiio: remove use of deprecated API to support OpenImageIO v3.0.0.3 #3418

mattyjams · 2024-11-12T04:03:02Z

This fixes up a few usages of long-deprecated OpenImageIO API to allow building against OpenImageIO v3.0.0.3, where those bits of API were finally removed.

This OpenImageIO commit in particular is where the deprecated API was removed:
AcademySoftwareFoundation/OpenImageIO@a4a759f

Checklist

[x] I have created this PR based on the dev branch

[x] I have followed the coding conventions

[ ] I have added unit tests that exercise this functionality (Reference:
testing guidelines)

[x] I have verified that all unit tests pass with the proposed changes

[x] I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)

ImageIOParameter has been a simple alias for ParamValue since at least OpenImageIO 2.0.

mattyjams · 2024-11-12T04:05:52Z

pxr/imaging/plugin/hioOiio/oiioImage.cpp

@@ -713,13 +713,27 @@ HioOIIO_Image::ReadCropped(int const cropTop,
    // If needed, convert double precision images to float
    bool res = false;
    if (imageInput->spec().format == TypeDesc::DOUBLE) {
-        res = imageInput->read_image(TypeDesc::FLOAT,
+        res = imageInput->read_image(
+#if OIIO_VERSION >= 30000


We could probably push this farther back in OIIO versions if we wanted to, as the overload that was being used was implemented this way fairly early in the 2.x series as far as I can tell. For now I opted to restrict it to just handling the build breakage, but let me know if I should try to trace back to an earlier version.

@mattyjams in https://github.com/PixarAnimationStudios/OpenUSD/pull/3365/files @jessey-git has the same-ish changes. I notice you have got imageInput->current*, and @jessey-git has _subimage, and _miplevel instead. @jessey-git hasn't ifdef'd this block. so I'm wondering if we can reconcile these two PRs

should it be imageInput-> or _whatever?

can we still support 2.x and remove the ifdef's?

thanks!

Ahh, sorry for the duplicate PR @meshula! This is indeed very similar to #3365.

In my case here, I followed the implementation of the read_image() overload we had been using as of the most recent 2.x release (now 2.5.17.0) to try to ensure that we were passing through exactly the same code as before, so that's where I got the current_subimage() and current_miplevel() calls from:
https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/v2.5.17.0/src/libOpenImageIO/imageinput.cpp#L894

It may not make a practical difference, but current_subimage() and current_miplevel() are both virtual in ImageInput, so it seems possible that in some exotic case they could be more than just simple accessors?:
https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/v3.0.0.3/src/include/OpenImageIO/imageio.h#L1153

And doing a bit of version spelunking, I think we should be safe to remove the preprocessor stuff if we're committing to only supporting 2.x+. The versioning in OpenImageIO gets a little tricky to follow going back this far, but I think the Release-2.0.0-beta1 tag would be closest to the earliest 2.x version? If that's correct, then the overload of read_image() we'd use with this change is available there such that we don't need to guard this:
https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/Release-2.0.0-beta1/src/include/OpenImageIO/imageio.h#L948

The other nice thing is that the format member of ImageSpec is still a TypeDesc there, so I was also able to collapse the conditional down to just determining the format.

Let me know what you think now! Thanks!

Heya, the reason I decided to use _subimage, _miplevel is that we already use those when making the seek_subimage check on line 691 of the same file. Whichever way we go, can we make both call sites match (I think they should match)?

Actually, this way is probably fine really. You can disregard my post above :)

mattyjams · 2024-11-12T04:10:17Z

pxr/imaging/plugin/hioOiio/oiioImage.cpp

@@ -289,7 +289,7 @@ _FindAttribute(ImageSpec const & spec, std::string const & metadataKey)
    bool convertMatrixTypes = false;
    std::string key = _TranslateMetadataKey(metadataKey, &convertMatrixTypes);

-    ImageIOParameter const * param = spec.find_attribute(key);
+    ParamValue const * param = spec.find_attribute(key);


I did not add any preprocessor checking here, since it looks like the ImageIOParameter alias was in place (and already deprecated) at least as far back as OpenImageIO 2.0:
https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/88df1f55795f79db0c702892fcb311a16509dc04/src/include/OpenImageIO/imageio.h#L106

I'm not sure if anyone is still building against a 1.x version of OpenImageIO, but I can try to distill an OIIO_VERSION to use as a guard here if that's needed.

dgovil · 2024-11-12T04:33:37Z

These look great, Matt.

I think it's fairly unlikely anyone is still using OIIO 1.x while also using a recent version of USD.

Larry deprecated 1.x in 2018, https://lists.aswf.io/g/oiio-dev/topic/openimageio_2_0_has_arrived/99728437

So while I defer to Pixar's judgement, I think it would be fine to assume 2.x as the baseline.

jesschimein · 2024-11-12T17:47:20Z

Filed as internal issue #USD-10426

jesschimein · 2024-11-12T17:47:21Z

/AzurePipelines run

azure-pipelines · 2024-11-12T17:47:33Z

Azure Pipelines successfully started running 1 pipeline(s).

As of OpenImageIO v3.0.0.3, a set of deprecated overloads of ImageInput::read_image() were removed. This change unrolls the overload that was being used into its equivalent in the current API, which was also available in OpenImageIO 2.x.

meshula · 2024-11-23T22:21:45Z

@mattyjams @jessey-git Are you two both fine with resolving a single changelist here? You'll both be listed as authors in the final commit.

jessey-git · 2024-11-25T01:03:27Z

@mattyjams @jessey-git Are you two both fine with resolving a single changelist here? You'll both be listed as authors in the final commit.

That seems fine with me. Is there anything you need me to do?

meshula · 2024-11-25T04:26:39Z

Yes, when you are both happy with the PR here, please leave a comment to that effect :)

mattyjams · 2024-11-25T13:27:46Z

Yes, when you are both happy with the PR here, please leave a comment to that effect :)

Sounds good to me, thanks @meshula! This PR is ready to go from my end.

jessey-git · 2024-11-25T16:44:37Z

I left a comment above in the thread about my choice of _subimage, _miplevel and that would be the only item outstanding on my end for the PR.

jesschimein · 2024-11-25T17:48:54Z

/AzurePipelines run

azure-pipelines · 2024-11-25T17:49:04Z

Azure Pipelines successfully started running 1 pipeline(s).

jessey-git · 2024-11-25T18:21:11Z

I am also ok with this PR. I took another look and this all seems fine.

mattyjams · 2024-11-25T18:33:08Z

I left a comment above in the thread about my choice of _subimage, _miplevel and that would be the only item outstanding on my end for the PR.

Ahh, sorry I missed that @jessey-git!

At that point, _subimage and _miplevel are members of the HioOIIO_Image, and that's where those selections are being propagated into the underlying ImageInput, so we wouldn't be able to use current_subimage() and current_miplevel() there.

Apologies for my ignorance with OpenImageIO, but are there cases where current_subimage() and current_miplevel() might report back something different than what you supplied to ImageInput::seek_subimage()? I would assume for most formats that seek_subimage() would return false if you ask for a subimage or mip level that just doesn't exist in the ImageInput, but is there some interesting format where it might alter your choice such that the accessor functions might return something different than what went into seek_subimage()?

Otherwise, are you saying that you'd prefer we use _subimage and _miplevel in the call to ImageInput::read_image() instead of current_subimage() and current_miplevel()? If there's no possibility we'd ever need to account for a format that behaves as above, then that's fine with me.

jessey-git · 2024-11-25T19:06:39Z

@mattyjams Your understanding is correct and this PR is fine I think. I don't believe, and wouldn't expect, there to be any dissonance between the values passed into seek and what's ultimately returned for the current apis (assuming seek succeeded etc.). This PR is good to go from me too :)

hioOiio: use ParamValue type instead of deprecated ImageIOParameter

29d955f

ImageIOParameter has been a simple alias for ParamValue since at least OpenImageIO 2.0.

mattyjams commented Nov 12, 2024

View reviewed changes

mattyjams force-pushed the pr/support_OpenImageIO_3.0.0.3 branch from 4b919c6 to e3b7185 Compare November 22, 2024 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hioOiio: remove use of deprecated API to support OpenImageIO v3.0.0.3 #3418

hioOiio: remove use of deprecated API to support OpenImageIO v3.0.0.3 #3418

mattyjams commented Nov 12, 2024

mattyjams Nov 12, 2024

meshula Nov 22, 2024

mattyjams Nov 22, 2024

jessey-git Nov 22, 2024

jessey-git Nov 25, 2024

mattyjams Nov 12, 2024

dgovil commented Nov 12, 2024

jesschimein commented Nov 12, 2024

jesschimein commented Nov 12, 2024

azure-pipelines bot commented Nov 12, 2024

meshula commented Nov 23, 2024

jessey-git commented Nov 25, 2024

meshula commented Nov 25, 2024

mattyjams commented Nov 25, 2024

jessey-git commented Nov 25, 2024

jesschimein commented Nov 25, 2024

azure-pipelines bot commented Nov 25, 2024

jessey-git commented Nov 25, 2024

mattyjams commented Nov 25, 2024

jessey-git commented Nov 25, 2024

hioOiio: remove use of deprecated API to support OpenImageIO v3.0.0.3 #3418

Are you sure you want to change the base?

hioOiio: remove use of deprecated API to support OpenImageIO v3.0.0.3 #3418

Conversation

mattyjams commented Nov 12, 2024

Checklist

mattyjams Nov 12, 2024

Choose a reason for hiding this comment

meshula Nov 22, 2024

Choose a reason for hiding this comment

mattyjams Nov 22, 2024

Choose a reason for hiding this comment

jessey-git Nov 22, 2024

Choose a reason for hiding this comment

jessey-git Nov 25, 2024

Choose a reason for hiding this comment

mattyjams Nov 12, 2024

Choose a reason for hiding this comment

dgovil commented Nov 12, 2024

jesschimein commented Nov 12, 2024

jesschimein commented Nov 12, 2024

azure-pipelines bot commented Nov 12, 2024

meshula commented Nov 23, 2024

jessey-git commented Nov 25, 2024

meshula commented Nov 25, 2024

mattyjams commented Nov 25, 2024

jessey-git commented Nov 25, 2024

jesschimein commented Nov 25, 2024

azure-pipelines bot commented Nov 25, 2024

jessey-git commented Nov 25, 2024

mattyjams commented Nov 25, 2024

jessey-git commented Nov 25, 2024