[FFmpeg-devel] [PATCH]Lagarith decoder.

[Nathan Caldwell]

On Tue, Oct 13, 2009 at 3:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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.
>
>> +    int prob[258];              /*!< Table of cumulative probability for each symbol. */
>
> this is filled with uint32_t giving nice negative probs

Fixed.

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

Updated. It would be nice if Loren could comment on these descriptions.

>> +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?

It isn't. The table was copied from reference. Removed.

>> +    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?

In the case of bits==0, no. Any other case (bits < 0 or bits >31),
yes. This should be fixed now.

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

In this case the reference decoder would also overflow. Would you
prefer to match the reference, or not overflow?

>> +        if (!rac->prob[i]) {
>
>> +            prob = lag_decode_prob(gb);
>> +            if (i + prob >= 258)
>> +                prob = 257 - i;
>
> integer overflow, segfault below

Fixed.

> and isnt this check supposed to be an error condition instead of silent
> cliping?

In a perfect world, yes. Again, just matching the behavior of the
reference decoder.

>> +static inline int add_left_prediction(uint8_t *dst, uint8_t *src, int w,
>> +                                      int acc)
>
> code duplication from dsputil

I submitted this patch before the one that moved the huffyuv
prediction functions to dsputil. Fixed.

> [...]
>
>> +        if (dst[i * step])
>> +            l->zeros = 0;
>> +        else
>> +            l->zeros++;
>
> maybe
> l->zeros += !!dst[i * step];
> is faster

This won't work because we only want to count consecutive runs of
zeros. l->zeros needs to be reset to 0 at each non-zero byte.

> 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

This is what I originally had, however it introduces too many
divisions in the calculation of the zero runs.

>> +
>> +        i++;
>> +        if (esc_count && (l->zeros == esc_count)) {
>
> the esc_count check can likely be removed by changing esc_count=0 -> -1

Done.

>> +            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)

Done.

>> +        }
>> +    }
>> +    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)
>
> am i missing something or is half of that code unreachable and zero_run
> always 1?

Oops, you are correct. A few bugs in there.


--
-Nathan Caldwell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lagarith-rangecoder.patch
Type: application/octet-stream
Size: 5942 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091015/caf7ac40/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lagarith-decoder.patch
Type: application/octet-stream
Size: 19320 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091015/caf7ac40/attachment-0001.obj>