[FFmpeg-devel] remove int readers

Reimar Doeffinger Reimar.Doeffinger
Thu Jun 14 11:04:27 CEST 2007


Hello,
On Wed, Jun 13, 2007 at 09:18:42PM -0400, Ronald S. Bultje wrote:
> Apologies, new patch attached.  This one may actually work. :-).

Please ignore the length &[] vs. + pointer syntax, while it is sometimes
fun to discuss such things it is not really important for this patch.

> (For the compiler warnings, I guess it's not a good idea to ask for -Werror
> to be enabled as long as gcc gives warnings where it's wrong, or would such
> patches still be accepted?)

gcc warns all the time adds some new, completely useless warning (at
least useless to anyone who has programmed C for years), so no, -Werror
is a bad idea.

[...]
> @@ -698,15 +698,9 @@
>  
>  static int64_t get_pts(const uint8_t *p)
>  {
> -    int64_t pts;
> -    int val;
> -
> -    pts = (int64_t)((p[0] >> 1) & 0x07) << 30;
> -    val = (p[1] << 8) | p[2];
> -    pts |= (int64_t)(val >> 1) << 15;
> -    val = (p[3] << 8) | p[4];
> -    pts |= (int64_t)(val >> 1);
> -    return pts;
> +    return ((int64_t)((p[0] >> 1) & 0x07)   << 30) |
> +                     ((AV_RB16(p + 1) >> 1) << 15) |
> +                      (AV_RB16(p + 3) >> 1);

IMO replace only the two val = reads by AV_RB16, putting it all in one
statement makes it much harder to read (and also to verify that it
actually does the same thing).

> -        version = get_bits(&gb, 8) << 16;
> -        version |= get_bits(&gb, 8) << 8;
> -        version |= get_bits(&gb, 8);
> -
> +        version = get_bits(&gb, 24);

I think you must use get_bits_long to read 24 bits. And I assume you
checked that this actually gives the same result? (depends on bitstream
reader used).

> @@ -714,15 +714,10 @@
>  offset_t ffm_read_write_index(int fd)
>  {
>      uint8_t buf[8];
> -    offset_t pos;
> -    int i;
>  
>      lseek(fd, 8, SEEK_SET);
>      read(fd, buf, 8);
> -    pos = 0;
> -    for(i=0;i<8;i++)
> -        pos |= (int64_t)buf[i] << (56 - i * 8);
> -    return pos;
> +    return (offset_t)AV_RB64(buf);

Hmm... why should that cast be needed?

Btw. I think the next step would be to put the bytestream stuff into
libavutil and use it where appropriate ;-)

Greetings,
Reimar Doeffinger




More information about the ffmpeg-devel mailing list