Add an extra argument to fetchSuite to enable the host to return NULL…

[MrKepzie]

This PR implements what was suggested by Pierre to add a way for a plug-in to check if a specific function is supported by a given host in a suite.

An extra argument is added to the fetchSuite function of the OfxHost struct, allowing the plug-in to ask for the Host a suite structure which potentially has NULL instead of a function pointer for an unsupported function in the structure.

The plug-in can also check that the host actually support returning NULL functions in suite structures by inspecting the property kOfxPropHostSupportsNullSuiteFunctionPointers.

Note that since inspecting this property requires the kOfxPropertySuite suite to be loaded first, it is expected that the host returns NULL function pointers for unsupported functions of the kOfxPropertySuite and that the plug-in set allowNullFunctionPointers to 1 when fetching this suite.

[PierreJasmin]

This is mostly useful during Description stage - to complement Property Supported Testing, so before parameter creation. The idea overall is to be able to internally map what an host actually supports.

I think it's maybe not evolutionary nice to add an extra argument as I read to the fetchSuite function of the OfxHost struct, wouldn't that break something?

It does not seem that OfxHost Struct was planned with the idea that it could change. So OfxPropertySuiteV1 would then need to be used for this properly and be the first suite one fetches, and use by plugin to set that preference, and if then gets that property from host also set to 1 or true or "kOfxPropHostNullSuiteFunctionPointersSupported" :) -- the plugin can then assume he can collect from Host such mapping via checking for NULL for all suites.

No action needed here for hosts supporting all the stuff in a given suite.

It might be sufficient outside of the Description Contexts to do as we do now and just have the host return an error for something not supported. Trying to prevent extra work to host in terms of not maintaining two different suites internally for each. And old assumption ideally needs to be maintained for old plugins to work in new version.

[MrKepzie]

Adding the parameter to fetchSuite was the only thing I could come up with that sounds nice, or adding an extra member of the OfxPlugin struct.
Adding a property to the image effect descriptor means that we need to wait describe/describeInContext to actually figure out if a plug-in can receive NULL suite, however in practise the plug-in may call fetchSuite a lot sooner in the load action. This is what is done in the official C++ wrapper on the plug-in side.
Also, if you don't add this parameter, how the host is supposed to know which plug-in is calling fetchSuite ?

That requires the host to keep as member somewhere which plug-in it called an action on (possibly over thread local storage to be thread safe) to determine which plug-in is calling fetchSuite.

Adding an extra argument overall is the smallest and logical way of doing it, since anyway this is an argument specifying what should be returned by the host.
On host side there's mostly nothing to change but the signature of fetchSuite.

If a host wants does not support some functions of a suites and wants to advertise these functions, it just needs to define 2 structures, one with all functions, and one with unsupported functions that are NULL. Given the parameter from fetchSuite it just returns one or the other.

[MrKepzie]

@PierreJasmin Two possible solutions to cope with the problem of compatibility due to a change of the fetchSuite function signature:

1. If a plug-in wants a suite with possibly NULL function, it has to wait for doing so in describeInContext, where the host will at this time know whether the plug-in supports NULL suite function with the property kOfxPropHostNullSuiteFunctionPointersSupported

2. We change the OfxHost struct into a OfxHostV2.
   We need to also introduce a version number so that in the future we know to which type we can cast the host pointer/plugin pointer to on respectively host side/plug-in side, for compatibility reasons. Currently we only have kOfxImageEffectPluginApiVersion which is not enough.
   That means we could freely change the OfxHost/OfxPlugin structures as long as the plug-in/host checks the correct version first.
   A way to implement that without breaking compatibility is to add a new exported function to the standard, namely:

   #define  kOfxStandardVersionMajor 1
   #define  kOfxStandardVersionMinor 4
   #define  kOfxStandardVersionRev 1

   #define OFX_VERSION_ENCODE(major, minor, revision) ( \
            ( (major) * 10000 ) \
            + ( (minor) * 100 )  \
           + ( (revision) * 1 ) )

   #define kOfxStandardVersion OFX_VERSION_ENCODE( \
           kOfxStandardVersionMajor, \
           kOfxStandardVersionMinor, \
           kOfxStandardVersionRev)


   /** @brief Defines the OpenFX standard version implemented on plug-in side.
   **/
   OfxExport int OfxGetStandardVersion(void) { return kOfxStandardVersion; }

This way, the host can first look-up whether this function exists in the plug-in.
If the function exists, the host can cast the OfxPlugin struct to the version that is defined by the standard, i.e: OfxPluginV2.
If the function does not exist, the host assumes that the plug-in only knows how to talk the standard defined originally.

The OfxPluginV2 struct would only change the signature of the setHost function:

    // Was originally:
   //    void     (*setHost)(OfxHost *host);
    // Added host version of the standard so that the plug-in can cast the host pointer to the correct structure. This function will only be called by hosts that support the OfxGetStandardVersion() function.
    void     (*setHost)(void *host, int hostOfxStandardVersion);

