-
Notifications
You must be signed in to change notification settings - Fork 118
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
Binary data Parameters #145
Comments
See my comment in #142 (comment) with a PropertySuiteV2 I wrote 3 years ago that does the same |
Note that in my case, the plug-in "defined" metadata (e.g: a media reader), thus needed to create properties on its own. |
I prefer Alexandre's term of "Bytes" instead of "BinaryData" |
For Bytes parameters, we can avoid adding to or changing the API by doing it like this. Any comments?
|
That seems simple enough. Is the ownership model the same as Strings (up to the next API call, so plugin must copy, as in https://openfx.readthedocs.io/en/main/Reference/ofxCoreAPI.html#strings)? |
Yes, that is what I was thinking. The plugin could do its own decoding directly off that data pointer if it's done within the same call. |
I removed some discussion around supporting a Bytes property type in the general sense. |
Is this animatable? Often per-frame data. |
As for animation, we can decide whether we'd like it to animate like custom params, using a custom callback similar to OfxCustomParamInterpFuncV1, or be non-animatable. Per the spec, Customs and Strings may be animatable, depending on the host. So we should probably do the same for Bytes. As for memory, should be the same as string params (and this update should make that explicit to avoid any confusion). This was already discussed above. As for length, if I understand the proposal correctly, that's what the |
doc note: it is highly recommended to use the Memory Suite for larger allocations to give a chance to host memory management wise. The plugin is responsible to allocate and then to destroy that memory and reset pointer to null on action DestroyInstance Special Issue: Some hosts that have undo caching. We don't want each value change to trigger a sync private data etc Does this parameter if length is not 0 triggers a requirement to sync private data (save project including auto-save) - as is expected for custom data param maybe if one has set PropPersistent on that parameter? |
Unless you're suggesting something different than usual, my understanding is that when the plugin calls paramSetValue, the host must copy the data it got from the plugin (not just the struct, but the pointed-to memory). The plugin may then free its copy if it wants. And when the plugin calls paramGetValue, it must copy the value it receives from the host (same: not just the struct, but the pointed-to memory). The spec says that data is only valid until the next API call, so the host might overwrite or free its copy right afterward. Do you agree? So there is no data sharing here. If the plugin changes some values within the pointed-to memory, the host will never see those changes. |
Which specs? If I set to Persistent, I expect the host to hold that data, right? If you mean I have to use GetValue each time (e.g. each frame render) to get it back, ok. And you say that then I should after setValue like at exit of render call free that memory as the host will have copied it otherwise I will create a memory leak :) |
Spec location ("Strings"): https://openfx.readthedocs.io/en/main/Reference/ofxCoreAPI.html#strings Persistance just means the host will save & reload that param when the project is reopened. It's the default. It has nothing to do with sharing of data between plugin and host during a run. As with any param value, I would expect you have to get it each frame during render(). And what the plugin does with its own copy of that param value data is up to it. Of course it should free whatever it allocates. (But for instance, a plugin might just copy the param bytes into a known fixed location, or send them to another process, or pass them to a function that processes them right then. Totally up to the plugin.) |
The Bytes param type really works exactly like String or Custom, just with a length. |
This makes sense to me. The major difference between String and Custom is that Customs have an animation callback I wonder how many hosts implement this callback property for customs though? (It would be nice perhaps if String params had such a callback, for consistency. But that's another discussion.) |
A use case is per frame data - say it's 50K per frame, and the clip is 1000 frames, do we want to transmit 50 Mb each render loop? |
It's just a param, like a string or custom. No sync private data is needed; it's not private data. The host should save & load it just like any other param. Am I missing something? SyncPrivateData is just telling the plugin to sync its private data into params. If you need that to be called to get your private data into a custom or string param today, you'll need to do the same for the Bytes param. But that doesn't affect this spec change. As for your per-frame data, I assume you'll be calling |
My use-case for Bytes is purely for binary "sequence data", like Mocha or Silhouette project data when running as a plugin, or for custom Lens Flare data, or anything else that is binary and "big". I wasn't thinking in terms of per-frame. I know Silhouette doesn't support custom param animation callbacks. |
I see, I may have misunderstood Pierre. Pierre, perhaps you're saying you plan to store a single blob of data (not a value per frame, and no param animation expected), and this would be 50MB (it might represent an array of 1000 frames worth of data, but the API doesn't care about that). In that case yes, you'll have to memcpy the 50MB each frame, and similarly if you change it the host will have to memcpy it. So should we just say that Bytes params are non-animatable, full stop, just to keep things simple? Any other host folks care to chime in on this? |
I don't think it would be a huge burden to make them animatable. They currently are NOT in Silhouette, but it would be possible. If we mandate that String, Custom and Bytes params should be able to be animatable then I can add that. |
@fxtech there's a host param
|
I don't think we need an animation callback for that. Just regular KF indexing (previous or equal...) could be useful. In that case a UI for that in KF editor could be a bit like an animated boolean and that would be ok. All one might want is to move time location, delete and copy-paste. BTW another host that returned not animatable for choice just fixed it for next version. Almost cleared all hosts not supporting choice animation :) - down to 1 of 15 I think. |
I don't think the spec is clear on what happens if the host returns |
Forgive me if I remember wrong, but I believe the original intent for custom param interact was for folks who wanted to have custom UI interaction in their effects controls. I never tried to implement that in OpenFX. |
Not sure what you're talking about here? As far as I know there's no interacts for custom params, at least not in this context. |
Slightly drifting OT but Just saying way back then as we were listing all other API functionalities, this was also related to display custom UI in parameter control. We at some point also said we could use the Overlay suite in there too (still documents to use OpenGL), but seems it would likely need a background texture addition (and is likely 8bit display)? https://github.com/AcademySoftwareFoundation/openfx/blob/main/Examples/Custom/custom.cpp https://openfx.readthedocs.io/en/main/Reference/ofxParameter.html If no one implements this anyway, would likely be better to just use standard KF suite for all non numeric values. |
OK, understood -- let's try to keep this discussion topical. This is the official history of this spec change. :-) Further OT discussion to Slack please. My vote is yes and no in that order, so it would be like String. In that case |
"In that case getParamValueAtTime returns the current-or-previous value for hosts that allow animation." um... it returns data = NULL and length = 0 if you request AtTime where there is no KF/data maybe. It just uses the KF indexing as normal to find if 1) there is KF (num of it) and 2) where are the adjacent to current time ones... one can always get two if this implies some interpolation. In this case unlike choice, the only impact of not supporting anim is the possible size of transmitting back this at every frame render... |
Is that what String and Custom params do? I think we should try to be consistent. (Except for host bugs of course!) |
I never requested animated String and Custom here so I don't know |
I wasn't originally thinking of making this animatable, but it probably should be for parity with Custom (this is meant to be a Custom alternative). I imagine the main use will be a single blob of data used for the whole instance (ie. analysis data, lens-flare description, etc). |
We agreed that Bytes animation is required, and the host will use step interpolation to derive values. There will be no |
In order to provide a default value for Bytes, I'll need to rev |
100%. Since there's no UI, there's no need for plugin-specified default values. Empty byte array should be the default. |
default is NULLPTR, 0 |
Signed-off-by: Paul Miller <paul@fxtech.com>
PR #161 |
OK timely, we just tested custom data and about half the hosts at least don't support per frame data. |
I'll update the sample plugin to handle missing host support for Bytes parameters. |
The sample should also try setting and verifying (e.g. after save and relaunch) a value with null bytes followed by data, to ensure the host isn't erroneously using string functions on the param value. |
Open Effects Proposal for Standard Change
Please read the contribution guidelines first.
Standard Change Workflow
standard change
tagfeature/PROPOSAL-NAME
branch)maintainer merges PR to master which closes PR and issue
Requirements for accepting a standard change:
Summary
There is currently no way to have parameters that store opaque binary.
Motivation
Currently, opaque binary data stored as a Custom parameter type must be encoded as a null-terminated string,
which can add overhead for encoding/decoding, as well as adds some ambiguity to hosts which know nothing
about what is in the data (is it binary or just a null-terminated string?), and therefore must also encode/decode
the value when saving their projects.
Problem
See Motivation above.
Impact
A new Parameter type (kOfxParamTypeBytes) is required, along with an associated OfxBytes struct for storing the relevant information.
If a host doesn't support kOfxParamTypeBytes, it can return kOfxStatErrUnsupported from paramDefine(), at which point a plugin can always fall back to using a traditional Custom parameter.
Documentation Impact
Descriptions of the new definition and struct must be written as part of the header documention, in addition to a sample.
Stakeholders
Both plugin writers and host vendors will benefit from this, since Bytes parameters will not need to go through potentially two extra encoding/decoding steps, thus speeding up hosts and plugins and reducing project data size.
Discussion
The text was updated successfully, but these errors were encountered: