[FFmpeg-devel] [PATCH] document rtsp.h

[Michael Niedermayer]

On Thu, Feb 05, 2009 at 05:41:38PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Thu, Feb 5, 2009 at 12:27 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Thu, Feb 05, 2009 at 09:24:21AM -0500, Ronald S. Bultje wrote:
> >> I can add more newlines if you think it's unreadable this way, but
> >
> > please do
> 
> I added a few. Not as many as AVCodecContext, but about as often as
> AVFormatContext. Should be OK.
> 
> >> +/**
> >> + * Network layer over which RTP packet data will be transported.
> >> + */
> >>  enum RTSPLowerTransport {
> >
> > only RTP? not also RDT, ...
> 
> All. I replaced it with RTP/etc. Wouldn't want to put too much effort
> on RDTs existence.
> 
> >> +/**
> >> + * Packet protocol
> >
> > is this a standard term? or self invented?
> 
> RFC3551 calls it a profile, so I renamed it to "Packet profile".
> 

> >> +    /**
> >> +     * This is not part of the public API and shouldn't be used outside ffmpeg.
> >> +     */
> >>      RTSP_TRANSPORT_LAST
> >
> > this stateent makes it look like the rest is part of the public API, is
> > it so?
> 
> Not sure, really. I'm hoping we can stop installing rtsp.h, then it
> isn't and the comment can be removed. Luca? I guess we should remove
> the comments once we stop installing rtsp.h.

what breaks if its not installed? users really have no business bypassing
libav* API and acces demuxers behinds its back


> 
> >> +    /** UDP multicast port range; the ports to which we should connect to
> >> +     * receive multicast UDP data. */
> >> +    int port_min, port_max;
> >
> > shouldnt the words "udp" and "multi" occur in the names?
> 
> I think the variable refers to the name of the variable in the
> "Transport:" line. Here's it's called "port" for udp/multicast, and
> "client_port" and "server_port" for unicast/udp. I can rename it to
> udp_multicast_port_min/max if wanted.


> 
> >> +    /** UDP client ports; these should be the local ports of the UDP RTP
> >> +     * (and RTCP) sockets over which we receive RTP/RTCP data. */
> >> +    int client_port_min, client_port_max;
> >
> > what about TCP?
> 
> We don't store the TCP port number anywhere because it is irrelevant
> to the setup of the session (it's already set-up since it is the very
> same connection as the SDP/RTSP one). This variable (and the one
> below/above) is UDP only.
> 

> >> +/**
> >> + * This describes the server response to each RTSP command.
> >> + */
> >
> >>  typedef struct RTSPHeader {
> >
> > poor name ...
> 
> I can rename it if you suggest something better. :-).

ServerResponse assuming your comment is correct
an alternative would be ReceivedRTSPHHeader


> 
> >> +    /**< the "RealChallenge1:" field from the server */
> >> +    char real_challenge[64];
> >
> > /**< ?
> 
> Fixed, thanks.
> 
> >> +    RTSP_STATE_PLAYING, /**< initialized and streaming data */
> >> +    RTSP_STATE_PAUSED,  /**< initialized, but not streaming data */
> >
> > s/streaming/receiving/ ?
> 
> Also fixed.
> 
> >> + * standards-incompliant Transport: lines in the SETUP request.
> >
> > "Transport:"
> 
> Also fixed.
> 

> >> +    /** whether we are currently receiving data from the server */
> >>      enum RTSPClientState state;
> >
> > what is state?
> > whether we are currently receiving data from the server
> >
> > didnt work out
> 
> I'm not sure what you want me to add. I added how the variable is
> changed but my guess is you want something else. sorry, I'm not
> completely sure what information you're looking for here...

the variable is called state.
Add a text that fits after the question
What is the variable state?


> 
> >> +    /** The following are used for dynamic/private protocols (payloads) */
> >> +    //@{
> >> +    /** handler structure */
> >> +    RTPDynamicProtocolHandler *dynamic_handler;
> >> +    /** private data associated with the dynamic protocol */
> >> +    PayloadContext *dynamic_protocol_context;
> >> +    //@}
> >
> > whats a dynamic/private protocol ?
> 
> A private protocol is one with a payload ID (in the SDP description or
> RTP header) above 96. Anything below 96, the mediatype for the payload
> ID is fixed. Anything above, it's "private" and is mapped to a
> mediatype in the rtpmap: SDP line. In the end, the mediatype is the
> string given in each RTPDynamicProtocolHandler, e.g. "x-pn-realaudio"
> for Realaudio-in-RDT, etc.
> 
> A dynamic protocol is what we in ffmpeg implement outside rtpdec.c, in
> practice, so it's one of those that implements a
> RTPDynamicProtocolHandler (a "plugin" to rtpdec.c). These are commonly
> the same as the private ones. Anyway, to not confuse anyone, I removed
> the "private", since it literally really refers just to dynamic
> protocols.

why are not all dynamic?


[...]
> +/**
> + * Packet profile of the data that we will be receiving. Real servers
> + * commonly send RDT (although they can sometimes send RTP as well),
> + * whereas most others will send RTP.
> + */
>  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.
> +     */
>      RTSP_TRANSPORT_NB
>  };
>  

as said i dislike this because it pretends that the rest of this file is
public API


[...]
> +/**
> + * Client state, i.e. whether we are currently receiving data (PLAYING) or
> + * setup-but-not-receiving (PAUSED). State can be changed in applications
> + * by calling av_read_play/pause().
> + */
>  enum RTSPClientState {
> -    RTSP_STATE_IDLE,
> -    RTSP_STATE_PLAYING,
> -    RTSP_STATE_PAUSED,

> +    RTSP_STATE_IDLE,    /**< not initialized */

RTSP_STATE_NOT_INIT ?


[...]
> +/**
> + * Private data for the RTSP demuxer.
> + */
>  typedef struct RTSPState {
>      URLContext *rtsp_hd; /* RTSP TCP connexion handle */
> +    /** number of items in the 'rtsp_streams' variable */
>      int nb_rtsp_streams;
> -    struct RTSPStream **rtsp_streams;
> +    struct RTSPStream **rtsp_streams; /**< streams in this session */
>  
> +    /** whether we are currently receiving data from the server. This
> +     * value is set to PLAYING after a call to rtsp_read_play() and to
> +     * PAUSED after initialization or after a call to rtsp_read_pause().  */
>      enum RTSPClientState state;

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

do you realize that this is broken ?

pause, seek to 1s from start, play 5h, pause, play

the last play will seek back to 1s
no? then the comment is wrong ...


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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20090206/8f128c5b/attachment.pgp>