On Thu, 2010-01-14 at 23:01 +0100, Daniel Stenberg wrote:
> On Tue, 12 Jan 2010, Chris Conroy wrote:
>
> > I have finished rebasing to CVS and updating per your previous comments.
>
> Okej, I cleaned it somewhat and attached here is my updated version of your
> patch. I think we're closing in to something we can commit and work on further
> against CVS, but I have a few things I'd like to mention/discuss first:
>
> * lib/rtsp.c at line 536 calls Curl_client_write() with CLIENTWRITE_RTP as
> the only bit. That's the only place that uses CLIENTWRITE_RTP afaics. Thus,
> I think extending Curl_client_write() to handle CLIENTWRITE_RTP as an
> additional bit is pointless. It could just as well be a separate and RTSP-
> specific function that does the callback. Also, the rtsp.c code doesn't
> check the return code from Curl_client_write() there which is a big mistake.
Good points all. I'll change this accordingly.

>
> * CURLOPT_RTPFUNCTION. I'm not happy with this protocol-specific callback with
> data. libcurl in general tries to abstract the protocol and provide data,
> and thus I would rather like to see this function as "alternate data stream"
> or similar to allow for other (future?) protocols that can provide
> alternative or additional data. Also, in that same spirit, I don't like how
> the prototype is RTP-specific with the channel as a separate argument when
> we can just as well decrement the pointer with 3 bytes and document that the
> data passed on to the callback always begin with [1 byte channel][2 bytes
> length]. That way the same proto as the regular write function could be used
> and it would also easily apply to other protocols.
This is probably the cleanest way to do it. There's a slight hassle for
the application but it does clean up the API. I'll get on it..

> * The rtsp.c code uses the hardcoded number 0x24 on multiple places with no
> explanation. It would be good with a comment or a #defined value!
My apologies. I think there's at least one or two comments there, but it
refers to the ASCII code for '$' which signals the start of interleaved
RTP.

>
> * CURLOPT_RTSP_VERSION still makes no sense to me. I suggest we just scrap it.
> There's no point in adding code for something that isn't used today and
> won't be used in the forseeable future.
Sure thing.

>
> * I modified a change you did on line 673 in rtsp.patch about the Expect:
> headers as I believe the logic was broken. I restored the earlier logic but
> with the new function names.
Whoops, good catch. This isn't actually my change but a bad merge. The
17.9.7 code is what I was working off of originally and the logic
changed between that and CVS.

--Chris

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