[PATCH] AMF Object Callback
Hello, the attached patch is a re-submit of one I posted earlier. It adds an AMF callback method that lets the client application work directly with received AMF objects to parse them for information like LLNW or Akamai authentication instead of having to build custom stuff like that directly into rtmpdump. Please let me know if the patch is bad or you find something wrong with it. Thanks.
Chris Larsen wrote:
Hello, the attached patch is a re-submit of one I posted earlier. It adds an AMF callback method that lets the client application work directly with received AMF objects to parse them for information like LLNW or Akamai authentication instead of having to build custom stuff like that directly into rtmpdump. Please let me know if the patch is bad or you find something wrong with it. Thanks.
Making a global callback like this is terrible. How are apps supposed to manage this? What if one library adds a callback and then another library or app tries to add a different callback? What if different threads talking to different servers want to each use their own callbacks? Static global variables that might get manipulated from multiple threads are absolutely forbidden. Changing the global library state like this is forbidden. Doing either of these things invites all kinds of crashes and other mysterious problems. A patch that extends the RTMP structure might be acceptable. The AMF_Dump function is primarily a debug function, you should not abuse it this way. The feature presented here is totally unusable.
Static global variables that might get manipulated from multiple threads are absolutely forbidden. Changing the global library state like this is forbidden. Doing either of these things invites all kinds of crashes and other mysterious problems.
A patch that extends the RTMP structure might be acceptable. The AMF_Dump function is primarily a debug function, you should not abuse it this way.
Good points. I moved the callback pointer to the RTMP struct as you said so that it's thread safe. It is initialized to NULL when RTMP_Init() is called. Then I moved the calls to the HandleInvoke and HandleMeta methods. RTMP_Close() will NULL out the pointer and I added another method that an app can call to explicitly release the callback. Please let me know what you think and thank you.
Chris Larsen wrote:
Static global variables that might get manipulated from multiple threads are absolutely forbidden. Changing the global library state like this is forbidden. Doing either of these things invites all kinds of crashes and other mysterious problems.
A patch that extends the RTMP structure might be acceptable. The AMF_Dump function is primarily a debug function, you should not abuse it this way.
Good points. I moved the callback pointer to the RTMP struct as you said so that it's thread safe. It is initialized to NULL when RTMP_Init() is called. Then I moved the calls to the HandleInvoke and HandleMeta methods. RTMP_Close() will NULL out the pointer and I added another method that an app can call to explicitly release the callback. Please let me know what you think and thank you.
So I guess this list has become the C Programming workshop. Except for very trivial cases, I don't see that the current approach is of much use. If you wanted to write a callback that intercepts a particular type of message and computes a reply to send back, the callback function at least needs to also get the RTMP * pointer as an argument. If you want to write a callback that accesses some other variables in the program, you at least need to give it a context pointer that can be set to point to a structure containing whatever other data it needs. 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) 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. 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.
So I guess this list has become the C Programming workshop.
Except for very trivial cases, I don't see that the current approach is of much use. If you wanted to write a callback that intercepts a particular type of message and computes a reply to send back, the callback function at least needs to also get the RTMP * pointer as an argument. If you want to write a callback
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. that
accesses some other variables in the program, you at least need to give it a context pointer that can be set to point to a structure containing whatever other data it needs.
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
I've been using this callback in a very trivial fashion to grab and format status codes and parse the strings for authentication information, something that is unique to various CDNs. So I apologize for not thinking about more advanced uses. The only other ways to get such information out of libRTMP is to use the logging callback or parse stderr. However I was using static global variables which, as you pointed out, is a bad idea so certainly passing a context pointer of some kind makes much more sense. 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.
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 } } } } Thank you for your feedback.
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.
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. 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. Thanks, adammw111 On Sat, Jul 30, 2011 at 11:29 AM, Howard Chu <hyc@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. _______________________________________________ rtmpdump mailing list rtmpdump@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/rtmpdump
-- Adam Malcontenti-Wilson
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@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.
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@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@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
Thank you very much for your post Adam, it's a huge help and now I know a bit more about what Howard's after. I modified your first patch with the linked list and such as Howard specified.
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.
I don't know if the response needs to be a union since we're only creating a callback for AMF data and not other types, but if ya'll want to use a union that's cool.
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.
It is nice to have the ability for the callback to return a number of different responses so I implemented Howard's types and added some logging lines to let the user know exactly what the callback did within libRTMP.
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?
Unfortunately I don't have any public servers to use (and I would have included the authentication code directly in HandleInvoke but an NDA is preventing me, hence the desire for a callback). But I did run tests with this patch on Windows and Ubuntu and it worked properly and let me perform the authentication I needed as well as extract status codes for use in my app. This version also blocks duplicate callback subscriptions. It will also execute all callbacks until one callback tells it to abort. That way you can have a logging callback that watches all messages and then a separate one that takes action on a certain type. Let me know what ya'll think, thanks.
Hi Chris (and others), I've looked over it and it appears ok, it essentially replicates the second version of my patch (named librtmp_callback_v6.patch) - I'm not sure which is better, but I can see that some parts are different. I'm assuming you wrote this as a modification of my first patch, so you might not have included some of the changes in the second. If we can combine both this and my second patch to get one we can all agree on it would be good. More comments inline. On Wed, Aug 3, 2011 at 1:56 AM, Chris Larsen <clarsen@euphoriaaudio.com> wrote:
Thank you very much for your post Adam, it's a huge help and now I know a bit more about what Howard's after. I modified your first patch with the linked list and such as Howard specified.
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.
I don't know if the response needs to be a union since we're only creating a callback for AMF data and not other types, but if ya'll want to use a union that's cool.
The idea is simply because if we wanted to have a callback that returned something other than AMF, we'd have to make new callback prototype. By using a RTMPCallbackResponse struct, we can hold whatever we may need in the future with the same prototype. I think this is the biggest difference between our two patches.
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.
It is nice to have the ability for the callback to return a number of different responses so I implemented Howard's types and added some logging lines to let the user know exactly what the callback did within libRTMP.
I'm not really sure about the difference between the two, sounds more like a problem on agreeing what a "default" success should actually do and really just a name change. I'm not sure about doing the check of the status code again within HandleInvoke and HandleMetadata just for logging but I guess it's ok.
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?
Unfortunately I don't have any public servers to use (and I would have included the authentication code directly in HandleInvoke but an NDA is preventing me, hence the desire for a callback). But I did run tests with this patch on Windows and Ubuntu and it worked properly and let me perform the authentication I needed as well as extract status codes for use in my app.
This version also blocks duplicate callback subscriptions. It will also execute all callbacks until one callback tells it to abort. That way you can have a logging callback that watches all messages and then a separate one that takes action on a certain type.
I don't understand what you mean here extactly, wasn't this possible in my first patch?
Let me know what ya'll think, thanks.
_______________________________________________ rtmpdump mailing list rtmpdump@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/rtmpdump
Thanks, adammw111 -- Adam Malcontenti-Wilson
Thanks Adam, sorry for the delay, tied up with work.
I don't know if the response needs to be a union since we're only creating a callback for AMF data and not other types, but if ya'll want to use a union that's cool.
The idea is simply because if we wanted to have a callback that returned something other than AMF, we'd have to make new callback prototype. By using a RTMPCallbackResponse struct, we can hold whatever we may need in the future with the same prototype. I think this is the biggest difference between our two patches.
I put the union back into my patch and test it out so it works and makes sense for a more generic callback as opposed to just AMF.
It is nice to have the ability for the callback to return a number of different responses so I implemented Howard's types and added some logging lines to let the user know exactly what the callback did within libRTMP.
I'm not really sure about the difference between the two, sounds more like a problem on agreeing what a "default" success should actually do and really just a > name change. I'm not sure about doing the check of the status code again within HandleInvoke and HandleMetadata just for logging but I guess it's ok.
I left this as is for now but we can change it if necessary
This version also blocks duplicate callback subscriptions. It will also execute all callbacks until one callback tells it to abort. That way you can have a logging callback that watches all messages and then a separate one that takes action on a certain type.
I don't understand what you mean here extactly, wasn't this possible in my first patch?
Sorry, yeah, your version did to. Let me know what else needs to be changed and we'll see what Howard says. Thanks!
Hi Chris, Just a couple of nitpicks reading though the patch: 1. You seem to use "struct RTMPCallbackData" a lot instead of just "RTMPCallbackData" - it's typedef'd so may as well use it to be consistent 2. RTMP_Init already memset's the RTMP structure to 0, so the + r->callback.cbd_len = 0; + r->callback.cbd_val = NULL; are superfluous. 3. txn should already be int so casting is not required at r->m_methodCalls[i].num == (int)txn, rather it is required when setting the result of AMFProp_GetNumber as it returns a double. 4. If your going to be checking the status code in the HandleInvoke / HandleMetadata sections, it would be better to use if != RTMP_CB_CONTINUE rather than != FALSE. 5. I'm not sure if RTMPCallbackList is really needed with a linked list implementation, although it is good to store the length (if kept in sync). This is probably a performance/memory issue which I would think Howard would know more about this than I do... 6. RTMP_AttachCallback doesn't have a function prototype, I think it needs to replace the older RTMP_SetCallback prototype. My bad. 7. If I'm reading RTMP_CallCallback correctly, wouldn't RTMP_CB_CONTINUE make the log say "no matching callbacks found" even if some have already ran? 8. This one doesn't really matter, but I personally prefer to use a separate "RTMP_CallAMFCallback" to use for HandleInvoke and HandleMetadata just so we can remove as much logging/return code checking logic out of there as we can. Also means that we don't need RTMPCallbackResponse in the entire HandleInvoke / HandleMetadata scope, and if the RTMPCallbackResponse ever changes (which I hope it wouldn't) that you would only need to make changes in RTMP_CallAMFCallback and not everywhere it is called. For example, RTMP_CallAMFCallback could return TRUE or FALSE, and one state would free and return, the other would continue so HandleMetadata's logic would be as short as: if (RTMP_CallAMFCallback(r, &obj, RTMP_CB_FILTER_META)) { AMF_Reset(&obj); return 0; } Also, I noticed that in both RTMP_ReleaseCallback is a void function. Perhaps it would be good to return the success/failure, although even on failure there's not much an external library can do... I might not have been very clear what I talking about here so just reply for any clarification, or your thoughts on this. Hopefully these are the last set of changes we need to make to get it implemented, and the majority of these changes are minor. Thanks, Adam On Tue, Aug 23, 2011 at 7:52 AM, Chris Larsen <clarsen@euphoriaaudio.com> wrote:
Thanks Adam, sorry for the delay, tied up with work.
I don't know if the response needs to be a union since we're only creating a callback for AMF data and not other types, but if ya'll want to use a union that's cool.
The idea is simply because if we wanted to have a callback that returned something other than AMF, we'd have to make new callback prototype. By using a RTMPCallbackResponse struct, we can hold whatever we may need in the future with the same prototype. I think this is the biggest difference between our two patches.
I put the union back into my patch and test it out so it works and makes sense for a more generic callback as opposed to just AMF.
It is nice to have the ability for the callback to return a number of different responses so I implemented Howard's types and added some logging lines to let the user know exactly what the callback did within libRTMP.
I'm not really sure about the difference between the two, sounds more like a problem on agreeing what a "default" success should actually do and really just a > name change. I'm not sure about doing the check of the status code again within HandleInvoke and HandleMetadata just for logging but I guess it's ok.
I left this as is for now but we can change it if necessary
This version also blocks duplicate callback subscriptions. It will also execute all callbacks until one callback tells it to abort. That way you can have a logging callback that watches all messages and then a separate one that takes action on a certain type.
I don't understand what you mean here extactly, wasn't this possible in my first patch?
Sorry, yeah, your version did to.
Let me know what else needs to be changed and we'll see what Howard says. Thanks!
_______________________________________________ rtmpdump mailing list rtmpdump@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/rtmpdump
-- Adam Malcontenti-Wilson
participants (3)
-
Adam Malcontenti-Wilson -
Chris Larsen -
Howard Chu