[FFmpeg-devel] [PATCH v2] examples: add flac_test

[Ludmila Glinskih]

Hi,

Thanks for you comments!

> +static int generate_raw_frame(uint16_t *frame_data, int i, int
> sample_rate,
> > +                              int channels, int frame_size)
> > +{
> > +    double t, tincr, tincr2;
> > +    int j, k;
> > +
> > +    t = 0.0;
> > +    tincr = 2 * M_PI * 440.0 / sample_rate;
> > +    tincr2 = tincr / sample_rate;
> > +    for (j = 0; j < frame_size; j++)
> > +    {
> > +        frame_data[channels * j] = (int)(sin(t) * 10000);
> > +        for (k = 1; k < channels; k++)
> > +            frame_data[channels * j + k] = frame_data[channels * j] * 2;
> > +        t = i * tincr + (i * (i + 1) / 2.0 * tincr2);
> > +    }
> > +    return 0;
> > +}
>
> This was mentioned before: using floating point in tests causes
> problems which can be avoided by using integers only. This includes the
> sin() function. (Maybe generate some sort of square wave instead? Or
> just silence? I don't know.)
>
> Yeah, I didn't decide how to do it right yet. I like Michael's idea about
audiogen.


> > +    result = avcodec_open2(ctx, enc, NULL);
> > +    if (result < 0)
> > +    {
> > +        av_log(NULL, AV_LOG_ERROR, "Can't open encoder\n");
> > +        return AVERROR_UNKNOWN;
>
> In this particular case, it would probably make sense to forward the
> error code. (Also affects some other lines in the patch.) I don't know
> how important this is for API tests, or what exactly we want, though.
>
I don't have any idea why I need it (right now). As soon as I find a reason
-- I'll change it.


> > +    ctx->request_sample_fmt = AV_SAMPLE_FMT_S16;
> > +    /* XXX: FLAC ignores it for some reason */
> > +    ctx->request_channel_layout = ch_layout;
> > +    ctx->channel_layout = ch_layout;
>
> Only some decoders can change the output, and then only in some cases.
> Normally, the API user is supposed to use libraries like libswresample
> to convert data to the required format. These fields (including
> request_sample_fmt) merely expose additional decoder features. They
> don't have to be present.
>
I test 4 different channel layouts, 2 of them have different values of
channel layout before and after encoding-decoding. Presetting
ctx->channel_layout fixes it.

> +    out_frame = av_frame_alloc();
> > +    if (!out_frame)
> > +    {
> > +        av_log(NULL, AV_LOG_ERROR, "Can't allocate output frame\n");
> > +        return AVERROR(ENOMEM);
>
> This leaks in_frame on error. But it might be ok in such a test. We
> have to decide whether it is. (I'd say it's ok.)
>
In my opinion it's ok. If in some situations it leads to problems -- it's
easy to fix).


> > +        if (got_output)
> > +        {
> > +            result = avcodec_decode_audio4(dec_ctx, out_frame,
> &got_output, &enc_pkt);
> > +            if (result < 0)
> > +            {
> > +                av_log(NULL, AV_LOG_ERROR, "Error decoding audio
> packet\n");
> > +                return AVERROR_UNKNOWN;
> > +            }
> > +
> > +            if (got_output)
> > +            {
> > +                if (result != enc_pkt.size)
> > +                {
> > +                    av_log(NULL, AV_LOG_INFO, "Decoder consumed only
> part of a packet, it is allowed to do so -- need to update this test\n");
>
> The message probably lacks an "if" ("if it is allowed").
>
 As I understood from the documentation -- every decoder is allowed to do
so. Message is to inform that this test doesn't cover this case.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>