[FFmpeg-devel] Test for FLAC

[wm4]

On Fri, 10 Apr 2015 03:45:33 +0300
Ludmila Glinskih <lglinskih at gmail.com> wrote:

> Hi!
> 
> I'm a newbie girl, interested in writing tests for the ffmpeg API. I
> would like to propose a simple test for the FLAC codec. Since there is
> no infrastructure for API testing yet, I added it to the examples
> directory. I hope it's not too bad=). I will be very grateful for you
> comments.
> 
> My pull request: https://github.com/FFmpeg/FFmpeg/pull/132

I agree with reimar. This should be sent as patch to the mailing list.
Look at git send-email (http://git-scm.com/docs/git-send-email) how to
do this conveniently (it requires some setup, but is rather convenient
once it works). Anyway, this is just for the future.

So I'll try to review by copy&pasting lines from the github diff viewer.

> + * @example flac_test.c

The doxygen should probably be removed. Not only is it not correct
anymore, but there's also no need to explain basic API usage. It's a
test not an example - only tricky and non-obvious things should be
commented.

(I take it this test was mostly copy&pasted from the examples to turn
it into a test, but things like these should be adjusted.)

> +static int check_sample_fmt(AVCodec *codec, enum AVSampleFormat sample_fmt)

Since it's a test, maybe it should be made predictable by selecting
fixed parameters? A codec might be extended to support more sample
formats, and then the test's output might change.

> +static int generate_raw_frame(uint16_t *frame_data, int i, int sample_rate, int channels, int frame_size)
> ...
> +          frame_data[channels * j] = (int)(sin(t) * 10000);

Using a function like sin() seems like a bad idea. It uses floats, and
float calculations tend to yield minor differences between platforms.
Tests need to output predictable results on all platforms. So something
else should be used to generate dummy audio (if we even need it).

> +int main(int argc, char** argv)
> ...
> +      return AVERROR(ENOMEM);

It doesn't make too much sense to return this from main(). main()
returns 0 on success, and a positive number between 1-255 on error (at
least on UNIX), while AVERRORs are negative numbers.

> + result = avcodec_fill_audio_frame(in_frame, encoder_ctx->channels, encoder_ctx->sample_fmt,
> (const uint8_t*)frame_data, frame_data_size, 0);

Unfortunately, this is blatantly wrong API usage. You are allocating a
frame correctly, but not the frame data. The way this code does it is
at best tolerated-but-deprecated legacy API usage, and at worst can
cause memory corruption and similar issues (I didn't look too closely
at the code to check which case applies here).

This should be using av_frame_get_buffer() instead, which will allocate
the frame data _and_ setup the frame data pointers. It's called
on an allocated AVFrame, so these are still 2 steps.

> +   uint8_t buffer[AUDIO_INBUF_SIZE + FF_INPUT_BUFFER_PADDING_SIZE];
> ....
> + av_init_packet(&dec_pkt);
> + memcpy(buffer, enc_pkt.data, enc_pkt.size);
> + dec_pkt.data = buffer;
> + dec_pkt.size = enc_pkt.size;

This code is broken: it assumes the pointer on the stack is aligned to
32 bytes. Encoders may invoke SIMD instructions like SSE on the data,
which typically requires it to be aligned this way. Also, who says that
AUDIO_INBUF_SIZE is big enough? You can probably easily come up with an
upper bound, but using dynamic memory allocation makes this much easier.

It would be better to call av_new_packet() instead of this dance.
(Don't forget to free the packet as well.)

But I really wonder why this code does this at all, and doesn't just
use the encoder's packet directly. I wonder if there's a good reason
for this.

> + memcpy(raw_out + out_offset, out_frame->data[0], out_frame_data_size);

This ignores all the other planes. Packet audio formats have all data
in plane 0 (i.e. data[0]), but many audio codecs produce
non-interleaved audio (called planar audio in ffmpeg). So the first
channel will be in data[0], the second in data[1], etc. The example
ignored this for simplicity, but a test should definitely cover all
output.

And while we're talking about output: you should test other fields as
well. For example, you need to test that the AVFrame has the correct
samplerate set and all that.

> + DOC_EXAMPLES-$(CONFIG_FLAC_TEST_EXAMPLE) += flac_test

Oops, I thought this was porting a pre-existing example to a test, but
it just _adds_ a new example. My comments still mostly apply, though.