[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:
>>      /**
>>       * Layout of the header (in bits):
>>       * 1:  len_included
>
> code to skip status packets

?

You mean "I didn't document how to skip status packets"? I did, it
says that len_included is set for packet concatenation, which is used
for status packets in RDT, and that a value of >= 0xFF00 as seqno is
used as status packet indicator. So to skip them, you skip the length
of the status packet if the len field is set and it is a status
packet. If the len field is not set, there is no data packet and we
return -1 (e.g. EOS packet). That's exactly what the code does. You
want me to write out the full code in a comment before making an
implementation?

[..]
> 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

It's more complex because it's more correct. The original code works,
but is not completely spec-compliant. By analogy of an AAC decoder, it
could decode only one specific profile, so it may work for that stream
that iTunes serves when ripping CDs or buying songs, but it won't work
for future streams that may pop up at some point which may not come
from iTunes.

I'll look at getbits, but it won't make it simpler, just different...

>>  /**< return 0 on packet, no more left, 1 on packet, 1 on partial packet... */
>> @@ -289,7 +317,7 @@
>>  ff_rdt_parse_packet(RDTDemuxContext *s, AVPacket *pkt,
>>                      const uint8_t *buf, int len)
>>  {
>> -    int seq_no, flags = 0, stream_id, set_id;
>> +    int seq_no, flags = 0, stream_id, set_id, is_keyframe;
>>      uint32_t timestamp;
>>      int rv= 0;
>>
>> @@ -306,10 +334,10 @@
>>
>>      if (len < 12)
>>          return -1;
>> -    rv = ff_rdt_parse_header(buf, len, &set_id, &seq_no, &stream_id, &timestamp);
>> +    rv = ff_rdt_parse_header(buf, len, &set_id, &seq_no, &stream_id, &is_keyframe, &timestamp);
>>      if (rv < 0)
>>          return rv;
>> -    if (!(stream_id & 1) && (set_id != s->prev_set_id || timestamp != s->prev_timestamp)) {
>> +    if (is_keyframe && (set_id != s->prev_set_id || timestamp != s->prev_timestamp)) {
>>          flags |= PKT_FLAG_KEY;
>>          s->prev_set_id    = set_id;
>>          s->prev_timestamp = timestamp;
>
> Addition of is_keyframe, surely should be seperate as well

Attached (+ reindent).

Ronald
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rdt_parse_header-add-is_keyframe.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081115/0f30a830/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: post-keyframe-reindent.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081115/0f30a830/attachment.asc>