FFGL 2 SDK

FFGL, OSC, GLSL. If you like abbreviations, this is the forum for you
User avatar
subpixel
Hasn't felt like this about software in a long time
Posts: 139
Joined: Thu Jun 05, 2014 09:32
Location: Melbourne, AU

FFGL 2 SDK

Post by subpixel » Sat Jul 13, 2019 14:37

I have been hacking away at a copy of the FFGL 2 SDK. I have many questions, such as:
  • Why is this nasty pointer recasting code still all up in my face? (FFGL.cpp)
  • It seems that getParamRange() is using the FFMixed inputValue argument to store the requested data, whereas everything else seems to use the FFMixed retval variable - what gives?
  • Is the "FFT" buffer really 2048 FFT "bins", or is it audio sample data? What are we really expecting in that buffer? And is making three copies of the data really necessary (third copy is in class Audio, something like fft = _fft)?
  • SetBeatinfoStruct members should be type float, right?
  • I'm not a seasoned C++ code monkey, but I see "const" at the end of some method declarations. Seems like a good idea, why aren't more methods const?
  • What are the pros and cons on either using char* or std::string more consistently? Can/should we make a GLString type based on GLchar to keep everything looking like it is using the correct character type?
  • Do parameter names really need to be non-null-terminated char[16]? (And did anyone notice the AddSubtract example tries to use a name longer than 16 characters?)
  • Is it just me, or is the code in the ffglquickstart more or less duplicating the original structures. Can we not just extend the original structures without keeping separate copies of all the data? It is all very confusing right now. Also, can we do away with the "Ptr" classes and just keep the params in a vector? (And also get rid of the linked list stuff in the main class?)
Also, running the debugger, I have noticed calls from Reoslume Arena v7.0.0 rev64956 to methods with bogus parameter IDs, in addition to an in-general large number of calls to query parameter names, types, etc, such as when Resolume is starting up. I'll address such matters in another post.

It also seems that Resolume tries to fill the FFT buffer with one value at a time. That seems like a bad idea. One of the reasons for using a buffer is for wholesale data transfer. Where's the buffer at? :P

I have some code to share for comments and discussion, but am not familiar with GitHub pull requests and so on, so I'll post a zip to download.

Work In Progress - Resolume FFGL 2.x Codebase - subpixel
http://subpixels.com/resolume/ffgl/wip/

subpixel

Menno
Team Resolume
Posts: 90
Joined: Tue Mar 12, 2013 13:56

Re: FFGL 2 SDK

Post by Menno » Mon Jul 15, 2019 13:34

  • Why is this nasty pointer recasting code still all up in my face? (FFGL.cpp)
    Plugins aren't expected to interact with FFGL.cpp. Plugins are expected to use the FFGLPluginSDK.h as their entrypoint into the ffgl sdk. FFGL's internals require all this pointer casting because ffgl uses an opcode based interface. Everything passed between the plugin and it's host needs to be a generic type, pointers are used because then you can have the specification dictate what is pointed to, which is usually just a structure wrapping the actual arguments/return values.
  • It seems that getParamRange() is using the FFMixed inputValue argument to store the requested data, whereas everything else seems to use the FFMixed retval variable - what gives?
    I'm not quite sure i understand what you mean, but the RangeStruct is a complex return type that cannot be stored in a FFMixed. If the plugin were to return a FFMixed it would need to use a pointer, if the pointer would point to heap allocated memory there would be a need to be able to free it, so instead it's outputting in memory allocated by the host and passed in as address.
  • Is the "FFT" buffer really 2048 FFT "bins", or is it audio sample data? What are we really expecting in that buffer? And is making three copies of the data really necessary (third copy is in class Audio, something like fft = _fft)?
    I'm not an audio guy so i'm not 100% sure but yeah it should be fft bin values. (the values of those little bars when you set a param to fft). No this extra copy shouldn't be needed, could be a move or const& argument, whatever you like more.
  • SetBeatinfoStruct members should be type float, right?
    yes one float for bpm and one for barPhase
  • I'm not a seasoned C++ code monkey, but I see "const" at the end of some method declarations. Seems like a good idea, why aren't more methods const?
    Functions that modify a class's members cannot be const. constness in ffgl doesn't provide as much safety as i would like because the host/plugin interface cannot propagate constness. Is there any specific function you'd expect to be const?
  • What are the pros and cons on either using char* or std::string more consistently? Can/should we make a GLString type based on GLchar to keep everything looking like it is using the correct character type?
    The FFGL interface is C oriented, so everything needs to be char* eventually. Internally std::string could be used if it weren't for restrictions that the ffgl spec imposes. eg the spec imposes maximum string lengths and allows string to not be nul terminated. std::string doesn't support these constraints. I like std::string a lot but ffgl needs adjustments to it's interface before we're able to use it everywhere where strings are involved.
  • Do parameter names really need to be non-null-terminated char[16]? (And did anyone notice the AddSubtract example tries to use a name longer than 16 characters?)
    No you may nul terminate them, you'll lose a character though. As said with previous question, i'd like to change the requirement from 'max 16 long' to 'always nul terminated'.
  • Is it just me, or is the code in the ffglquickstart more or less duplicating the original structures. Can we not just extend the original structures without keeping separate copies of all the data? It is all very confusing right now. Also, can we do away with the "Ptr" classes and just keep the params in a vector? (And also get rid of the linked list stuff in the main class?)
    The quickstart is intended to be an optional component you can ignore. Any seriously sized plugin probably wont use it and will use the bare classes as they are.

