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

[Michael Niedermayer]

On Tue, Nov 11, 2008 at 09:16:43AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Nov 11, 2008 at 9:15 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> > New patch attached, hopefully this addresses your concerns.
> 
> zzz...
> 
> R

> Index: ffmpeg-svn/libavformat/rdt.c
> ===================================================================
> --- ffmpeg-svn.orig/libavformat/rdt.c	2008-10-11 08:47:36.000000000 -0400
> +++ ffmpeg-svn/libavformat/rdt.c	2008-11-11 09:13:10.000000000 -0500
> @@ -39,7 +39,12 @@
>      AVStream *st;
>      void *dynamic_protocol_context;
>      DynamicPayloadPacketHandlerProc parse_packet;
> -    uint32_t prev_sn, prev_ts;
> +    uint32_t timestamp;
> +    /*< Sub-stream and Set-number; A RTP packet belongs to a particular set
> +     * (e.g. audio streams, or video streams). Each set can have multiple
> +     * rules ("sub-streams") that clients can subscribe to, such as
> +     * different codecs and/or bitrates. */

does doxygen associate this with the correct variable?


> +    int sub_strm, set_nr;

seperate lines, please
also please do not abbreviate stream
and the variables are still extreemly poorly named and completely lack
a definition let alone a clear definition. This is so messy that i cannot
even suggest better names as i have little clue what you mean with the ones
here.


audio/video is a type not a set
one could speak of the set of all video streams or of a subset of all video
streams but your text is too unclear to even guess if either or none of this
is meant.
Also id like to say that if you want me (compared to someone else) to approve
this your useage of the word rule to refer to some kind of streams must be
removed. A stream or set of streams can be associated with a rule or a set of
rules but a rule is no stream. These are semantic problems with the terms you
use.


[..]
> @@ -184,10 +190,10 @@
>      }
>      if (len < 10)
>          return -1;
> -    if (sn)  *sn  = (buf[0]>>1) & 0x1f;
> -    if (seq) *seq = AV_RB16(buf+1);
> -    if (ts)  *ts  = AV_RB32(buf+4);
> -    if (rn)  *rn  = buf[3] & 0x3f;
> +    if (set_nr)    *set_nr    = (buf[0]>>1) & 0x1f;
> +    if (seq)       *seq       = AV_RB16(buf+1);
> +    if (timestamp) *timestamp = AV_RB32(buf+4);
> +    if (sub_strm)  *sub_strm  = buf[3] & 0x3f;

cosmetic changes must be in seperate patches

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/20081111/934ac3e8/attachment.pgp>