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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
