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

[Michael Niedermayer]

On Sat, Nov 15, 2008 at 03:51:17PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Sat, Nov 15, 2008 at 4:36 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Fri, Nov 14, 2008 at 10:05:36PM -0500, Ronald S. Bultje wrote:
> >> @@ -236,12 +244,32 @@
> >>       * [2] http://www.wireshark.org/docs/dfref/r/rdt.html and
> >>       *     http://anonsvn.wireshark.org/viewvc/trunk/epan/dissectors/packet-rdt.c
> >>       */
> >> -    if (set_id)    *set_id    = (buf[0]>>1) & 0x1f;
> >> -    if (seq_no)    *seq_no    = AV_RB16(buf+1);
> >> -    if (timestamp) *timestamp = AV_RB32(buf+4);
> >> -    if (stream_id) *stream_id = buf[3] & 0x3f;
> >> +    init_put_byte(&pb, buf, len, 0, NULL, NULL, NULL, NULL);
> >> +    set_id         = get_byte(&pb);
> >> +    len_included   = set_id & 0x80;
> >> +    need_reliable  = set_id & 0x40;
> >> +    set_id         = (set_id >> 1) & 0x1f;
> >> +    seq_no         = get_be16(&pb);
> >> +    if (len_included)
> >> +        url_fskip(&pb, 2); // packet length
> >> +    stream_id      = get_byte(&pb);
> >> +    is_no_keyframe = stream_id & 0x1;
> >> +    stream_id      = (stream_id >> 1) & 0x1f;
> >> +    timestamp      = get_be32(&pb);
> >> +    if (set_id == 0x1f)
> >> +        set_id     = get_be16(&pb);
> >> +    if (need_reliable)
> >> +        url_fskip(&pb, 2); // reliable sequence number
> >> +    if (stream_id == 0x1f)
> >> +        stream_id  = get_be16(&pb);
> >> +
> >> +    if (pset_id)      *pset_id = set_id;
> >> +    if (pseq_no)      *pseq_no = seq_no;
> >> +    if (pstream_id)   *pstream_id = stream_id;
> >> +    if (ptimestamp)   *ptimestamp = timestamp;
> >> +    if (pis_keyframe) *pis_keyframe = !is_no_keyframe;
> >
> > this is worse than before, it is even less readable and got pretty bloated
> > it also still misuses variables like set_id and stream_id as temporaries
> > for random things.
> > and its a mix of cosmetic and functional changes, the change to a differnt
> > way of reading MUST be seperate from functional changes.
> >
> > also if you want to change the code so radically, the get_bits() system is
> > more appropriate
> 
> Ok, so I guess getbits and getbytes are maybe overkill for this, I
> just though it'd be pretty since I don't have to do len-checks, but
> anyway... I can use getbits, but my impression is that you think it's
> just overkill either way, so attached is something simpler.
> 

> It includes two renames (set/stream_id -> pset/stream_id), for the
> purpose of having that information accessible even when the pointer is
> absent. I can do that in separate patches if wanted. It also does
> bufferpointer shifting which I can do in a separate patch, I guess I'm
> just wondering if this approach is OK with you.


> 
> Ronald

> Index: ffmpeg-svn/libavformat/rdt.c
> ===================================================================
> --- ffmpeg-svn.orig/libavformat/rdt.c	2008-11-15 11:25:10.000000000 -0500
> +++ ffmpeg-svn/libavformat/rdt.c	2008-11-15 12:47:32.000000000 -0500
> @@ -175,10 +175,10 @@
>  
>  int
>  ff_rdt_parse_header(const uint8_t *buf, int len,
> -                    int *set_id, int *seq_no, int *stream_id,
> +                    int *pset_id, int *seq_no, int *pstream_id,
>                      int *is_keyframe, uint32_t *timestamp)

if you want to prefix pointers by p in a function, fine but i think
it should be done consistently (within that function), that is for all pointer
arguments. And in a seperate patch


[...]

> +#define UPDATE_LEN(x) \
> +    buf += x; \
> +    len -= x; \
> +    consumed += x;

this is very ugly
RTFS bitstream.c/h bytestream.h and the code using them

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

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20081116/c2a2dfd5/attachment.pgp>