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?

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. Also, you seem to have completely missed the philosophy behind this code, and the reason why AVals are used everywhere. It's not enough to write good code or correct code. You should always write *optimal* code. Some of the rules for optimal code: 1) Never compute at runtime what is already known at compile time. E.g., never use strlen("a string constant") when you can simply use sizeof("a string constant")-1. 2) Never compute the same data twice at runtime, unless the second occurrence is very long after the first one and it's cheaper to recompute it than to remember it. In this case, your struct RTMPPluginOption is the obvious offender: +typedef struct RTMPPluginOption { + const char *name; /* Name of the option in SetupURL string */ + const char *type; /* Type name to be shown in the help screen */ + const char *usage; /* A short description in the help screen */ + void (*parseOption)(const AVal *name, const AVal *val, void *ctx); + /* Function that will be called if the SetupURL string contains + * this option. ctx is the user data pointer returned by the + * create hook. */ +} RTMPPluginOption; It's even more heinous since you *know* that you need to use AVals to talk to the rest of the library. This function +static RTMP_Plugin *FindPluginByOpt(const AVal *opt, + const RTMPPluginOption **optinfo) +{ + RTMPPluginNode *pl = GetPlugins(); + while (pl) + { + const RTMPPluginOption *plopt = pl->plugin->options; + while (plopt && plopt->name) + { + if (opt->av_len == strlen(plopt->name) && + strcasecmp(opt->av_val, plopt->name) == 0) + { + *optinfo = plopt; + return pl->plugin; + } + + plopt++; + } + + pl = pl->next; + } + + return NULL; The surest sign that a programmer has been careless is repeated use of strlen() inside a loop. This should have used something like: if (opt->av_len == plopt->name.av_len && !strcasecmp(opt->av_val, plopt->name.av_val)) 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'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. 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.

Here is a plugin (together with fully patches rtmpdump sources) that uses the API: http://users.tkk.fi/~aajanki/rtmpdump-yle/yle-dl-1.99.1.tar.gz

The plugin does custom authentication for http://areena-beta.yle.fi/ng/areena site. An example command for testing the plugin:

rtmpdump -r "rtmp://flashu.yle.fi/mediakanta//x swfUrl=http://areena-beta.yle.fi/static/player/1.1/flowplayer/flowplayer.cluster-3.... tcUrl=rtmp://flashu.yle.fi/mediakanta/ pageUrl=http://areena-beta.yle.fi/ng/areena/tv/1370427/#/play yle=2f4d1d102a34462696ad23573adfd296" -o video.flv

Antti