Once the host knows the plug-in version, it can call setHost indicating in turn its version.
Plug-in retrives OfxHostV2 struct which would contain the new fetchSuite signature.

Does this change sounds reasonable to you or out of scope?

[PierreJasmin]

Maybe 1. is enough for now - and move OfxStructV2 sort of ideas to a further version problem.

[PierreJasmin]

I have another idea, we could simply on as-needed basis build a table of function names to check against. This way it would be queryable from any action.
Maybe we could even just make a kOfxFuncSupported as we can query suite via const char *suiteName, int suiteVersion as args - etc and use the function name (e.g. (*paramGetIntegral)) as as string "paramGetIntegral" for argument so we don't have to use tables to define that.

[Dithermaster]

Paul Miller has looked at this from both host and plug-in side - do you have any comments or ideas? ///d@

…

On Thu, Feb 8, 2018 at 12:29 PM, PierreJasmin ***@***.***> wrote: I have another idea, we could simply on as-needed basis build a table of function names to check against. This way it would be queryable from any action. Maybe we could even just make a kOfxFuncSupported as we can query suite via const char *suiteName, int suiteVersion as args - etc and use the function name (e.g. (*paramGetIntegral)) as as string "paramGetIntegral" for argument so we don't have to use tables to define that. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#48 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF_wI1IT0t2VbVbiqttfyaJFwibX09LSks5tSz0QgaJpZM4RO1BR> .

[garyo]

Something like Pierre's suggestion of kOfxFuncSupported seems much better than changing the signature of a core API function.

[garyo]

@fxtech (Paul), any opinions on this?

To flesh out Pierre's suggestion:

• A new function, probably in a new host-side suite that can be queried for, OfxFuncSupported (const char *suiteName, int suiteVersion, const char *functionName);
  ?

I'd like to see some sample code for this stuff; both from a host perspective and how a plugin would call it.
I'm thinking that most hosts would just always return True with certain specific exceptions for suites they've partially implemented. That seems simple. (Host folks?)
But the plugin side seems to get problematic over time: does a plugin need to call this before calling any function in any suite, because it might be unimplemented? Or will the spec tag certain suite functions as optional, where the plugin has to call this first? This is really a more general question. Why wouldn't a host implement all of a suite? What's a legit use case for this?

[fxtech]

On 2/27/18 10:10 AM, GaryO wrote: @fxtech <https://github.com/fxtech> (Paul), any opinions on this? To flesh out Pierre's suggestion: * A new function, probably in a new host-side suite that can be queried for, |OfxFuncSupported (const char *suiteName, int suiteVersion, const char *functionName);| ?

I didn't really like that at all. I don't see how this: if (OfxFuncSupported(kOfxParameterSuite, 1, "paramDeleteAllKeys"))     g_paramSuite->paramDeleteAllkeys() Is sufficiently different from: if (g_paramSuite->paramDeleteAllKeys)     g_paramSuite->paramDeleteAllKeys(); If used on-site, these are probably roughly equivalent to: if (g_paramSuite->paramDeleteAllKeys() != kOfxStatErrMissingHostFeature)     // it worked! My issue was more about set-up, where it would be useful to know ahead of time if certain features will really be implemented. In that case, the existing check for kOfxStatErrMissingHostFeature is useless, but I'd argue the other two options are still equally viable. Assuming it is supposed to be a requirement that all suite function pointers should point to some function as of v1.4, and a check for kOfxStatErrMissingHostFeature is the only current option, I don't see how a relaxing of the "function pointer should not be nullptr" requirement would hurt anything. I'm not convinced kOfxPropNullSuiteFunctionPointersSupported is required either.

I'm thinking that most hosts would just always return True with certain specific exceptions for suites they've partially implemented. That seems simple. (Host folks?)

I think this could be error-prone - a host would have to keep track of two places where information about what parts of suites are implemented.

But the plugin side seems to get problematic over time: does a plugin need to call this before calling /any/ function in /any/ suite, because it might be unimplemented? Or will the spec tag certain suite functions as optional, where the plugin has to call this first? This is really a more general question. Why wouldn't a host implement all of a suite? What's a legit use case for this?

I'm most in favor of adding some *optional* tags to the documentation, if we can all agree what can be optional, and allow the function pointer to be nullptr.

[PierreJasmin]

• A simple reason an host cannot implement everything in a suite is because it does not have that feature in the host. The simple example I gave was gotoTime in timeline suite and another example is an host might support paramGetValueAtTime but not paramSetValueAtTime... it's not like we can tell to the host well don't support the parameter suite. If I have a module in my plugin that depends on paramSetValueAtTime, then I need to know in Describe in Context so I skip that section of the plugin (or simply bail out).

• NullPtr: Yes we could skip the property flag and increment suite version instead. And in new version prescribe to return NULLPtr. That's the other option.

[garyo]

