[FFmpeg-devel] [PATCH] RTSP-MS 10/15: ASF header parsing

Wed Feb 4 18:23:15 CET 2009

On Wed, Feb 04, 2009 at 09:01:01AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> take 2...
> 
> On Wed, Feb 4, 2009 at 3:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Tue, Feb 03, 2009 at 09:43:49PM -0500, Ronald S. Bultje wrote:
> >> +    enum RTSPLowerTransport lower_transport; /**< TCP/UDP uni-/multicast */
> >
> > well, better than before but primarely comments should document what a field
> > is, this indeed is obvious from the enumeration and name but its not written
> > in the comment.
> >
> >
> >>  } RTSPTransportField;
> >>
> >>  typedef struct RTSPHeader {
> >> @@ -66,7 +66,7 @@
> >>      int seq; /**< sequence number */
> >>      char session_id[512];
> >>      char real_challenge[64]; /**< the RealChallenge1 field from the server */
> >> -    char server[64];
> >> +    char server[64];                 /**< the Server field, to identify WMS */
> >
> > and the structure represents what?
> 
> I documented most (not all, some are pretty obvious so I left those
> out). I also didn't document RTSPActionServerSetup because it is
> unused in the RTSP code, only used in ffserver.c. OK if I move it
> there? (Separate patch, clearly.)
> 
> > and session_id?
> 
> Added.
> 
> >>  typedef struct RTSPState {
> >>      URLContext *rtsp_hd; /* RTSP TCP connexion handle */
> >>      int nb_rtsp_streams;
> >
> >> -    struct RTSPStream **rtsp_streams;
> >> +    struct RTSPStream **rtsp_streams; /**< for each m= line in the SDP */
> >
> > this is not a description of what rtsp_streams is
> > just try:
> > A: what is rtsp_streams?
> > B: for each m= line in the SDP
> >
> > didnt work ...
> 
> I added documentation for RTSPStream which describes (more general,
> multi-line) what they are, is that OK? I think from that description,
> this variable is pretty clear. I removed this weird line and replaced
> it by a simple "streams in session".
> 
> >>      enum RTSPClientState state;
> >>      int64_t seek_timestamp;
> >
> > no comments for these?
> 
> Added.
> 

> >> +    enum RTSPTransport transport;     /**< RTP or RDT */
> >> +    enum RTSPLowerTransport lower_transport; /**< TCP/UDP uni-/multicast */
> >> +    enum RTSPServerType server_type;  /**< brand of server (MS, RM, other) */
> >
> > align
> 
> That becomes really ugly, I have to line-wrap almost all comments. I'd
> like to leave just and only lower_transport unaligned just so I can
> fit more than 2 words on a comment line, please?

why dont you put the comments before the vars like AVCodecContext

> 
> > and the abbreviated company names are not enum values though this is
> > very minor
> 
> Fixed.
> 

> >> -    void *cur_tx;
> >> +    void *cur_tx;                     /**< stream for last packet */
> >
> > where is the typedef for type stream? there is none? then what is a stream?
> > this is a void* so no way to know ...
> 
> Crappy comment, I rewrote it so it's clear what the variable is. I
> already know what your next question will be ("what is
> RTSPStream->tx_ctx?"), but I think that's relatively clearly
> documented.

you mean "RTP/RDT parse context"
i dunno, doesnt sound clear to me, besides you could mention this i the
comment of cur_tx, its always better to say things instead of refereing
to some other place if its just 2-3 words.
besides why is it void*?

> 
> Ronald

> Index: ffmpeg-svn/libavformat/rtsp.h
> ===================================================================
> --- ffmpeg-svn.orig/libavformat/rtsp.h	2009-02-04 08:46:59.000000000 -0500
> +++ ffmpeg-svn/libavformat/rtsp.h	2009-02-04 08:57:24.000000000 -0500
> @@ -38,8 +38,8 @@
>  };
>  
>  enum RTSPTransport {
> -    RTSP_TRANSPORT_RTP,
> -    RTSP_TRANSPORT_RDT,
> +    RTSP_TRANSPORT_RTP, /**< Standards-compliant RTP */
> +    RTSP_TRANSPORT_RDT, /**< Realmedia Data Transport */
>      /**
>       * This is not part of the public API and shouldn't be used outside ffmpeg.
>       */

> @@ -54,6 +54,10 @@
>  #define RTSP_RTP_PORT_MIN 5000
>  #define RTSP_RTP_PORT_MAX 10000
>  
> +/**
> + * This describes the Transport: line of a single stream as negotiated
> + * by the SETUP RTSP command.
> + */

This describes the "Transport:" line ...

>  typedef struct RTSPTransportField {

>      int interleaved_min, interleaved_max;  /**< interleave ids, if TCP transport */

what are interleave ids?

>      int port_min, port_max; /**< RTP ports */
> @@ -61,10 +65,13 @@
>      int server_port_min, server_port_max; /**< RTP ports */
>      int ttl; /**< ttl value */
>      uint32_t destination; /**< destination IP address */
> -    enum RTSPTransport transport;
> -    enum RTSPLowerTransport lower_transport;
> +    enum RTSPTransport transport;            /**< RTP or RDT (Real) */
> +    enum RTSPLowerTransport lower_transport; /**< TCP/UDP uni-/multicast */
>  } RTSPTransportField;
>  

> +/**
> + * This describes the server response to each RTSP command.
> + */
>  typedef struct RTSPHeader {

the comment is clear, no complaint but the struct name ...

>      int content_length;
>      enum RTSPStatusCode status_code; /**< response code from server */
> @@ -73,9 +80,11 @@

>      int64_t range_start, range_end;

comment?

>      RTSPTransportField transports[RTSP_MAX_TRANSPORTS];
>      int seq; /**< sequence number */
> -    char session_id[512];
> +    char session_id[512];            /**< The Session field, value is set
> +                                      * by the server and re-transmitted by
> +                                      * the client in every RTSP command. */

>      char real_challenge[64]; /**< the RealChallenge1 field from the server */
> -    char server[64];
> +    char server[64];                 /**< the Server field, to identify WMS */
>  } RTSPHeader;

iam not fully satisfied by this
server="111.222.123.111"
server="microsoft wurst m?ll schleim 1.0
server="1234-AFSDHDJ128973-JSGF"

what iam trying to say is your comment leaves it quite open what this
field could contain ...

>  
>  enum RTSPClientState {

> @@ -84,6 +93,10 @@
>      RTSP_STATE_PAUSED,
>  };
>  
> +/**
> + * Identifies particular servers that require special handling, such as
> + * standards-incompliant Transport: lines in the SETUP request.
> + */
>  enum RTSPServerType {
>      RTSP_SERVER_RTP,  /**< Standards-compliant RTP-server */
>      RTSP_SERVER_REAL, /**< Realmedia-style server */

nice

> @@ -91,28 +104,44 @@
>      RTSP_SERVER_LAST
>  };
>  
> +/**
> + * Private data for the RTSP demuxer.
> + */
>  typedef struct RTSPState {
>      URLContext *rtsp_hd; /* RTSP TCP connexion handle */
>      int nb_rtsp_streams;
> -    struct RTSPStream **rtsp_streams;
> +    struct RTSPStream **rtsp_streams; /**< streams in this session */
>  
> -    enum RTSPClientState state;
> -    int64_t seek_timestamp;
> +    enum RTSPClientState state;       /**< whether we are currently receiving
> +                                       * data or not from the server */

> +    int64_t seek_timestamp;           /**< the seek value requested when
> +                                       * calling av_seek_frame(). This way,
> +                                       * the seek value is saved if we are
> +                                       * currently paused, and it can be
> +                                       * transmitted at the next PLAY.
> +                                       * See rtsp_read_play(). */

hmm
av_seek_frame() -> set seek_timestamp
1 hour later pause 
2 hours later resume, we here just use the seek_timestamp from av_seek_frame()?
(and iam intentionally not looking at the actual code because the comments
 in the header should make sense wihout ...)

>  
>      /* XXX: currently we use unbuffered input */
>      //    ByteIOContext rtsp_gb;
>      int seq;        /* RTSP command sequence number */
> -    char session_id[512];
> -    enum RTSPTransport transport;
> -    enum RTSPLowerTransport lower_transport;
> -    enum RTSPServerType server_type;
> +    char session_id[512];             /**< same as RTSPHeader->session_id */
> +    enum RTSPTransport transport;     /**< RTP or RDT */
> +    enum RTSPLowerTransport lower_transport; /**< TCP/UDP uni-/multicast */
> +    enum RTSPServerType server_type;  /**< brand of server (WMS/REAL/other) */
>      char last_reply[2048]; /* XXX: allocate ? */
> -    void *cur_tx;
> +    void *cur_tx;                     /**< RTSPStream->tx_ctx of the last
> +                                       * stream that we read a packet from. */

>      int need_subscription;
>      enum AVDiscard real_setup_cache[MAX_STREAMS];
>      char last_subscription[1024];

not commented?

>  } RTSPState;
>  

> +/**
> + * Describes a single stream, as identified by a single m= line block in the
> + * SDP content. In the case of RDT, one RTSPStream can represent multiple
> + * AVStreams. In this case, each AVStream in this set has similar content
> + * (but different codec/bitrate).
> + */
>  typedef struct RTSPStream {
>      URLContext *rtp_handle; /* RTP stream handle */
>      void *tx_ctx; /* RTP/RDT parse context */

I think you should describe this independant of refering to specific
implementations lile RDT

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090204/de5a8d08/attachment.pgp>