[rtmpdump] [PATCH] AMF Object Callback

[Adam Malcontenti-Wilson]

Updated the patch and examples as per suggestions, as well as changing
the callback parameter from an AMFObject to a more generic, extendible
custom struct with a union, similar to how AMFObjectProperty stores
data, so future callback hooks can return something other than an
AMFObject.

As for comment 2, I disagree here. The code distinguishes between the
three states, however the default is that an error in the callback
will continue librtmp's processing rather than aborting it, as more
often when an error occurs you'd want librtmp to do the sane thing
rather than dropping the packet completely. However, for when that's
needed I've also included RTMP_CB_ERROR_ABORT return code. So just to
list:
1. callback did everything and succeeded, so main processing should
stop: RTMP_CB_ABORT
2. callback failed and main processing must stop: RTMP_CB_ERROR_ABORT
3. callback wants to allow main processing to continue:
RTMP_CB_SUCCESS (or any other error code, however only RTMP_CB_SUCCESS
will not generate a log warning)
There's also RTMP_CB_ERROR_INTERNAL which is not really implemented in
my patch, but for things like bad pointer to callback, etc. which
would also continue.
Because of there's only really two states librtmp would care about
(whether to stop processing or not), that's why RTMP_CallCallback
returns TRUE (stop) or FALSE (continue).

I haven't tested my linked list too thoroughly so you might want to
check I haven't done something stupid (that's why the patch is v6 and
not v5).

For testing, can rtmpsrv be used (or modified) to test auth schemes
using callbacks, or is it too much of a 'stub'? Or do we have some
list of public rtmp servers using custom crypto to test this on?

Thanks,
adammw111

On Sun, Jul 31, 2011 at 4:54 PM, Howard Chu <hyc at highlandsun.com> wrote:
> 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.
>



-- 
Adam Malcontenti-Wilson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: example1.c
Type: text/x-csrc
Size: 2467 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/rtmpdump/attachments/20110802/bcff438f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: example2.c
Type: text/x-csrc
Size: 3524 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/rtmpdump/attachments/20110802/bcff438f/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: librtmp_callback_v6.patch
Type: application/octet-stream
Size: 7185 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/rtmpdump/attachments/20110802/bcff438f/attachment-0001.obj>