api(deepdata): change merge_deep_pixels return type, add OIIO_NODISCARD_ERROR

[luna-y-kim]

merge_deep_pixels() calls copy_deep_sample(), which has an error status return, so change merge_deep_pixels()'s return type from void to bool.

For OIIO_NODISCARD_ERROR, all internal calls that previously ignored return values are handled:

• OIIO_CONTRACT_ASSERT where the call can't fail in context, or the caller is a void function or a constructor.
• return false where the caller already returns a bool error state.
• (void) cast for the deprecated internal overload.

Follow-up to #5252
Part of #4781

Tests

N/A

Checklist:

• I have read the guidelines on contributions and code review procedures.
• I have read the Policy on AI Coding Assistants
  and if I used AI coding assistants, I have an Assisted-by: TOOL / MODEL
  line in the pull request description above.
• (N/A) I have updated the documentation if my PR adds features or changes
  behavior.
• (N/A) I am sure that this PR's changes are tested in the testsuite.
• I have run and passed the testsuite in CI before submitting the
  PR, by pushing the changes to my fork and seeing that the automated CI
  passed there. (Exceptions: If most tests pass and you can't figure out why
  the remaining ones fail, it's ok to submit the PR and ask for help. Or if
  any failures seem entirely unrelated to your change; sometimes things break
  on the GitHub runners.)
• My code follows the prevailing code style of this project and I
  fixed any problems reported by the clang-format CI test.
• If I added or modified a public C++ API call, I have also amended the
  corresponding Python bindings. If altering ImageBufAlgo functions, I also
  exposed the new functionality as oiiotool options.

[lgritz]

I feel like OIIO_CONTRACT_ASSERT is the right choice for those DeepData::DeepData constructors, since not much can go wrong and they can't return an error anyway.

But in this case, DD::split() does return a bool, so maybe if copy_deep_sample fails, just return false here instead of asserting?

And similar to the other assertions inside functions that return an error status bool.

[luna-y-kim]

I kept going in circles, and I keep coming back to return false not quite fitting here. (I might be overthinking it.) I'd like to hear your thoughts about this.

DD::split()'s docs say it returns "true if any splits occurred, false if the pixel was not modified." So false is a normal "nothing to split" result, not an error, which is also why I couldn't put OIIO_NODISCARD_ERROR on it.

And by the time DD::copy_deep_sample() is called, we're mid-loop with the pixel already partially modified (even for the first round because DD::insert_samples() ran just before, which bumps up m_nsamples[pixel]). Having return false there would go against the documented behavior.

Also, the false branch in DD::copy_deep_sample() is unreachable at this point anyways (src is *this, so the number of channels is equal, and the zf/zb guard already rules out a null data pointer).

p.s. I thought about the idea of flipping split() to return a real success/failure bool, but that would change behavior for the caller that relies on "true = a split occurred", so it seems too risky.

[lgritz]

Aha, you are right, this is a tricky case.

It's not an error status, since asking it to split any samples that span the z value might have had a sample that needs to be split, but might not, and neither of those is an error.

But also, it's not the kind of "pure function" result (like the return value from sin(x), or the "essential" return of ImageInput::create()) where the only conceivable reason to call the function is for the return value, so it surely indicates an error if you don't use that result.

For DD::split(), the primary rationale is that you want the sample, if it exists, to be split. The return value provides the secondary service of telling you whether it needed to split anything or not, but it's easy to imagine that some callers don't need to know, they just want to ensure that no samples span that z value.

So I say: this function should have NEITHER annotation. It's actually OKAY to ignore the return value, and that does not indicate any bug or mis-programming.

[lgritz]

here, too

[luna-y-kim]

This one makes sense. I'll update it!

[luna-y-kim]

Separately (slightly off topic): DD::merge_deep_pixels() returns void, but its doc comment says it returns a bool error state. Which was intended (void or bool)?

[luna-y-kim]

I forgot to rebase, so I re-pushed. Sorry for the noise.

[lgritz]

Separately (slightly off topic): DD::merge_deep_pixels() returns void, but its doc comment says it returns a bool error state. Which was intended (void or bool)?

Good catch!

It calls copy_deep_sample, which does have an error status return, so I think we should change the new merge_deep_pixels() to to return bool and bubble that up.

In this one case it is safe to change the return value, only because we already added the new function signature, and it's only in main, and has not been backported to any release branches. So we lucked out again, and we are free for you to make this change. It would not be able to get backported (at least not separately from #5252), since C++ can't overload based on return type alone.

Would you mind quickly making that change as part of this PR, and we can merge and make it right? Thanks.

[luna-y-kim]

On it! I'll push shortly.

[luna-y-kim]

I couldn't get make pystubs to run locally, and the CI stub check is disabled, so I left it as is for now. Would that be ok?

(src/python/stubs/OpenImageIO/__init__.pyi:266)

def merge_deep_pixels(self, pixel: typing.SupportsInt, src: DeepData, srcpixel: typing.SupportsInt) -> None: ...

[lgritz]

Yeah, that's ok. Lots of us have trouble running it locally. Sometimes the change is obvious how we do it by hand, and other times, we just let it fail and regenerate in CI, and then the download artifact from that CI run has the new version of the file.