[rtmpdump] Proposal: dynamically loadable plugins

[Howard Chu]

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.2.3.swf
> tcUrl=rtmp://flashu.yle.fi/mediakanta/
> pageUrl=http://areena-beta.yle.fi/ng/areena/tv/1370427/#/play
> yle=2f4d1d102a34462696ad23573adfd296" -o video.flv
>
> Antti