[rtmpdump] [PATCH] AMF Object Callback

[Howard Chu]

Adam Malcontenti-Wilson wrote:
> Hi,
>
> I've been reading the discussion and tried to make some of the changes
> suggested, and ended up changing the patch a bit to allow it to be
> more general/extendible but it's still based on the original idea from
> Chris. I've attached both the patch, and two examples of how the
> callback could be used, but they're not extensively tested as I don't
> really have a server I can test custom authentication on, and I should
> note that I am also not a professional C coder but I do not expect
> this to be a workshop, rather a way of improving librtmp.

Overall this is closer to what I'd expect to see, nice work.

And going off on a tangent - the only distinction between "professional" and 
"amateur" is simply that a professional does a piece of work for the sole 
purpose of being paid. Trust me when I say there are no professionals on this 
project. Note that that's absolutely not a commentary on quality of work; in 
fact I'd say the average quality of work produced by professional programmers 
is far inferior to the average quality of amateurs. 99% of the programming 
I've done for the past dozen years has been "amateur" - unpaid work on open 
source projects. Only a handful of items have actually been contracted/paid 
for. This is an important point to me - IMO people who pursue a task for their 
own personal reasons produce consistently better work than people who do 
something simply because it's a job that they get paid for.
/end tangent

> The patch supports multiple callbacks using RTMP_AttachCallback and
> some rudimentary filtering depending on whether you want messages that
> normally go to HandleInvoke or HandleMetadata.

Only a couple issues with this cut.

1) You malloc the callback data and you realloc a list of pointers to that 
data. This is pretty inefficient, you should have just used a linked-list of 
callback structures. realloc is one of the worst functions around and should 
be avoided as much as possible.

2) You just return a true/false result from the callback function itself. 
That's not sufficient, you want to be able to distinguish the three states 
that I outlined in my previous feedback. I.e., callback did everything and 
succeeded, so main processing should stop; callback failed and main processing 
must stop, or callback wants to allow main processing to continue. It's always 
important to let the caller know when something actually fails, and to let it 
have a meaningful error code from the failure.

3) In your examples: the reason we use typedefs is to keep things consistent. 
So when you go to the trouble of defining one, you should actually use it.

Instead of declaring your example's prototype:

int callback(RTMP * r, AMFObject * obj, void * ctx);

You should have just done:

RTMPCallback callback;


> Thanks,
> adammw111
>
> On Sat, Jul 30, 2011 at 11:29 AM, Howard Chu<hyc at highlandsun.com>  wrote:
>> Chris Larsen wrote:
>>>>
>>>> So I guess this list has become the C Programming workshop.
>>>
>>> Hey, I'm here to learn and to try and help the community. I do work mostly
>>> in C++ so I'm not a brilliant C coder.
>>
>> I'm not here to teach newbies, I've got more rewarding ways to spend my
>> time. Sign up for a course, go pay for a tutor, whatever. I didn't get
>> involved in this project, writing code in the hopes that beginners will
>> learn from it.
>> I write code that aims to solve my problems, as best as possible. I don't
>> want code from newbies coming in either, code that hasn't been fully thought
>> through. If you want to play in this sandbox, you need to be able to keep
>> up, think for yourself, with no hand-holding. I've already spent more time
>> on this email thread than I should.
>>
>>>> Also, if you want to intercept a particular message and then act on it,
>>>
>>> you probably need to be able to return a result code. I would say
>>>>
>>>>         #define AMF_CB_CONTINUE 0xffff  /* continue with normal processing
>>>
>>> */
>>>>
>>>>         #define AMF_CB_SUCCESS  0       /* callback did everything, stop
>>>
>>> processing */
>>>>
>>>>         /* any other value: error code, stop processing */
>>>
>>>> So your prototype should look like:
>>>>         typedef int (AMF_ObjectCallback)(RTMP *, AMFObject *, void *);
>>>> With
>>>>         void RTMP_SetAMFCallback(RTMP *r, AMFObject *obj, void *ctx)
>>>
>>> Regarding the return code, since the onus is on the client to take any
>>> actions it needs depending on the object, does libRTMP really care whether
>>> the client found what it needed to? The client should just return when
>>> it's
>>> done with the object and libRTMP will continue on it's way. A return code
>>> could be used for logging so I could add that if you want me to. I'll add
>>> the RTMP and context pointers though.
>>
>> Obviously you haven't read thru HandleInvoke() very carefully. For various
>> messages it immediately triggers a reply. If you're trying to write a
>> callback to handle a new authentication secret, or some other interesting
>> keyword, you need to be able to slot in somewhere in the processing flow.
>> Ideally you would want to just insert the callback behavior inside, before
>> any other replies are generated, otherwise you need to duplicate a lot of
>> functionality if you want to generate your own reply.
>>
>> E.g., look at how SecureToken is handled in HandleInvoke. It has to be
>> processed before the RTMP_SendCreateStream() call. Or look at
>> SendUsherToken, which gets processed *after* RTMP_SendCreateStream().
>> Basically, if you want to be able to write arbitrary callbacks to handle
>> arbitrary new security mechanisms down the road, HandleInvoke needs to be
>> completely gutted and redesigned to allow that.
>>
>>>> The other problem with all of this is that it requires the callback to
>>>
>>> duplicate a lot of librtmp's parsing before it can discover if it actually
>>> needs to do anything.
>>>
>>> All of the parsing has been completed before the callback is executed. The
>>> client just has to include amf.h and it can do whatever is necessary. I
>>> didn't touch the AMF_Dump calls but I could wrap them in an else statement
>>> so that if the client has set a callback, the dumps are not performed. Let
>>> me know if I should do that.
>>>
>>>> So again, the question is - what good is this? How do you actually expect
>>>
>>> to be able to use this feature? Give an actual example, one that would
>>> actually work,
>>>>
>>>> given what you've proposed.
>>>
>>> The example I have implemented, as has been talked about on the list
>>> before,
>>> is that of authenticating to an ingest FMS server where the CDN has
>>> written
>>> a custom module to authorize publishing.  It's pretty much the same as the
>>> AMF_Dump function. Some of the CDNs use a multiple connect approach where
>>> they send an error message with a token and then you have to reconnect
>>> using
>>> that token. When the initial connect fails, my client checks to see if a
>>> token was received and if so, attempts to reconnect.
>>>
>>> void MyClass::AMFCallback(RTMP *r, AMFObject *obj, void *ctx){
>>>    for (int n = 0; n<    obj->o_num; n++){
>>>      if (obj->o_props[n].p_type == AMF_STRING&&
>>> obj->o_props[n].p_name.av_len>    0
>>>        &&    strncmp(obj->o_props[n].p_name.av_val, "description", 11) == 0
>>>        &&     obj->o_props[n].p_vu.p_aval.av_val != NULL){
>>>
>>>        std::string temp = obj->o_props[n].p_vu.p_aval.av_val;
>>>        if (temp.find("authentication required notice") != temp.npos){
>>>            MyClass *client = (MyClass *)ctx;
>>>            client->auth_value = temp; // parsed of course to get the actual
>>> value
>>>        }
>>>      }
>>>    }
>>> }
>>
>> So in your example you're saving some data into a static global variable,
>> and the main app crunches it and adds some resulting info to its next
>> Connect request. I guess that works.
>>
>> Since we're discussing adding a general extension to the library, I would
>> have envisioned something where the callback contains all of the logic
>> needed to complete a step (such as authenticating to an FMS server). Then
>> the callback can be wrapped inside a dynamically loaded plugin, and used
>> (almost) transparently by multiple apps. With your approach, every app has
>> to copy your code for operating the callback.