[FFmpeg-devel] avcodec: add a WavPack DSD decoder

[Lynne]

Jul 22, 2019, 12:03 AM by david at wavpack.com:

> Hi,
>
> As I promised late last year, here is a patch to add a WavPack DSD decoder.
>
> Thanks!
>
> -David Bryant
>

> +    unsigned char probabilities [MAX_HISTORY_BINS] [256];
> +    unsigned char *value_lookup [MAX_HISTORY_BINS];

Use uint8_t throughout the patch.
Also don't add spaces between array declarations or lookups.


> +static void init_ptable (int *table, int rate_i, int rate_s)
> +{
> +    int value = 0x808000, rate = rate_i << 8, c, i;
> +
> +    for (c = (rate + 128) >> 8; c--;)
> +        value += (DOWN - value) >> DECAY;
> +
> +    for (i = 0; i < PTABLE_BINS/2; ++i) {

What's up with the random increment position in loops? It changes to before and after the variable throughout. Make it consistent and after the variable.
Also we support declarative for (int loops. Can save lines.


> +    DSDfilters filters [2], *sp = filters;

Same, spaces after variables for arrays, all throughout the file.


> +            if (code > max_probability) {
> +                int zcount = code - max_probability;
> +
> +                while (outptr < outend && zcount--)
> +                    *outptr++ = 0;
> +            }
> +            else if (code)
> +                *outptr++ = code;
> +            else
> +                break;

We don't put else on a new line, and prefer to have each branch wrapped in bracket unless all branches are single lines.


> +    for (p0 = 0; p0 < history_bins; ++p0) {
> +        int32_t sum_values;
> +        unsigned char *vp;
> +
> +        for (sum_values = i = 0; i < 256; ++i)
> +            s->summed_probabilities [p0] [i] = sum_values += s->probabilities [p0] [i];

sum_values is uninitialized. Does you compiler not warn about this?


> +        if (sum_values) {
> +            total_summed_probabilities += sum_values;
> +            vp = s->value_lookup [p0] = av_malloc (sum_values);

I don't like the per-frame alloc. The maximum sum_values can be is 255*255 = UINT16_MAX.
 60k of memory isn't much at all, just define value_lookup[255*255] in the context and you'll probably plug a few out of bounds accesses too.


> +            mult = high / s->summed_probabilities [p0] [255];
s->summed_probabilities [p0] [255]; can be zero, you already check if its zero when allocating currently. You should probably check for divide by zero unless you're very sure it can't happen.


> +        crc += (crc << 1) + code;

Don't NIH CRCs, we have av_crc in lavu. See below how to use it.


> +static int wv_unpack_dsd_copy(WavpackFrameContext *s, void *dst_l, void *dst_r)
> +{
> +    uint8_t *dsd_l              = dst_l;
> +    uint8_t *dsd_r              = dst_r;

You're shadowing arguments. Your compiler doesn't warn on this either?
You're calling the function with uint8_ts anyway, just change the type.


> +    while (total_samples--) {
> +        crc += (crc << 1) + (*dsd_l = bytestream2_get_byte(&s->gb));
> +        dsd_l += 4;
> +
> +        if (dst_r) {
> +            crc += (crc << 1) + (*dsd_r = bytestream2_get_byte(&s->gb));
> +            dsd_r += 4;
> +        }
> +    }

av_crc(av_crc_get_table(AV_CRC_32_IEEE/LE), UINT32_MAX, dsd_start_r/l, dsd_r/l - dsd_start_r/l) should work and be faster.


> +    s->fdec_num = 0;

Private codec context is always zeroed already.


> +    int chan = 0, chmask = 0, sample_rate = 0, rate_x = 1, dsd_mode = 0;
> +                chmask = avctx->channel_layout;
>      uint32_t chmask, flags;

frame->channel_layout is uint64_t.


> +    samples_l = frame->extended_data[wc->ch_offset];
> +    if (s->stereo)
> +        samples_r = frame->extended_data[wc->ch_offset + 1];
> +
> +    wc->ch_offset += 1 + s->stereo;

Have you checked non-stereo decodes fine and the channels are correctly ordered?


> +        if (id & WP_IDF_LONG) {
> +            size |= (bytestream2_get_byte(&gb)) << 8;
> +            size |= (bytestream2_get_byte(&gb)) << 16;
> +        }

Could use bytestream2_get_le16u/be16u to save 2 lines.


> +    if (!got_dsd) {
> +        av_log(avctx, AV_LOG_ERROR, "Packed samples not found\n");
> +        return AVERROR_INVALIDDATA;
> +    }

I think you should check avctx is completely configured before this, after parsing all WP_IDs, in case something is corrupt.


> +        frame->nb_samples = s->samples + 1;
> +        if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
> +            return ret;
> +        frame->nb_samples = s->samples;

?. Is the extra sample used as temporary buffer or something?


> +AVCodec ff_wavpack_dsd_decoder = {
> +    .name           = "wavpack_dsd",
> +    .long_name      = NULL_IF_CONFIG_SMALL("WavPack DSD"),
> +    .type           = AVMEDIA_TYPE_AUDIO,
> +    .id             = AV_CODEC_ID_WAVPACK_DSD,
> +    .priv_data_size = sizeof(WavpackContext),
> +    .init           = wavpack_decode_init,
> +    .close          = wavpack_decode_end,
> +    .decode         = wavpack_decode_frame,
> +    .capabilities   = AV_CODEC_CAP_DR1,
> +};

Seeking is probably broken. You should add a flush function to reset the decoder entirely.