diff mbox series

[FFmpeg-devel] : examples/transcode_aac.c - Do not use global pts for frame pts counting

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
Related show

Checks

Context Check Description
andriy/configure warning Failed to apply patch
andriy/configure warning Failed to apply patch

Commit Message

Ray May 11, 2021, 12:38 p.m. UTC
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)


From 52cbed063ee54e667905ca243e8ee4a811a108dc Mon Sep 17 00:00:00 2001
From: whatdoineed2do/Ray <whatdoineed2do@gmail.com>
Date: Tue, 11 May 2021 13:16:48 +0100
Subject: [PATCH] Do not use global pts for frame pts counting

Signed-off-by: whatdoineed2do/Ray <whatdoineed2do@gmail.com>
---
 doc/examples/transcode_aac.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

--
2.26.3

Comments

Nicolas George May 11, 2021, 1:53 p.m. UTC | #1
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,
James Almer May 11, 2021, 2:57 p.m. UTC | #2
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.
Nicolas George May 11, 2021, 3:01 p.m. UTC | #3
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 mbox series

Patch

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;