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

[Michael Niedermayer]

On Fri, Nov 14, 2008 at 10:05:36PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Nov 14, 2008 at 5:49 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Fri, Nov 14, 2008 at 05:44:46PM -0500, Ronald S. Bultje wrote:
> >> On Fri, Nov 14, 2008 at 4:32 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> > On Fri, Nov 14, 2008 at 03:15:26PM -0500, Ronald S. Bultje wrote:
> >> >> Applied. Attached is a next patch that changes the variable names as suggested.
> >> >
> >> > whatever it is it mixes unrelated changes
> >>
> >> Alright, sorry, just the renames then.
> >
> > ok
> 
> Here's the next part, which changes the implementation to actually
> match the documentation more closely, you'll probably like this better
> than the existing code.
> 
> Ronald

> Index: ffmpeg-svn/libavformat/rdt.c
> ===================================================================
> --- ffmpeg-svn.orig/libavformat/rdt.c	2008-11-14 21:39:14.000000000 -0500
> +++ ffmpeg-svn/libavformat/rdt.c	2008-11-14 21:46:20.000000000 -0500
> @@ -174,17 +174,25 @@
>  
>  int
>  ff_rdt_parse_header(const uint8_t *buf, int len,
> -                    int *set_id, int *seq_no, int *stream_id, uint32_t *timestamp)
> +                    int *pset_id, int *pseq_no, int *pstream_id,

These are still renames ...


> +                    int *pis_keyframe, uint32_t *ptimestamp)
>  {
> -    int consumed = 10;
> +    ByteIOContext pb;
> +    int consumed = 0, need_reliable, len_included, stream_id, set_id, seq_no, is_no_keyframe;
> +    uint32_t timestamp;
>  
> -    if (len > 0 && (buf[0] < 0x40 || buf[0] > 0x42)) {
> -        buf += 9;
> -        len -= 9;
> -        consumed += 9;
> +    /* skip status packets */
> +    while (len >= 5 && buf[1] == 0xFF /* status packet */) {
> +        int pkt_len;
> +
> +        if (!(buf[0] & 0x80))
> +            return -1; /* frame contains no data packet */
> +        pkt_len  = AV_RB16(buf+3);
> +        buf += pkt_len;
> +        len -= pkt_len;
> +        consumed += pkt_len;
>      }
> -    if (len < 10)
> -        return -1;
> +
>      /**
>       * Layout of the header (in bits):
>       * 1:  len_included

code to skip status packets



> @@ -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


>  
> -    return consumed;
> +    return consumed + url_ftell(&pb);
>  }
>  

now i really no longer know if this is yet another seperate change
or a consequence of some other change


>  /**< 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

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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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/20081115/65b72a09/attachment.pgp>