[FFmpeg-devel] [PATCH] RDT/Realmedia patches #2

[Michael Niedermayer]

On Fri, Nov 14, 2008 at 11:12:29AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> 2008/11/12 Michael Niedermayer <michaelni at gmx.at>:
> > On Tue, Nov 11, 2008 at 04:00:42PM -0500, Ronald S. Bultje wrote:
> >> On Tue, Nov 11, 2008 at 2:56 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> > It doesnt matter which applies so much, what matters is that the variable
> >> > where and are completely undocumented and i have absolutely no idea what
> >> > they are without "reverse engeneering" it out of the code.
> >>
> >> OK, this patch adds a comment (and nothing else) on the meaning of
> >> these variables where we parse them.
> [..]
> >> Index: ffmpeg-svn/libavformat/rdt.c
> >> ===================================================================
> >> --- ffmpeg-svn.orig/libavformat/rdt.c 2008-11-11 15:30:12.000000000 -0500
> >> +++ ffmpeg-svn/libavformat/rdt.c      2008-11-11 15:59:36.000000000 -0500
> >> @@ -184,6 +184,21 @@
> >>      }
> >>      if (len < 10)
> >>          return -1;
> >> +    /**
> >> +     * Layout of the header (in bits):
> >
> >> +     * 1:  Length included flag
> >> +     * 1:  Need reliable flag
> >
> > these sound like good variable names, maybe a little abbreviated like
> > len_included need_reliable. But its really too terse as comment explaining
> > things, also it may make sense to put the description on the multimedia
> > wiki ...
> 
> OK, I greatly expanded the documentation, should be all clear now, at
> least as far as it's clear to me. Wiki is also fine although I'd
> prefer slightly to focus on getting stuff in SVN first.
> 
> >> +     * 5:  ID of a set of streams of one media type (e.g. audio of video)
> >
> > Considering the surrounding comments i suspect it should be
> >
> > ID of s set of streams of identical content, though possibly different
> > bitrate, codec, ...
> 
> OK, changed...
> 
> >> +     * 16: sequence number
> >> +     * 1:  back-to-back flag
> >> +     * 1:  slow data flag
> >> +     * 5:  ID of the stream within this particular set. A set can contain
> >> +     *     multiple streams of different codecs and/or bitrates
> >
> >> +     * 1:  keyframe flag (unset if packet belongs to keyframe)
> >
> > id call it non-keyframe flag if 1 indicates non keyframes
> >
> >
> >> +     * 32: timestamp
> >
> > DTS? PTS? PCR/SCR?
> 
> PTS, as in RM. Fixed.
> 
> >>      if (sn)  *sn  = (buf[0]>>1) & 0x1f;
> >>      if (seq) *seq = AV_RB16(buf+1);
> >>      if (ts)  *ts  = AV_RB32(buf+4);
> >
> > these names are too short
> 
> I'll fix variable names in subsequent patches, your first complaint
> was that they're unclear regardless of their name, that's what I'm
> trying to address here.
> 
> You'll also notice that the comments reveal that in practice, the
> rdt.c code works, but we're not handling it exactly according to the
> docs (e.g. we're not handling extended stream/set IDs, and we're not
> really detecting status packets in the best possible way), so I may
> change that also in subsequent patches.
> 
> New patch attached (again).
> 
> Ronald

> Index: ffmpeg-svn/libavformat/rdt.c
> ===================================================================
> --- ffmpeg-svn.orig/libavformat/rdt.c	2008-11-11 15:30:12.000000000 -0500
> +++ ffmpeg-svn/libavformat/rdt.c	2008-11-14 11:05:33.000000000 -0500
> @@ -184,6 +184,47 @@
>      }
>      if (len < 10)
>          return -1;
> +    /**
> +     * Layout of the header (in bits):
> +     * 1:  Flag indicating whether this header includes a length field;
> +           this can be used to concatenate multiple RDT packets in a
> +           single UDP/TCP data frame and is used to precede RDT data
> +           by stream status packets

> +     * 1:  Flag indicating whether this header includes a "reliable
> +           sequence number" (need_reliable); these are apparently
> +           sequence numbers of data packets alone. For data packets, this
> +           flag is always set, according to the Real documentation [1]

> +     * 5:  ID of a set of streams of identical content, possibly with
> +     *     codecs or bitrates

is this sentance supposed to make sense?
also why are the * a the begin randomly ommited?


> +     * 1:  Flag set for certain streams deemed less tolerable for packet
> +           loss

are you sure its stream and not packet?


> +     * 16: Packet sequence number; if >=0xFF00, this is a non-data packet
> +           containing stream status info, the second byte indicates the
> +           type of status packet (see wireshark docs / source code [2])
> +     * if (len_included) {
> +     *     16: Packet length
> +     * } else {
> +     *     Packet length = remainder of UDP/TCP frame
> +     * }
> +     * 1:  Back-to-Back flag; used for timing, set for one in every 10
> +           packets, according to the Real documentation [1]
> +     * 1:  Slow-data flag; currently unused, according to Real docs [1]
> +     * 5:  ID of the stream within this particular set of streams
> +     * 1:  Non-keyframe flag (unset if packet belongs to a keyframe)
> +     * 32: Timestamp (PTS)

> +     * if (set_id == 0x1F) {

undefined variable


> +     *     16: extended set_ID
> +     * }

> +     * if (need_reliable_fag) {

undefined variable


> +     *     16: Reliable sequence number
> +     * }

> +     * if (stream_id == 0x3F) {

undefined variable

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20081114/7515d186/attachment.pgp>