User avatar
subpixel
Hasn't felt like this about software in a long time
Posts: 139
Joined: Thu Jun 05, 2014 09:32
Location: Melbourne, AU

Re: FFGL 2 SDK

Post by subpixel » Tue Jul 16, 2019 02:03

I was under the impression that the team at Resolume created the latest versions of the SDK. Am I mistaken?

Why not just make the changes you want?

The change to OpenGL 4 seems to be a major breaking change, so why not change the small stuff like parameter names?

PS: In case it didn't come across, I was attempting humour in my original post. I do have a lot of questions though. :)
Last edited by subpixel on Tue Jul 16, 2019 04:59, edited 1 time in total.

User avatar
subpixel
Hasn't felt like this about software in a long time
Posts: 139
Joined: Thu Jun 05, 2014 09:32
Location: Melbourne, AU

Pointer recasting in FFGL.cpp

Post by subpixel » Tue Jul 16, 2019 03:18

subpixel: Why is this nasty pointer recasting code still all up in my face? (FFGL.cpp)

Menno: Plugins aren't expected to interact with FFGL.cpp. Plugins are expected to use the FFGLPluginSDK.h as their entrypoint into the ffgl sdk. FFGL's internals require all this pointer casting because ffgl uses an opcode based interface. Everything passed between the plugin and it's host needs to be a generic type, pointers are used because then you can have the specification dictate what is pointed to, which is usually just a structure wrapping the actual arguments/return values.

subpixel: I come from a C background and understand what the pointer recasting is for. What I was asking is: why not hide all that ugly mess and encourage stronger typing within the SDK for types other than unsigned int?

I don't know if you had a chance to peruse the code I posted - I made a bunch of changes to make nearly all the (type *) casts go away, and to not have method/function arguments and return values throwing (void*) pointers around.

Change to FFMixed in FFGL.h, from:

Code: Select all

typedef union FFMixed
{
    FFUInt32 UIntValue;
    void* PointerValue;
} FFMixed;
to:

Code: Select all

// Forward declaration of types referenced in union FFMixed
class CFreeFrameGLPlugin;
typedef struct PluginInfoStructTag PluginInfoStruct;
typedef struct PluginExtendedInfoStructTag PluginExtendedInfoStruct;
typedef struct SetParameterStructTag SetParameterStruct;
typedef struct SetBeatinfoStructTag SetBeatinfoStruct;
typedef struct SetHostinfoStructTag SetHostinfoStruct;
typedef struct RangeStructTag RangeStruct;
typedef struct GetRangeStructTag GetRangeStruct;
typedef struct FFGLViewportStructTag FFGLViewportStruct;
typedef struct FFGLTextureStructTag FFGLTextureStruct;
typedef struct ProcessOpenGLStructTag ProcessOpenGLStruct;
typedef struct GetParameterElementNameStructTag GetParameterElementNameStruct;
typedef struct GetParameterElementValueStructTag GetParameterElementValueStruct;
typedef struct SetParameterElementValueStructTag SetParameterElementValueStruct;

