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.

* 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.

* 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!

* 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.

* 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.

-- 
  / daniel.haxx.se

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html