[rtmpdump] [PATCH] AMF Object Callback

[Adam Malcontenti-Wilson]

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 at 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 at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/rtmpdump
>
>



-- 
Adam Malcontenti-Wilson