typedef union FFMixed
{
    FFUInt32 UIntValue;
    void* PointerValue;

    float FloatValue;
    double* pDouble;
    const char* StrValue;

    CFreeFrameGLPlugin* pCFreeFrameGLPlugin;

    const PluginInfoStruct* pPluinInfo;
    const PluginExtendedInfoStruct* pPluginExtendedInfo;
    SetParameterStruct* pSetParameter;
    SetBeatinfoStruct* pSetBeatinfo;
    SetHostinfoStruct* pSetHostinfo;
    RangeStruct* pRange;
    GetRangeStruct* pGetRange;
    FFGLViewportStruct* pFFGLViewport;
    FFGLTextureStruct* pFFGLTexture;
    ProcessOpenGLStruct* pProcessOpenGL;
    GetParameterElementNameStruct* pGetParameterElementName;
    GetParameterElementValueStruct* pGetParameterElementValue;
    SetParameterElementValueStruct* pSetParameterElementValue;
} FFMixed;
There is no harm to the interface; everything is still a 32-bit value, whether that represent an unsigned int, float, pointer to basic type (double*) or pointer to structure/class object. Ideally, more (if not all) of union members pointers could be const pointers, and this definition used in both host and plugin code to encourage proper respect of data other's data structures.

Example 1.

Code: Select all

case FF_GETINFO:
    retval.PointerValue = (PluginInfoStruct*)getInfo();
    break;
In this case the cast is redundant since Pointervalue is (void*) and getInfo() has already anonymised and stripped any constness:

Code: Select all

void* getInfo()
{
    return (void*)( g_CurrPluginInfo->GetPluginInfo() );
}
My version:

Code: Select all

// 2019-07-11 ffgl@subpixels.com - changed return type from (void*) to (const PluginInfoStruct*)
const PluginInfoStruct* getInfo()
{
    return g_CurrPluginInfo->GetPluginInfo();
}

//...

    switch( functionCode )
    {
    case FF_GETINFO:
        retval.pPluinInfo = getInfo();
        break;

Example 2.

Code: Select all

switch( functionCode )
//...

case FF_SETPARAMETER:
    if( pPlugObj != NULL )
    {
        if( getParameterType( ( (const SetParameterStruct*)inputValue.PointerValue )->ParameterNumber ) == FF_TYPE_TEXT )
        {
            retval.UIntValue = pPlugObj->SetTextParameter( ( (const SetParameterStruct*)inputValue.PointerValue )->ParameterNumber,
                                                           (const char*)( (const SetParameterStruct*)inputValue.PointerValue )->NewParameterValue.PointerValue );
        }
        else
        {
            retval.UIntValue = pPlugObj->SetFloatParameter( ( (const SetParameterStruct*)inputValue.PointerValue )->ParameterNumber,
                                                            ( *(float*)&( (const SetParameterStruct*)inputValue.PointerValue )->NewParameterValue.UIntValue ) );
        }
    }
    else
    {
        retval.UIntValue = FF_FAIL;
    }
    break;
case FF_GETPARAMETER:
    if( pPlugObj != NULL )
    {
        if( getParameterType( inputValue.UIntValue ) == FF_TYPE_TEXT )
        {
            retval.PointerValue = pPlugObj->GetTextParameter( inputValue.UIntValue );
        }
        else
        {
            float fValue     = pPlugObj->GetFloatParameter( inputValue.UIntValue );
            retval.UIntValue = *(FFUInt32*)&fValue;
        }
    }
    else
    {
        retval.UIntValue = FF_FAIL;
    }
    break;
//...
}
becomes

Code: Select all

FFMixed retval;
retval.UIntValue = FF_FAIL;// Assume failure until we succeed

//...

