On Mon, 2010-01-11 at 00:43 +0100, Daniel Stenberg wrote:
> + * Currently only CURL_RTSP_VERSION_1_0 is supported
> + */
> + CINIT(RTSP_VERSION, LONG, 190),
>
> This seems unnecessary to add if you only support one version anyway. Besides,
> in the code you do:
>
> + result = add_bufferf(req_buffer,
> + "%s %s RTSP/1.0\r\n" /* Request Stream-URI RTSP/1.0 */
>
> ... which doesn't care about the option anyway!
You are 100% correct that the option is useless right now. However, that
is by design since there is only one RTSP RFC. I have a somewhat more
instructive comment in curl.h:

/*
 * These enums are for use with the CURLOPT_RTSP_VERSION option.
 * However, currently there is only one version of the protocol.
 * Application developers wishing to safeguard against a future protocol
 * spec should specify CURL_RTSP_VERSION_1_0 explicitly
 */

If you like, I can certainly remove it. The intention is to allow for
the possibility of a new RTSP version to be introduced with new behavior
and not break any applications that aren't expecting such changes. It
may simply be better to make such applications deal with this when the
time comes.

Let me know either way what your preference here is.

>
> + /* Let the application define a custom write method for RTP data
> + * This is like the other write callbacks with an extra parameter
> + * for the RTP channel.
> + */
> + CINIT(RTP_FUNCTION, FUNCTIONPOINT, 194),
>
> Does RTSP use the regular write callback for something else and the RTP one
> for just RTP data? I also would prefer if you removed the underscore in that
> name to match other *FUNCTION names and your RTPDATA counterpart.
Yes. RTSP uses the regular callback but RTP is "special". I am
conflicted about introducing a new function prototype for the RTP write
callback, but there needs to be some way to demultiplex on the channel.
I'm certainly open to alternative mechanisms if you can think of any
that are less ugly :P

> Regarding several (all?) of the HTTP-private functions you made global within
> the library: all such functions with libcurl are to be prefixed with Curl_
> (upper case C) to make it obvious to readers and to reduce the risk of the
> symbols causing problems on environments that will show those names to the
> outside.

I'm going to ahead and assume this includes data types as well. Sorry
for not thinking of this, but my environment doesn't leak symbols like
that!

> I would also like to see your 'struct RTSP' definition get moved to 'rtsp.h'
> in the same manner of how we do for all protocols in the current code. It
> makes it easier to find protocol-specific details and code.
Yes, this makes sense. I was simply following HTTP's lead here and found
it odd when I wrote it.

>
> + /* convert from the network encoding using a scratch area */
> + char *scratch = calloc(1, strlen(s)+1);
> + if(NULL == scratch) {
> + failf (data, "Failed to calloc memory for conversion!");
> + return FALSE; /* can't return CURLE_OUT_OF_MEMORY so return FALSE */
> + }
> + strcpy(scratch, s);
>
> This would be shorter, faster and nicer to instead just use strdup().
Again, I was just following HTTP's lead here. It's a straight cut and
paste from the HTTP protocol check. I'll fix it up for both protocols.

>
> else if(data->cookies &&
> + !(conn->protocol & PROT_RTSP) &&
> checkprefix("Set-Cookie:", k->p)) {
>
> I'm curious about this change. Does RTSP support cookies somehow but you
> disable that support here? Or why do you feel the need to make sure
> Set-Cookie: doesn't work with RTSP?
That was an errant change. RTSP doesn't support cookies, but if the user
feels like using them I guess there is no reason to prohibit it.

>
> I think we should also work slightly more on moving RTSP (and HTTP) specific
> code out from generic areas (such as lib/transfer.c) into their own source
> files and use function calls or perhaps even function pointers in the handler
> struct for them instead. This to reduce the protocol-specific chatter in
> generic code and to make it easier to find the protocol-specific code when
> you're browsing the code for it.
Agreed. Currently working on this and rebasing the changes to CVS.

Thanks for your comments. Once the new patch is ready I'll send it out.

--Chris

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2010-01-11