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

[Zane van Iperen]

On 19/1/20 11:22 pm, Moritz Barsnick wrote:
> 
> 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. ;-)
> 

I didn't even notice that!

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

Fixed.

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

According to https://www.ffmpeg.org/developer.html, declaring 'i' in the loop
is allowed.

$ git grep 'i++' | wc -l
10442
$ git grep '++i' | wc -l
292

...Okay, fixed.

>> +        s = (int8_t) ((*src & 0xF0u) << 0u);
> 
> Keep the typecast aligned with the value, no space.
> 

Fixed.

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

Fixed.

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

Fixed, that was a relic from a much earlier version of the code.

>> +#define MAX_CHANNELS (2)
>> +#define PREVIOUS_SAMPLE_COUNT (2)
> 
> These brackets aren't required.
> 

Fixed.

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

Fixed.

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

Done.

>> +    if ((r = ff_get_buffer(avctx, frame, 0)) < 0)
>> +        return r;
> 
> ffmpeg uses "ret", consistently.
> 

Done.

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

Done.

> Cheers,
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 


Thanks,
Zane