switch( functionCode )
{
//...
case FF_SETPARAMETER:
    if( pPlugObj != NULL )
    {
        const auto args = inputValue.pSetParameter;

        if( getParameterType( args->ParameterNumber ) == FF_TYPE_TEXT )
            retval.UIntValue = pPlugObj->SetTextParameter( args->ParameterNumber, args->NewParameterValue.StrValue );
        else
            retval.UIntValue = pPlugObj->SetFloatParameter( args->ParameterNumber, args->NewParameterValue.FloatValue );
    }
    break;
case FF_GETPARAMETER:
    if( pPlugObj != NULL )
    {
        if( getParameterType( inputValue.UIntValue ) == FF_TYPE_TEXT )
            retval.StrValue = pPlugObj->GetTextParameter( inputValue.UIntValue );
        else
            retval.FloatValue = pPlugObj->GetFloatParameter( inputValue.UIntValue );
    }
    break;
//...
}
Example 3.

Code: Select all

case FF_SET_BEATINFO:
    if( pPlugObj != NULL )
    {
        const SetBeatinfoStruct* beatInfo = reinterpret_cast< const SetBeatinfoStruct* >( inputValue.PointerValue );
        float bpm                         = *(float*)&beatInfo->bpm.UIntValue;
        float barPhase                    = *(float*)&beatInfo->barPhase.UIntValue;
        pPlugObj->SetBeatInfo( bpm, barPhase );
        retval.UIntValue = FF_SUCCESS;
    }
    else
    {
        retval.UIntValue = FF_FAIL;
    }

    break;
becomes

Code: Select all

case FF_SET_BEATINFO:
    if( pPlugObj != NULL )
    {
        const auto args = inputValue.pSetBeatinfo;
        pPlugObj->SetBeatInfo( args->bpm, args->barPhase );
        retval.UIntValue = FF_SUCCESS;
    }
    break;
Yes, we are still making an assumption about what the 32-bit value is coming in (based on the conventions of the interface with the FFGL host), but within the SDK we can use types for accuracy and clarity.

User avatar
subpixel
Hasn't felt like this about software in a long time
Posts: 139
Joined: Thu Jun 05, 2014 09:32
Location: Melbourne, AU

Passing/returning ranges

Post by subpixel » Tue Jul 16, 2019 04:16

subpixel: It seems that getParamRange() is using the FFMixed inputValue argument to store the requested data, whereas everything else seems to use the FFMixed retval variable - what gives?

Menno: I'm not quite sure i understand what you mean, but the RangeStruct is a complex return type that cannot be stored in a FFMixed. If the plugin were to return a FFMixed it would need to use a pointer, if the pointer would point to heap allocated memory there would be a need to be able to free it, so instead it's outputting in memory allocated by the host and passed in as address.

subpixel: Yes, RangeStruct is larger than 32 bits. So too are: PluginInfoStruct, PluginExtendedInfoStruct, ParamInfoStruct::Name, ParamInfoStruct::StrDefaultValue. These ARE passed back to the host as an FFMixed containing a pointer to the data.

Here are the other cases (not guaranteed exhaustive):

Code: Select all

case FF_GETINFO:
    retval.pPluinInfo = getInfo();
    break;
case FF_GETPARAMETERNAME:
    retval.StrValue = getParameterName( inputValue.UIntValue );
    break;
case FF_GETPARAMETERDEFAULT:
    retval = getParameterDefault( inputValue.UIntValue );
    break;
case FF_GETPARAMETERDISPLAY:
    if( pPlugObj != NULL )
        retval.StrValue = pPlugObj->GetParameterDisplay( inputValue.UIntValue );
    else
        retval.StrValue = (char*)FF_FAIL;
    break;
case FF_GETPARAMETER:
    if( pPlugObj != NULL )
    {
        if( getParameterType( inputValue.UIntValue ) == FF_TYPE_TEXT )
            retval.StrValue = pPlugObj->GetTextParameter( inputValue.UIntValue );
        else
            retval.FloatValue = pPlugObj->GetFloatParameter( inputValue.UIntValue );
    }
    break;
case FF_GETEXTENDEDINFO:
    retval.pPluginExtendedInfo = getExtendedInfo();
    break;
