[FFmpeg-devel] [PATCH]Lagarith decoder.

Michael Niedermayer michaelni
Tue Oct 13 11:28:32 CEST 2009


On Tue, Sep 22, 2009 at 11:54:03PM -0600, Nathan Caldwell wrote:
> Here are the latest Lagarith patches. The only changes to the
> rangecoder from the previous patch are cosmetic (K&R formatting). The
> decoder includes Loren's floating point emulation. Thanks to everyone,
> and especially Loren and Reimar, for all their help.

[...]
> +typedef struct lag_rac {
> +    AVCodecContext *avctx;
> +    unsigned low;
> +    unsigned range;
> +    unsigned scale;             /*!< Number of bits of precision in range. */
> +    unsigned hash_shift;        /*!< Number of bits to shift to calculate hash for radix search. */
> +
> +    uint8_t *bytestream_start;  /*!< Start of input bytestream. */
> +    uint8_t *bytestream;        /*!< Current position in input bytestream. */
> +    uint8_t *bytestream_end;    /*!< End position of input bytestream. */
> +

> +    int prob[258];              /*!< Table of cumulative probability for each symbol. */

this is filled with uint32_t giving nice negative probs


[...]
> +/* compute the 52bit mantissa of 1/(double)denom */
> +static uint64_t softfloat_reciprocal(uint32_t denom)

comment not doxygen compatible
and this really needs some explanation of why it is used instead of
1.0/denom in the doxy


[...]
> +static uint32_t lag_decode_prob(GetBitContext *gb)
> +{
> +    static const uint8_t series[] = { 1, 2, 3, 5, 8, 13, 21, 34 };

where is series[7] accessed?


> +    int i;
> +    int bit     = 0;
> +    int bits    = 0;
> +    int prevbit = 0;
> +    unsigned value = 1;
> +
> +    for (i = 0; i < 7; i++) {
> +        if (prevbit && bit)
> +            break;
> +        prevbit = bit;
> +        bit = get_bits1(gb);
> +        if (bit && !prevbit)
> +            bits += series[i];
> +    }
> +    bits--;

> +    if (bits <= 0)
> +        return 0;
> +    if (bits > 31) {
> +        /* This is most likely an error, just use the first 32 bits */
> +        bits = 31;
> +    }

are these not error conditions that warrent a error message and failure?


> +
> +    value  = get_bits_long(gb, bits);
> +    value |= 1 << bits;
> +
> +    return value - 1;
> +}
> +
> +static int lag_read_prob_header(lag_rac *rac, GetBitContext *gb)
> +{
> +    int i, j;
> +    int prob = 0;
> +    int scale_factor = 0;
> +    unsigned cumul_prob = 0;
> +    unsigned cumulative_target = 1;
> +    unsigned scaled_cumul_prob = 0;
> +
> +    rac->prob[0] = 0;
> +    rac->prob[257] = UINT_MAX;
> +    /* Read probabilities from bitstream */
> +    for (i = 1; i < 257; i++) {
> +        rac->prob[i] = lag_decode_prob(gb);

> +        cumul_prob += rac->prob[i];

can overflow


> +        if (!rac->prob[i]) {

> +            prob = lag_decode_prob(gb);
> +            if (i + prob >= 258)
> +                prob = 257 - i;

integer overflow, segfault below
and isnt this check supposed to be an error condition instead of silent
cliping?


> +            for (j = 0; j < prob; j++)
> +                rac->prob[++i] = 0;

[...]
> +static inline int add_left_prediction(uint8_t *dst, uint8_t *src, int w,
> +                                      int acc)
> +{
> +    int i;
> +
> +    for (i = 0; i < w - 1; i++) {
> +        acc += src[i];
> +        dst[i] = acc;
> +        i++;
> +        acc += src[i];
> +        dst[i] = acc;
> +    }
> +
> +    for (; i < w; i++) {
> +        acc += src[i];
> +        dst[i] = acc;
> +    }
> +
> +    return acc;
> +}

code duplication from dsputil


[...]
> +static int lag_decode_line(LagarithContext *l, lag_rac *rac,
> +                           uint8_t *dst, int width, int stride,
> +                           int step, int esc_count)
> +{
> +    int i = 0;
> +    int ret = 0;
> +
> +    /* Output any zeros remaining from the previous run */
> +    if (l->zeros_rem) {
> +        int count = FFMIN(l->zeros_rem, width);
> +        lag_memset(dst + i * step, 0, count, step);
> +        i += count;
> +        l->zeros_rem -= count;
> +    }
> +
> +    while (i < width) {
> +
> +        dst[i * step] = lag_get_rac(rac);
> +        ret++;
> +

> +        if (dst[i * step])
> +            l->zeros = 0;
> +        else
> +            l->zeros++;

maybe 
l->zeros += !!dst[i * step]; 
is faster

also getting rid of *step  and doing +=step might be faster
and then variables like zeros could be tried to be moved onto the
stack


> +
> +        i++;
> +        if (esc_count && (l->zeros == esc_count)) {

the esc_count check can likely be removed by changing esc_count=0 -> -1


> +            int count;
> +            int index = lag_get_rac(rac);
> +            ret++;
> +
> +            l->zeros = 0;
> +

> +            l->zeros_rem =
> +            count        = lag_calc_zero_run(index);
> +
> +            if (i + count > width)
> +                count = width - i;
> +            if (!count)
> +                continue;
> +
> +            lag_memset(dst + i * step, 0, count, step);
> +
> +            i += count;
> +            l->zeros_rem -= count;

l->zeros_rem = lag_calc_zero_run(index);
goto handle_zeros; (above in that if before the while)



> +        }
> +    }
> +    return ret;
> +}
> +

> +static int lag_decode_zero_run_line(LagarithContext *l, uint8_t *dst,
> +                                    const uint8_t *src, int width,
> +                                    int step, int esc_count)
> +{
> +    int i = 0;
> +    int count;
> +    uint8_t zero_run = 1;
> +    const uint8_t *start = src;
> +    uint8_t mask1 = -(esc_count < 2);
> +    uint8_t mask2 = -(esc_count < 3);
> +    uint8_t *end = dst + (width - 2) * step;
> +
> +    if (l->zeros_rem) {
> +        count = FFMIN(l->zeros_rem, width);
> +        lag_memset(dst, 0, count, step);
> +        l->zeros_rem -= count;
> +        dst += count;
> +    }
> +
> +    while (dst < end) {
> +        i = 0;
> +        while (!zero_run && dst + i * step < end) {
> +            i++;
> +            zero_run =
> +                (src[i] | (src[i + 1] & mask1) | (src[i + 2] & mask2));
> +        }
> +        if (!zero_run) {
> +            i += esc_count;
> +            lag_memcpy(dst, src, i, step);
> +            dst += i;
> +            l->zeros_rem =
> +            count        = lag_calc_zero_run(src[i]);
> +
> +            if (dst + l->zeros_rem * step > end)
> +                count = (end - dst) / step;
> +
> +            lag_memset(dst, 0, count, step);
> +            dst += count;
> +            src += i + 1;
> +            l->zeros_rem -= count;
> +        } else {
> +            lag_memcpy(dst, src, i, step);
> +            src += i;
> +        }
> +    }
> +    return start - src;
> +}

am i missing something or is half of that code unreachable and zero_run
always 1?


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

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- 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/20091013/62f11024/attachment.pgp>



More information about the ffmpeg-devel mailing list