[FFmpeg-devel] [PATCH v2 1/2] avcodec: add decoder for argonaut games' adpcm codec

[Moritz Barsnick]

On Sun, Jan 19, 2020 at 08:33:39 +0000, Zane van Iperen wrote:
> Adds support for the ADPCM variant used by some Argonaut Games' games,
> such as 'Croc! Legend of the Gobbos', and 'Croc 2'.

Some small nitpicks here:

>  - thistogram filter
>  - freezeframes filter
> -
> +- Argonaut Games ADPCM decoder
>

Please don't steal the empty line. ;-)

> +static int16_t *adpcm_decoder_1(uint8_t c, int16_t *dst, const uint8_t *src,
> +                          const int16_t *prev_,
> +                          int nsamples, int stride)

The arguments are usually aligned after line breaks - check other
implementation within ffmpeg.

> +    for (int i = 0; i < nsamples / 2; ++i, ++src) {

ffmpeg prefers the '++' after the variable, not before.
(And some developers prefer you to declare the iterator 'i' outside of
the loop, though I believe the rules on that have changed.)

> +        s = (int8_t) ((*src & 0xF0u) << 0u);

Keep the typecast aligned with the value, no space.

> +        dst += stride;
> +
> +        s = (int8_t) ((*src & 0x0Fu) << 4u);

Ditto.

All comments apply to adpcm_decoder_2() and other functions as well.

> +    unsigned char c;

I'm wondering whether you could stick to uint8_t consistenly?

> +#define MAX_CHANNELS (2)
> +#define PREVIOUS_SAMPLE_COUNT (2)

These brackets aren't required.

> +static av_cold int adpcm_decode_init(AVCodecContext *avctx)
> +{
> +    ADPCMArgoDecoderContext *ctx = avctx->priv_data;
> +
> +    if (avctx->channels > MAX_CHANNELS) {
> +        av_log(avctx, AV_LOG_ERROR, "Invalid channel count %d\n", avctx->channels);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (avctx->bits_per_coded_sample != 4) {
> +        av_log(avctx, AV_LOG_ERROR, "Invalid number of bits %d\n", avctx->bits_per_coded_sample);
> +        return AVERROR(EINVAL);
> +    }

These should probably be AVERROR_INVALIDDATA. AVERROR(EINVAL) is
"invalid arguments", i.e. if the user supplied an option value which is
not valid.

> +static int adpcm_decode_frame(AVCodecContext * avctx, void *data,
> +                                 int *got_frame_ptr, AVPacket * avpkt)

The '*' aligns with the variable name.

> +    if (avctx->channels == 1 && avpkt->size != 17) {
> +        av_log(avctx, AV_LOG_WARNING,
> +               "unexpected mono packet size, expected 17, got %d\n",
> +               avpkt->size);
> +    } else if(avctx->channels == 2 && avpkt->size != 34) {

Add a space: "if (".

> +    if ((r = ff_get_buffer(avctx, frame, 0)) < 0)
> +        return r;

ffmpeg uses "ret", consistently.

> +    dst = adpcm_decode_block((int16_t *) frame->data[0], avpkt->data, argo->prev, frame->nb_samples, avctx->channels);

Typecast aligns with the value, no space inserted.

Cheers,
Moritz