[FFmpeg-devel] [PATCH] Bink Video decoder for FFmpeg 0.6

Måns Rullgård mans
Tue Feb 16 21:00:34 CET 2010


Jason Garrett-Glaser <darkshikari at gmail.com> writes:

> I'll try my hand at some patch review.
>
> +    do {
> +        if (!get_bits1(gb)) {
> +            *dst++ = *src++;
> +            size--;
> +        } else {
> +            *dst++ = *src2++;
> +            size2--;
> +        }
> +    } while (size && size2);
>
> Maybe dst++ should be moved out of the if?  I don't trust gcc to
> optimize away the duplicated increment that would otherwise be in each
> branch.

OTOH, keeping it as is allows post-increment addressing modes.  I
don't trust gcc to optimise the increment _into_ the branches either.

> +    while (size--)
> +        *dst++ = *src++;
> +    while (size2--)
> +        *dst++ = *src2++;
>
> This looks like a memcpy to me.

This one is defined for overlapping blocks.  Don't know if it matter here.

> +                        if (!bits) {
> +                            t = get_bits1(gb) ? -1 : 1;
> +                        } else {
> +                            t = get_bits(gb, bits) | mask;
> +                            if (get_bits1(gb))
> +                                t = -t;
> +                        }
>
> Is get_bits valid with bits=0?  If so, this can be simplified.

get_bits(0) returns an undefined value, but is guaranteed not to
advance the bitstream.

-- 
M?ns Rullg?rd
mans at mansr.com



More information about the ffmpeg-devel mailing list