[FFmpeg-devel] [PATCH v3] Add support for Audible AA files

[Carl Eugen Hoyos]

Vesselin Bontchev <vesselin.bontchev <at> yandex.com> writes:

> + at example
> +ffmpeg -v debug -i input.aa -c:a copy output.wav

I don't think this is a very useful example, instead 
please document the option that the demuxer has in 
short words (you don't have to document the default 
value imo).

> + * Redistribution and use in source and binary forms, 
> with or without modification,
> + * are permitted provided that the following conditions are met:

> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public

Please choose one license for the new file you are 
adding (and keep the copyright notices).

> +    char key[512];
> +    char val[512];

Somebody else will hopefully comment if this is too much 
for the stack, I am not sure.

> +    if (c->codec_second_size == -1) {
> +        return AVERROR_DECODER_NOT_FOUND;

There are two possibilities:
Either you expect (or know) that other codecs in aa 
files exist, then this should be PATCH_WELCOME and/or 
SAMPLE_NEEDED.
Or you have implemented all known codecs, then this 
should be INVALID_DATA.

And if you disagree with one of my comments, please 
say so;-)

> +    if (c->aa_fixed_key_size != 16) {
> +        av_log(s, AV_LOG_FATAL, "[aa] aa_fixed_key 
> value needs to be 16 bytes!\n");

Where is this variable set?

> +        v0 = header_seed;
> +        v1 = header_seed + 1;
> +        AV_WB32(src, v0);
> +        AV_WB32(src + 4, v1);
> +        header_seed = v1 + 1;

Do the local variables here increase readability?
If yes, I would suggest to locally declare the 
variables.
(If not, please remove them.)

> +    } else {
> +        return AVERROR_DECODER_NOT_FOUND;

If this code cannot be reached (I believe it cannot), 
please make this an av_assert().

> +    if (AV_RB32(buf+4) != AA_MAGIC)
> +        return 0;
> +
> +    return AVPROBE_SCORE_MAX / 4;

32bit should lead to MAX/2

Thank you, Carl Eugen