[rtmpdump] Proposal: dynamically loadable plugins

[Antti Ajanki]

Hi, and thanks for the comments.

On 26.02.2012 10:57, Howard Chu wrote:
> Antti Ajanki wrote:
>> Hi,
>>
>> These patches implement the plugin API I proposed in my previous mail.
>> There is also a slightly modified version of the callback patch
>> included. Thanks for Adam for the comments. The same patches are also
>> available here:
>>
>> git clone http://users.tkk.fi/~aajanki/rtmpdump-yle/rtmpdump.git
>>
>> This has been tested only on Linux. It also cross-compiles on mingw, but
>> I haven't tried running it on Windows.
>
> I see that you don't look for plugins in the HOME dir on Windows. Why not?

It should check the home dir on Windows, too. I can't remember any valid 
reason why I had that part commented out on Windows.

>> The patches add two new dependencies: libltdl for cross-platform dynamic
>> library loading support and pthreads for synchronizing the loading of
>> the plugins.
>
> I'm quite uncomfortable with these added dependencies. libltdl I can
> sort of understand, but pthreads? No. That increases the footprint of
> the library by a huge amount, for practically no benefit. It is the
> calling app's responsibility to ensure that only one thread tries to
> initialize this library.

In general I find it poor API design to require the client code to call 
a global initializer function before other library functions, because 
the call to the init function is easily forgotten and that may cause 
strange problems later on. But in this case I see your point that the 
benefit of streamlining the API is probably not worth having pthreads as 
a dependency.

> Also, you seem to have completely missed the philosophy behind this
> code, and the reason why AVals are used everywhere.

Fair enough. I'll convert the code to use AVals.

> Also in the callback patch you've got a namespace violation; rtmp.h must
> not define any user-visible symbols that aren't prefixed by RTMP_ or
> rtmp_. Your NUM_CALLBACK_TYPES should be RTMP_NUM_CALLBACK_TYPES.

I'll fix that.

> I'm not in love with the notion of multiple types of callbacks, each
> with differing signatures. I would prefer a single callback definition,
> passing the invocation type down to the callback and letting the
> callback decide if it wants to handle it or not. In practice I suspect
> most of the 4 type pointers will be unused. But I'm willing to listen to
> reasons justifying why this approach is better.

It simplifies the plugin code. The plugin author sees directly from the 
prototype which parameters are relevant for that type of callback. All 
the complexity of parsing those parameters out of RTMP packets is 
handled by the library.

It also makes it easier to add new types of callbacks in the future by 
simply including a new prototype in the header file instead of changing 
the existing callback prototype (which would require recompiling all 
plugins).

> Overall it's a good idea and a good effort. But I won't merge it as-is.
> If I have time I may commit a cleaned up version later. If you would
> like to clean it up yourself and repost it, that would be better than
> waiting for me to do it.

I'll cleanup the patch but won't have time for it for the next few days.

Antti