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

Features/MultiThread_suite_V2 #59

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

revisionfx
Copy link
Contributor

Update the MultiThread suite to V2. Request is to clean up suites that have ambiguities in terms of what is implemented. All mutext calls in V2 are removed. Plug-in would need to use OS calls for that if needed.

Description

Plug-in can check if V2 is supported by host, if not revert to V1 or ignore.
This suite allows hosts to manage a thread pool and fight back against over-thread subscription on multi-core systems.

Suite Cleanup, create V2 of suite removing all references to mutext. Locking to be done with OS calls if needed.
Update the MultiThread suite to V2. Request is to clean up suites that have ambiguities in terms of what is implemented. All mutext calls in V2 are removed. Plug-in would need to use OS calls for that if needed.
@revisionfx
Copy link
Contributor Author

There is only a single file, initially copied in wrong folder and deleted

@garyo
Copy link
Contributor

garyo commented Mar 30, 2024

Just reviewing old PRs... I see the version of include/ofxMultiThread.h here has diff markers in it so it's clearly incorrect. If you'd like to send a proper version of what your intent is (just a clean updated version of that file for instance), I can create a new PR or update this one.

@revisionfx
Copy link
Contributor Author

Not sure what you mean by different marker. You mean when you switched from Features to Feature/ :)
It was simply suggested that Mutex should not be handled by suite.
Right now we use that suite or not on a per host basis but don't use Mutex wrappers. Haven't scanned host by host to see what they respond to these days...

A bigger issue for us is with hosts that can render multiple frames at once - we have kOfxImageEffectPluginRenderThreadSafety for that. For example host might not support the OpenGL suite but we can still render on GPU from CPU images, however we had issues in the past with nVidia driver in such case in some host spawning a bunch of multiple frames render at once (making this real slow - the warp engine loops and can't find a slot) so we are not then [kOfxImageEffectRenderFullySafe] - we are though if a user sets our plugin to render on CPU... Definition is only a Property Set - plugin descriptor (read/write) - does not say if one can change in Instance Changed... and have not tested. Alternatively in the GPU additional WIP features it was suggested we have a parallel interface for I use GPU and which one.

(https://openfx.readthedocs.io/en/latest/Reference/ofxRendering.html#ofxImageEffect_8h_1a9dd536b70cb80adac6bf6e67cb9e50b0)

https://openfx.readthedocs.io/en/latest/Reference/DoxygenIndex.html#c.kOfxImageEffectPluginRenderThreadSafety does support

@garyo
Copy link
Contributor

garyo commented Mar 30, 2024

No, I mean the file itself has "diff markers" in it. It would not compile as is. You replaced the actual file with something corrupted. The first line of include/ofxMultiThread.h is now this literal text:

<<<<<<< HEAD

@garyo
Copy link
Contributor

garyo commented Mar 30, 2024

As per your other points, I suggest you take that up with a host vendor who is willing to implement their side of it, and see where that goes. I wouldn't post it in a PR until you have that.

I couldn't read the changes in this PR because of the file corruption, so I don't really know what it contains. Maybe if you can send me an un-corrupted version of what you intend the header file to look like, I can clean up this branch with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants