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

[Michael Niedermayer]

On Tue, Feb 03, 2009 at 09:43:49PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Feb 3, 2009 at 1:35 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Tue, Feb 03, 2009 at 12:15:47PM -0500, Ronald S. Bultje wrote:
> >> Hi,
> >>
> >> On Tue, Feb 3, 2009 at 8:31 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> > to be honest i really think the rtp/rtsp code needs MUCH more cleanup
> >> > and documentation done before any new additions not less and later /
> >> > a promise to do 1 of 100 cleanups afterwards
> >>
> >> You want documentation of struct members, or cleanup of code?
> >
> > i want them documented then when i understand the code i can say if it
> > needs a cleanup.
> 
> Attached are 3 patches that (in rtsp.h):
> - add comments
> - reindent a few comments
> - group a few comments so they are not completely useless
> 

> Any other files that need comments? rtp.h seems pretty good so far,

compared to what?
/dev/random?

no IMO rtp.h is very poorly documented

besides
i picked a random field and it seemd unused
all unused fields should be removed ...


> and rdt.h is fully documented already. Can you tell me what code you'd
> like documented?

all that isnt documented


> 
> Ronald

> Index: ffmpeg-svn/libavformat/rtsp.h
> ===================================================================
> --- ffmpeg-svn.orig/libavformat/rtsp.h	2009-02-03 16:14:46.000000000 -0500
> +++ ffmpeg-svn/libavformat/rtsp.h	2009-02-03 21:12:56.000000000 -0500
> @@ -52,8 +52,8 @@
>      int server_port_min, server_port_max; /**< RTP ports */
>      int ttl; /**< ttl value */
>      uint32_t destination; /**< destination IP address */
> -    int transport;
> -    enum RTSPLowerTransport lower_transport;

> +    int transport;                           /**< RTP or RDT (Real) */

an int holds integers "RTP" "RDT (Real)" are not integers
thats the kind of comment that i do not need


> +    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?
and session_id?


>  } RTSPHeader;
>  

>  enum RTSPClientState {
> @@ -83,15 +83,15 @@
>  };
>  
>  enum RTSPTransport {
> -    RTSP_TRANSPORT_RTP,
> -    RTSP_TRANSPORT_RDT,
> +    RTSP_TRANSPORT_RTP, /**< Standards-compliant RTP packets */
> +    RTSP_TRANSPORT_RDT, /**< Realmedia Data Transport (RDT) packets */
>      RTSP_TRANSPORT_LAST
>  };

RTSP_TRANSPORT_LAST is the only that needs an comment i thing the others
seem obvious, but then commenting them as well doesnt hurt


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


>  
>      enum RTSPClientState state;
>      int64_t seek_timestamp;

no comments for these?


> @@ -100,11 +100,11 @@
>      //    ByteIOContext rtsp_gb;
>      int seq;        /* RTSP command sequence number */
>      char session_id[512];
> -    enum RTSPTransport transport;
> -    enum RTSPLowerTransport lower_transport;
> -    enum RTSPServerType server_type;

> +    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
and the abbreviated company names are not enum values though this is
very minor


>      char last_reply[2048]; /* XXX: allocate ? */

hmm


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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- 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/35e5a4b2/attachment.pgp>