[FFmpeg-devel] [PATCH]Lagarith decoder.
Nathan Caldwell
saintdev
Fri Oct 16 04:42:54 CEST 2009
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>
More information about the ffmpeg-devel
mailing list