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

[Ronald S. Bultje]

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
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: add-rdt_hdr-comment.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081114/b20d77aa/attachment.txt>