AcademySoftwareFoundation / openfx

OpenFX effects API
Other
410 stars 120 forks source link

Clarify why OfxMemorySuiteV1 should be used #126

Closed emil-e closed 8 months ago

emil-e commented 1 year ago

Problem

In https://openfx.readthedocs.io/en/master/Reference/suites/ofxMemorySuiteReference.html:

Use this suite for ordinary memory management functions, where you would normally use malloc/free or new/delete on ordinary objects.

It is not described why this suite should be used in the first place and why you cannot simply use malloc/free/new/delete as normal. Is it mandatory to use this suite for allocation? Can I not use standard allocators? Will I have poor performance? Will it crash on some hosts?

I can see that the support library in some cases uses std::string with the default allocator for some data, for example.

So what's the idea here? I'm very confused.

garyo commented 1 year ago

This allows the host more visibility into plugin memory usage. It is not mandatory. For instance the host may pre-allocate and manage large image buffers, and give those to the plugins faster than system malloc. But in practice, many hosts just wrap malloc/free with these calls.

emil-e commented 1 year ago

@garyo That makes sense. I don't see it as a big hassle to use imageMemoryAlloc for image buffers since it's easy to have control over that, but for randoms strings, vectors, third party libraries and so on, it's hard or impossible to pipe all those allocations back to the host.

I also see that the support library manages the suites as global variables rather than storing the suites per plugin: https://github.com/AcademySoftwareFoundation/openfx/blob/95a1179ac20357f165d89906ad1f49acc89fcede/Support/Library/ofxsImageEffect.cpp#L118

Is this really safe? Is is guaranteed that same suite would be returned for each individual plugin in the same library? Each plugin gets their own OfxHost through setHost and thus have their own fetchSuite pointer. The host could send different suites to different plugins so is this really safe?

garyo commented 1 year ago

Both of these questions would be excellent clarifications to the spec, which is defined by the doc comments in the source. No host would require all allocations like strings and vectors to go through the OpenFX API. You may get better performance, for instance the host may return pinned physical memory, and may avoid certain low-memory situations where the host has pre-allocated most of the physical RAM.

As for the suites, we could add a clarification to the spec that the host must return the same suite pointers to all plugins in the same shared lib. I'm pretty sure all existing hosts do that (it's not hard for the host to switch different behavior within a suite on a per-plugin basis if needed) and plugins I've seen store them as single globals, but as you point out nothing in the spec currently precludes them from doing that.

Feel free to add some clarifying text to the appropriate places in the spec if you like!

barretpj commented 1 year ago

On Mon, 24 Jul 2023 at 14:12, Gary Oberbrunner @.***> wrote:

As for the suites, we could add a clarification to the spec that the host must return the same suite pointers to all plugins in the same shared lib. I'm pretty sure all existing hosts do that (it's not hard for the host to switch different behavior within a suite on a per-plugin basis if needed) and plugins I've seen store them as single globals, but as you point out nothing in the spec currently precludes them from doing that.

Feel free to add some clarifying text to the appropriate places in the spec if you like!

That's a bit risky isn't it? Anything that changes the spec, even "clarifying" one interpretation of an ambiguous or unspecified part of the spec, risks making any hosts/plugins which do things differently (and perfectly within the current spec) suddenly becoming noncomformant.

Spec changes of any sort must go through the proper channels.

Phil

Message ID: @.*** .com>

garyo commented 1 year ago

That's a fair point, Phil. Even if we think everyone does it that way today there is a risk. Perhaps we could add this as "Recommended: hosts should return the same suite pointers to all plugins in the same shared lib" rather than a strict requirement. That should help future hosts behave nicely, and also serves to alert plugins that they may want to handle the case where a host behaves differently. What do you think of that?

barretpj commented 1 year ago

Hosts can't tell which plugin is asking for a suite pointer through fetchSuite, unless they've been given separate OfxHost pointers. So maybe this is more about recommending that the same OfxHost pointer should be passed in the calls to setHost() for every plugin in a given binary? But the comment on OfxSetHost() says "The host pointer [passed to OfxSetHost] is only assumed valid until OfxGetPlugin where it might get reset" ... which implies that each plugin might get a different one. So maybe this is (as originally suggested) an issue with the published support library using globals and making the assumption that all plugins will get the same host and therefore the same suites.

On Thu, 3 Aug 2023 at 15:26, Gary Oberbrunner @.***> wrote:

That's a fair point, Phil. Even if we think everyone does it that way today there is a risk. Perhaps we could add this as "Recommended: hosts should return the same suite pointers to all plugins in the same shared lib" rather than a strict requirement. That should help future hosts behave nicely, and also serves to alert plugins that they may want to handle the case where a host behaves differently. What do you think of that?

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/openfx/issues/126#issuecomment-1664086669, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADSELPJPSVD4RUQPHHEGELXTOYJBANCNFSM6AAAAAA2VJFLIE . You are receiving this because you commented.Message ID: @.***>

emil-e commented 1 year ago

From someone just starting out with OpenFX, I never made any assumptions about suite pointers being the same across plugins nor that they would all receive the same host pointer. The fact that each plugin declares its own setHost function pointer is an indication that it could be different, otherwise it would be a top-level global exported function next to OfxGetPlugin. Looking only at these things, it was very surprising to be to see the implementation of the support library using global vars for this - it felt instinctively incorrect.

However, if one wants to redirect normal allocations in one's language of choice to go through the OFX memory suite, it would be very annoying to do without the memory allocation being actually global.

garyo commented 1 year ago

Gary will add this note: "Recommended: hosts should return the same suite pointers to all plugins in the same shared lib or bundle"