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

[Ronald S. Bultje]

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
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rdt_parse_header-change-body.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081115/87f1fed0/attachment.txt>