| Message ID | 135941359.4438424.1620736697152@mail.yahoo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] : examples/transcode_aac.c - Do not use global pts for frame pts counting | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure | warning | Failed to apply patch |
Ray (12021-05-11): > The example transcode_aac.c uses a global pts for counting. For > libavcodec novices this can cause them to overlook this and result > with incorrect "start" times of output files if called multiple times > (see user error resulting in bug report > https://trac.ffmpeg.org/ticket/9228) I think it is a bad idea, it makes the example more complex, and only fix a very particular issue for people who know it already. Regards,
On 5/11/2021 10:53 AM, Nicolas George wrote: > Ray (12021-05-11): >> The example transcode_aac.c uses a global pts for counting. For >> libavcodec novices this can cause them to overlook this and result >> with incorrect "start" times of output files if called multiple times >> (see user error resulting in bug report >> https://trac.ffmpeg.org/ticket/9228) > > I think it is a bad idea, it makes the example more complex, and only > fix a very particular issue for people who know it already. > > Regards, An alternative solution is introducing a struct containing all variables used by the process, including {input,output}_format_context, {input,output}_codec_context, resample_context, fifo, and pts, which will simplify the code a lot by making every function in the example take a single argument for all of them, so that should address your concerns about making the example more complex. Our library usage examples should not result in people making mistakes like this when used as reference, so removing this global variable is a good idea.
James Almer (12021-05-11): > An alternative solution is introducing a struct containing all variables > used by the process, including {input,output}_format_context, > {input,output}_codec_context, resample_context, fifo, and pts, which will > simplify the code a lot by making every function in the example take a > single argument for all of them, so that should address your concerns about > making the example more complex. I agree, this would make the example more readable and clear. > Our library usage examples should not result in people making mistakes like > this when used as reference, so removing this global variable is a good > idea. If somebody follows the example without bothering to wonder what the various pieces of code do, they will get something wrong. Regards,
diff --git a/doc/examples/transcode_aac.c b/doc/examples/transcode_aac.c index c9badaa561..7f6148fa15 100644 --- a/doc/examples/transcode_aac.c +++ b/doc/examples/transcode_aac.c @@ -648,8 +648,6 @@ static int init_output_frame(AVFrame **frame, return 0; } -/* Global timestamp for the audio frames. */ -static int64_t pts = 0; /** * Encode one frame worth of audio to the output file. @@ -663,6 +661,7 @@ static int64_t pts = 0; static int encode_audio_frame(AVFrame *frame, AVFormatContext *output_format_context, AVCodecContext *output_codec_context, + int64_t* pts, int *data_present) { /* Packet used for temporary storage. */ @@ -675,8 +674,8 @@ static int encode_audio_frame(AVFrame *frame, /* Set a timestamp based on the sample rate for the container. */ if (frame) { - frame->pts = pts; - pts += frame->nb_samples; + frame->pts = *pts; + *pts += frame->nb_samples; } /* Send the audio frame stored in the temporary packet to the encoder. @@ -734,7 +733,8 @@ cleanup: */ static int load_encode_and_write(AVAudioFifo *fifo, AVFormatContext *output_format_context, - AVCodecContext *output_codec_context) + AVCodecContext *output_codec_context, + int64_t* pts) { /* Temporary storage of the output samples of the frame written to the file. */ AVFrame *output_frame; @@ -759,7 +759,7 @@ static int load_encode_and_write(AVAudioFifo *fifo, /* Encode one frame worth of audio samples. */ if (encode_audio_frame(output_frame, output_format_context, - output_codec_context, &data_written)) { + output_codec_context, pts, &data_written)) { av_frame_free(&output_frame); return AVERROR_EXIT; } @@ -790,6 +790,8 @@ int main(int argc, char **argv) SwrContext *resample_context = NULL; AVAudioFifo *fifo = NULL; int ret = AVERROR_EXIT; + /* timestamp for the audio frames. */ + int64_t pts = 0; if (argc != 3) { fprintf(stderr, "Usage: %s <input file> <output file>\n", argv[0]); @@ -850,7 +852,7 @@ int main(int argc, char **argv) /* Take one frame worth of audio samples from the FIFO buffer, * encode it and write it to the output file. */ if (load_encode_and_write(fifo, output_format_context, - output_codec_context)) + output_codec_context, &pts)) goto cleanup; /* If we are at the end of the input file and have encoded @@ -861,7 +863,7 @@ int main(int argc, char **argv) do { data_written = 0; if (encode_audio_frame(NULL, output_format_context, - output_codec_context, &data_written)) + output_codec_context, &pts, &data_written)) goto cleanup; } while (data_written); break;