case FF_GET_PARAMETER_ELEMENT_NAME:
{
    const auto args = inputValue.pGetParameterElementName;
    retval.StrValue = getParameterElementName( args->ParameterNumber, args->ElementNumber );
    break;
}
case FF_GETPLUGINSHORTNAME:
    retval.StrValue = getPluginShortName();
    break;
The range-returning code is the only one that doesn't follow this pattern.

Code: Select all

case FF_GET_RANGE:
    //TODO: 2019-07-17 ffgl@subpixels.com - investigate whether inputValue should be used to pass back the range
    retval = getParamRange( inputValue );
    break;
Details from getParamRange() (where I have made a different change already, not directly addressing this issue):

Code: Select all

// 2019-07-11 ffgl@subpixels.com
// - use FFMixed::pGetRange instead of pointer recasting
FFMixed getParamRange( FFMixed input )
{
    FFMixed ret;
    ret.UIntValue = FF_FAIL;
    if( s_pPrototype == NULL )
    {
        FFResult dwRet = initialise();
        if( dwRet == FF_FAIL )
            return ret;
    }
    ret.UIntValue = FF_SUCCESS;

    GetRangeStruct* getRange = input.pGetRange;

    RangeStruct range = s_pPrototype->GetParamRange( getRange->parameterNumber );
    getRange->range   = range;
    return ret;
}
I hear you re heap allocation. The range data you are after is ALREADY on the heap. The idiom used for the other calls is to return a pointer to data structures on the heap and trust the host not to abuse it.

Perhaps a better convention would be for the host to ALWAYS provide a pointer to it's own structure, buffer (with size), etc and the return value to always and only be a status code (success, failure, etc).

eg something like:

Code: Select all

FFResult __stdcall plugMain( FFUInt32 functionCode, FFMixed inputValue, FFInstanceID instanceID, FFMixed outputValue, FFUInt32 outputLimit )
where outputLimit could be a maximum buffer length or similar.

Failing that, keeping to the pattern used by every other call prior to this seems wise. I don't see any documentation nor warning about the current interface behaviour.

User avatar
subpixel
Hasn't felt like this about software in a long time
Posts: 139
Joined: Thu Jun 05, 2014 09:32
Location: Melbourne, AU

BeatInfostruct

Post by subpixel » Tue Jul 16, 2019 04:33

subpixel: SetBeatinfoStruct members should be type float, right?

Menno: yes one float for bpm and one for barPhase

subpixel: I mean declared as float, not declared as FFMixed.

Code: Select all

// SetBeatinfoStruct
// 2019-07-12 ffgl@subpixels.com - changed bpm and barPhrase types from FFMixed to float
typedef struct SetBeatinfoStructTag
{
    float bpm;
    float barPhase;
} SetBeatinfoStruct;
Similar example: RangeStruct.

User avatar
subpixel
Hasn't felt like this about software in a long time
Posts: 139
Joined: Thu Jun 05, 2014 09:32
Location: Melbourne, AU

ffglquickstart duplicating SDK structures (and data)

Post by subpixel » Tue Jul 16, 2019 04:51

subpixel: Is it just me, or is the code in the ffglquickstart more or less duplicating the original structures. Can we not just extend the original structures without keeping separate copies of all the data? It is all very confusing right now. Also, can we do away with the "Ptr" classes and just keep the params in a vector? (And also get rid of the linked list stuff in the main class?)

Menno: The quickstart is intended to be an optional component you can ignore. Any seriously sized plugin probably wont use it and will use the bare classes as they are.

subpixel: When I was reading the ffglquickstart code, I was comforted to see that it does (in a somewhat more elegant way) many of the same things that my own plugins were trying to do to manage parameter values. It doesn't solve some of the issues I found with my own code though.

Issue: basic stuff like paramater name, parameter type, default value, all need to be declared somewhere. Code has string constants, static data or whatever to set up its own structures which are then passed to the main SDK to be copied. Why can't the copy used by the SDK be THE copy, accessible from the SDK and the plugin code? My point is that it can be and maybe even should be. If you want to have custom extensions to the Param class, fine. Whatever the SDK doesn't need, it doesn't need. Polymorphism handles what it does need, and the plugin-specific code can use whatever extensions it likes.

