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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
Thanks,
Zane
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
