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

[FEATURE REQUEST] OIIO::ImageBufAlgo::make_texture doesn't take an nthreads argument #4254

Open
etheory opened this issue May 3, 2024 · 2 comments
Assignees
Labels
bug Crash or wrong behavior of an existing feature. enhancement Improvement of existing/working features. feature request good first issue Good one-day project for beginners without much knowledge of the code base.

Comments

@etheory
Copy link
Contributor

etheory commented May 3, 2024

https://openimageio.readthedocs.io/en/stable/imagebufalgo.html says:
All ImageBufAlgo functions take an optional nthreads parameter that signifies the maximum number of threads to use to parallelize the operation. The default value for nthreads is 0, which signifies that the number of thread should be the OIIO global default set by OIIO::attribute() (see Section [Global Attributes](https://openimageio.readthedocs.io/en/stable/imageioapi.html#sec-globalattribs)), which itself defaults to be the detected level of hardware concurrency (number of cores available).

But this is not true for make_texture. In the renderer my company uses, we do texture conversions automatically on scene load, concurrently with using multiple OpenImageIO clients via the shared TextureSystem. As such, we are beholden to whatever another client may have set nthreads to globally when calling make_texture.

How hard would it be to add an nthreads argument to make_texture, and why doesn't it already have one when the docs say it should do?

Thanks!

Is your feature request related to a problem? Please describe.
Texture conversion thread usage being separable from other client usage of OIIO.

Describe the solution you'd like
An nthreads argument for make_texture.

Describe alternatives you've considered
Running maketx from the command line with an nthreads comment in a sys shell from C++ (urgh, no thanks).

Additional context
I could make a PR, but before I do that, I'm wondering if there was a good reason this wasn't the case already, to prevent me "finding out the hard way".

@lgritz
Copy link
Collaborator

lgritz commented May 3, 2024

I think it's just an oversight.

If I had to guess about the origin of this, I'd say that probably some version of make_texture predated IBA, or at least predated the addition of the nthreads parameter nearly universally in IBA functions. At some point, we moved it to imagebufalgo.h and recategorized it conceptually as an IBA function, but failed to make it conform to the usual calling convention for IBA, which is to have the last argument be nthreads.

Please do feel free to fix this in a PR, I think it's obviously the right thing to do.

@etheory
Copy link
Contributor Author

etheory commented May 4, 2024

Thanks @lgritz, I'll give it a crack.

@etheory etheory self-assigned this May 4, 2024
@etheory etheory added bug Crash or wrong behavior of an existing feature. enhancement Improvement of existing/working features. feature request good first issue Good one-day project for beginners without much knowledge of the code base. labels May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Crash or wrong behavior of an existing feature. enhancement Improvement of existing/working features. feature request good first issue Good one-day project for beginners without much knowledge of the code base.
Projects
None yet
Development

No branches or pull requests

2 participants