This is what I want to tackle next. I think that so long as the calling convention between host and plugin is not broken (except where we specifically want to change it, as mentioned elsewhere), we should tear apart all the crap things in the SDK (like the hand-coded linked list to store the parameters) and fix them.

User avatar
flyingrub
Met Resolume in a bar the other day
Posts: 8
Joined: Thu Feb 21, 2019 10:24
Location: Out of space

Re: FFGL 2 SDK

Post by flyingrub » Tue Jul 16, 2019 14:27

Pointer recasting in FFGL.cpp :
I already had a look at your .zip and found this solution very elegant, I don't see a reason why this should not be implemented like this.

Passing/returning ranges :
I wanted to have a status return value and a data return value so I use this different pattern here.

BeatInfostruct :
They should definitely be float. :oops:

ffglquickstart duplicating SDK structures (and data) :
I was thinking about this too. I already had a few issue dealing with the linked list and pointer manually has I am not used to this at all ( thanks to smart pointer and modern std ). I think that the old ffgl SDK could need an update.

About the `Ptr` typedef, I wanted to hide the shared_pointer to not confuse someone who has few experience with C++. To be able to use polymorphism we need to use pointer anyway.

User avatar
subpixel
Hasn't felt like this about software in a long time
Posts: 139
Joined: Thu Jun 05, 2014 09:32
Location: Melbourne, AU

Re: FFGL 2 SDK

Post by subpixel » Tue Jul 16, 2019 21:30

Pointer recasting in FFGL.cpp :
Thanks for the feedback.

Passing/returning ranges :
The idiom is to return FF_FAIL on error (bit pattern for -1 cast as a uint32), else a pointer to the struct with the data. eg see things like return (char*)FF_FAIL.

BeatInfostruct :
Don't beat yourself up about it. šŸ˜„

ffglquickstart duplicating SDK structures (and data) :
Re "About the `Ptr` typedef, I wanted to hide the shared_pointer to not confuse someone who has few experience with C++. To be able to use polymorphism we need to use pointer anyway.":

(Edit: I wrote something daft about polymorphism in C++... Yes, you do need a pointer.)

There doesn't seem to be any advantage to using shared pointers here. The params objects will be held in a vector. The vector can own them, and everything else can use a regular pointer to the copy held by the vector. The vector calls the destructor of each element when it is itself destroyed.

I was waiting for some feedback on what I had done so far, which I have now. Unfortunately the Particles example didn't work for me and I don't know why yet. I haven't really touched upon anything much to do with the OpenGL changes yet - I am definitely excited about being able to do animation on the GPU! - but want to have a solid foundation for my projects before I start making a mess (maintaining an evolving "standard" for my existing plugins has been tedious). I'll try out some of the other examples from your flyingrub repository today (it is 6am now here in Melbourne) as they stand, then integrated with the SDK updates I've made so far. Once I'm happy with that I'll start on the next phase of removing the shared pointer code. When that is working, the last phase will be to update the SDK to incorporate the bulk of (if not all of) the ffglquickstart niceness.

New questions...

Passing shader uniforms
I note that the ffglquickstart shader code has foregone storing uniform "locations", and calls the finduniform methods every time. I imagine that could be a relatively expensive call (string search? hash map? who knows?); did you do any performance profiling? It is pretty easy to store the uint32 result for each uniform in the params list/vector (I did that in some of my existing plugins), so suggest we do that.

Logging
Where does the log go to? I didn't figure that out for FFGL1.5 Resolume plugins. Maybe it is changed in FFGL2. It would be nice if shader errors etc could be logged to the Resolume log file. There are of course considerations re 3rd party plugins "polluting" Resolume's log files; perhaps more of a business process issue than a technical one.

User avatar
subpixel
Hasn't felt like this about software in a long time
Posts: 139
Joined: Thu Jun 05, 2014 09:32
Location: Melbourne, AU

Re: FFGL 2 SDK

Post by subpixel » Tue Jul 16, 2019 22:14

Maybe when I try putting derived classes in a vector I will find why shared pointers are "necessary"... It has occurred to me that the vector would have to be a vector of pointers, not a vector of objects. I will have to think about this more and find out as I play with it. My previous thinking was likely the c# way. šŸ˜

Post Reply