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

From: David Bryant <david@wavpack.com>

On 7/21/19 5:57 PM, Lynne wrote:
> Jul 22, 2019, 12:03 AM by david@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.
Done. New patch attached for all changes (hope that's the right way to do it).
>
>
>> +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.

Fixed random increment position and put in declarative int loops (even when they didn't save a line).

>
>
>> +    DSDfilters filters [2], *sp = filters;
> Same, spaces after variables for arrays, all throughout the file.

Done. Also fixed spaces after function names.

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

Fixed.

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

This was pointed out already as actually initialized, but I made it clearer (obviously it wasn't).

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

It's actually up to 32 allocs per frame because there's one for each history bin (value_lookup is an array of pointers, not
uint8s), and I didn't like it either because I had to worry about de-allocing on error. Refactored to use a single array in the
context as a pool. Thanks for the suggestion!

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

I'm very sure. The checks are within a few lines above each of the three divides.

>
>
>> +        crc += (crc << 1) + code;
> Don't NIH CRCs, we have av_crc in lavu. See below how to use it.

It's not a standard crc, but more of a recirculating checksum, so the NIH code is required.

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

They're not shadowed (dsd vs. dst) which is why my compiler didn't complain, but I took your suggestion of just changing the types.

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

see above

>
>
>> +    s->fdec_num = 0;
> Private codec context is always zeroed already.

removed

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

good to know...fixed

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

Yes.

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

Thanks. Also found a few places where I could use bytestream2_get_be32()!

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

I don't think anything is required here because all the medadata frames that we recognize are checked when parsed, and
WP_ID_DSD_DATA is the only one required for DSD audio, and all the others are silently ignored.

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

Your guess is as good as mine. This was part of the code "borrowed" from the PCM version (with the threading removed) so maybe
there is (or was) a situation that was writing one extra sample off the end. The code here certainly doesn't, but it seemed
pretty innocuous and I don't like just ripping out things I don't understand.

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

Seeking seems to work fine, again because it was working with PCM and I didn't mess with any of that. I added a flush to clear
the dsd2pcm contexts because that might have left an audible tick (that's the only context that's held over between frames that
isn't identical for the whole file).

Kind regards,

David

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
From 8c13bdb9e4a7748d3f9d937db573fc65ef645d65 Mon Sep 17 00:00:00 2001
From: David Bryant <david@wavpack.com>
Date: Wed, 24 Jul 2019 21:25:26 -0700
Subject: [PATCH] avcodec: add a WavPack DSD decoder

Signed-off-by: David Bryant <david@wavpack.com>
---
 libavcodec/Makefile      |   1 +
 libavcodec/allcodecs.c   |   1 +
 libavcodec/avcodec.h     |   1 +
 libavcodec/codec_desc.c  |   7 +
 libavcodec/wavpack.h     |   2 +
 libavcodec/wavpack_dsd.c | 775 +++++++++++++++++++++++++++++++++++++++++++++++
 libavformat/wvdec.c      |  32 +-
 7 files changed, 807 insertions(+), 12 deletions(-)
 create mode 100644 libavcodec/wavpack_dsd.c

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

Commit Message

Comments

Patch