Pierre points out a few suite functions that may not make sense for certain hosts. It seems to me like these are special cases; not something that has to be generally supported. If that's the case, then IMHO the standard should specify that certain functions in certain suites are optional, and provide some way, specific to each suite but following a general pattern, to indicate that those particular functions are not implemented.
Host vendors: what are you doing now about those functions Pierre mentions? Returning errors? Could you create a list of functions that might fall into this category? It seems like for this to be useful all hosts would need this list anyway. The solution may be different if there are three than if there are fifty.
If the list is small, and they're carefully called out in the header and doc, then setting their function pointers to NULL would be acceptable, or better why not create a specific property on the affected host like kOfxImageEffectHostPropTimelineSuiteGotoTimeUnsupported.

[fxtech]

On 2/27/18 3:47 PM, GaryO wrote: Host vendors: what are you doing now about those functions Pierre mentions? Returning errors? Could you create a list of functions that might fall into this category? It seems like for this to be useful all hosts would need this list anyway. The solution may be different if there are three than if there are fifty.

Silhouette implements the following suites:     kOfxMemorySuite     kOfxMessageSuite    (v1 & 2)     kOfxPropertySuite     kOfxImageEffectSuite     kOfxParameterSuite (see notes)     kOfxInteractSuite     kOfxTimeLineSuite     kOfxMultiThreadSuite (see notes)     kOfxSilhouetteSuite    (contact me if you want info on Silhouette extensions) OfxMultiThreadSuiteV1 I didn't want to have to implement this but at least one plugin "requires" it, even though they don't use it. It's partially supported though:     multiThread runs on run thread, numCPUs reports 1     all the mutex functions return kOfxStatFailed (they shouldn't be needed with only one thread) I'll probably end up supporting all of it properly in an update if I have to, though I'd prefer to just remove it entirely. So far nobody has complained about it not working. OfxParameterSuiteV1     paramCopy - returns kOfxStatErrUnsupported     paramGetDerivative - returns kOfxStatErrUnsupported     paramGetIntegral - returns kOfxStatErrUnsupported

[PierreJasmin]

ParamSuite is all over the place with regards to what is supported, recently I hit an host that was always returning 1 to GetNumKeys - at first glance it looked like it was all working.

abort is an example for ImageEffect

TimelineSuite gotoTime

Property Suite is an example of something that we expect that it's fully supported

Some I don't know, like Interact interactSwapBuffers...

[fxtech]

interactSwapBuffers probably isn't needed at all. I'm guessing an interact update is all part of some existing double-buffered update/display cycle that the plugin would have no control over anyway. That's a no-op for me.

…

On 2/27/18 4:21 PM, PierreJasmin wrote: ParamSuite is all over the place with regards to what is supported, recently I hit an host that was always returning 1 to GetNumKeys - at first glance it looked like it was all working. abort is an example for ImageEffect TimelineSuite gotoTime Property Suite is an example of something that we expect that it's fully supported Some I don't know, like Interact interactSwapBuffers... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABnuAqoIZEsq54JXuG22ED4zSc5Y-f7Jks5tZH_sgaJpZM4RO1BR>.

[revisionfx]

OK resolution is - only perhaps Parameter suite is really the hard one here where this applies and this param suite is the one that should embed a particularly way to solve what is implemented or not SO as not to put this burden on all suites function pointer. All the other suites can be elegantly addressed via a V2 or adding a property...

[garyo]

I'm just re-reading this ancient PR. The last comment from Pierre seems to indicate some kind of agreement that the Parameter suite is the only problematic one and so rather than the approach in this PR, we should do something specific in a new revision of that suite -- is that correct? Can this PR be closed, and a new issue created for the Parameter suite problems?

[revisionfx]

" Can this PR be closed, and a new issue created for the Parameter suite problems?"

That was 5 years ago - would need to find my memory backup :)
In general, would be useful to scan suite functions to understand what is supported in Describe In Context (when we create params) without actually effecting the call per se... collecting kOfxStatErrMissingHostFeature at that stage would be useful.

Maybe add this to next agenda - a possible example is the Draw Suite, for example Mr Wells when we worked on that was suggesting that perhaps a texture would be useful - would certainly be useful in custom Param UI if they use the same set. Not asking we do that just providing an example.

New suite and versions of suites could adopt the mandatory and optional model and be set to nullptr by host further down?

[garyo]

I'm very wary of having null function pointers in the API. It's practically asking for crashes when plugins don't check before calling. I prefer the host returning an error, or adding a property to indicate (lack of) support.

[revisionfx]

OK no nullptr, summary 1) define a way at param creation time to understand HostMissingFeature and 2) host returns error when called after that?

[Guido-assim]

I like the idea of having a host properties like the previous mentioned kOfxImageEffectHostPropTimelineSuiteGotoTimeUnsupported. For every function that returns kOfxStatErrUnsupported the host can set a property. The property naming can be something generic including suite name and version and function name. From a host perspective I think this is a nicer solution than having to keep two versions of a suite and it avoids null pointers.