diff mbox series

[FFmpeg-devel] swr/swresample: avoid reapplication of firstpts

Message ID 20231212112008.2500-1-ffmpeg@gyani.pro
State New
Headers show
Series [FFmpeg-devel] swr/swresample: avoid reapplication of firstpts | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Gyan Doshi Dec. 12, 2023, 11:20 a.m. UTC 
During a resampling operation where

1) user has specified first_pts
2) SWR_FLAG_RESAMPLE is not set initially (directly or otherwise)
3) first_pts has been fulfilled (always using hard compensation)

then upon first encountering a delay where a soft compensation is
required, swr_set_compensation will lead to another init of swr which
will reset outpts to the specified firstpts thus leading to an output
frame having its pts = firstpts. When the next input frame is received,
swr will see a large delay and inject silence from firstpts to the
current frame's pts. This can lead to severe desync and in worst case,
loss of audio playback.

Parameter initcount added to keep track of swr_init calls of the swrcontext
and to ultimately avoid enforcing firstpts again.

Fixes #4131.
---
 libswresample/swresample.c          | 7 +++++--
 libswresample/swresample_internal.h | 1 +
 libswresample/version.h             | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

Comments

Michael Niedermayer Dec. 14, 2023, 7:13 p.m. UTC | #1 
On Tue, Dec 12, 2023 at 04:50:08PM +0530, Gyan Doshi wrote:
> During a resampling operation where
> 
> 1) user has specified first_pts
> 2) SWR_FLAG_RESAMPLE is not set initially (directly or otherwise)
> 3) first_pts has been fulfilled (always using hard compensation)
> 
> then upon first encountering a delay where a soft compensation is
> required, swr_set_compensation will lead to another init of swr which
> will reset outpts to the specified firstpts thus leading to an output
> frame having its pts = firstpts. When the next input frame is received,
> swr will see a large delay and inject silence from firstpts to the
> current frame's pts. This can lead to severe desync and in worst case,
> loss of audio playback.
> 
> Parameter initcount added to keep track of swr_init calls of the swrcontext
> and to ultimately avoid enforcing firstpts again.
> 
> Fixes #4131.
> ---
>  libswresample/swresample.c          | 7 +++++--
>  libswresample/swresample_internal.h | 1 +
>  libswresample/version.h             | 2 +-
>  3 files changed, 7 insertions(+), 3 deletions(-)

Would it work to instead set firstpts = AV_NOPTS_VALUE on allocation
and then use firstpts == AV_NOPTS_VALUE to detect if it has been set
already ?
If that workes, that seems simpler than counting init

thx

[...]
Gyan Doshi Dec. 15, 2023, 7:02 a.m. UTC | #2 
On 2023-12-15 12:43 am, Michael Niedermayer wrote:
> On Tue, Dec 12, 2023 at 04:50:08PM +0530, Gyan Doshi wrote:
>> During a resampling operation where
>>
>> 1) user has specified first_pts
>> 2) SWR_FLAG_RESAMPLE is not set initially (directly or otherwise)
>> 3) first_pts has been fulfilled (always using hard compensation)
>>
>> then upon first encountering a delay where a soft compensation is
>> required, swr_set_compensation will lead to another init of swr which
>> will reset outpts to the specified firstpts thus leading to an output
>> frame having its pts = firstpts. When the next input frame is received,
>> swr will see a large delay and inject silence from firstpts to the
>> current frame's pts. This can lead to severe desync and in worst case,
>> loss of audio playback.
>>
>> Parameter initcount added to keep track of swr_init calls of the swrcontext
>> and to ultimately avoid enforcing firstpts again.
>>
>> Fixes #4131.
>> ---
>>   libswresample/swresample.c          | 7 +++++--
>>   libswresample/swresample_internal.h | 1 +
>>   libswresample/version.h             | 2 +-
>>   3 files changed, 7 insertions(+), 3 deletions(-)
> Would it work to instead set firstpts = AV_NOPTS_VALUE on allocation
> and then use firstpts == AV_NOPTS_VALUE to detect if it has been set
> already ?
> If that workes, that seems simpler than counting init

That does work.  I avoided it since I didn't know if I needed to make 
swr_alloc2() or change in place.

v2 sent.

Regards,
Gyan
diff mbox series

Patch

diff --git a/libswresample/swresample.c b/libswresample/swresample.c
index f2a9b40474..008af3610f 100644
--- a/libswresample/swresample.c
+++ b/libswresample/swresample.c
@@ -375,8 +375,9 @@  av_cold int swr_init(struct SwrContext *s){
     if (s->firstpts_in_samples != AV_NOPTS_VALUE) {
         if (!s->async && s->min_compensation >= FLT_MAX/2)
             s->async = 1;
-        s->firstpts =
-        s->outpts   = s->firstpts_in_samples * s->out_sample_rate;
+        if (!s->initcount)
+            s->firstpts =
+            s->outpts   = s->firstpts_in_samples * s->out_sample_rate;
     } else
         s->firstpts = AV_NOPTS_VALUE;
 
@@ -458,6 +459,7 @@  av_assert0(s->out.ch_count);
     if(!s->resample && !s->rematrix && !s->channel_map && !s->dither.method){
         s->full_convert = swri_audio_convert_alloc(s->out_sample_fmt,
                                                    s-> in_sample_fmt, s-> in.ch_count, NULL, 0);
+        s->initcount++;
         return 0;
     }
 
@@ -510,6 +512,7 @@  av_assert0(s->out.ch_count);
             goto fail;
     }
 
+    s->initcount++;
     return 0;
 fail:
     swr_close(s);
diff --git a/libswresample/swresample_internal.h b/libswresample/swresample_internal.h
index ad902d73fa..71d0c60e9f 100644
--- a/libswresample/swresample_internal.h
+++ b/libswresample/swresample_internal.h
@@ -145,6 +145,7 @@  struct SwrContext {
     float max_soft_compensation;                    ///< swr maximum soft compensation in seconds over soft_compensation_duration
     float async;                                    ///< swr simple 1 parameter async, similar to ffmpegs -async
     int64_t firstpts_in_samples;                    ///< swr first pts in samples
+    int initcount;                                  ///< swr no. of times the swrcontext has been inited
 
     int resample_first;                             ///< 1 if resampling must come first, 0 if rematrixing
     int rematrix;                                   ///< flag to indicate if rematrixing is needed (basically if input and output layouts mismatch)
diff --git a/libswresample/version.h b/libswresample/version.h
index 46a4e2fc62..fe4469ae6b 100644
--- a/libswresample/version.h
+++ b/libswresample/version.h
@@ -31,7 +31,7 @@ 
 #include "version_major.h"
 
 #define LIBSWRESAMPLE_VERSION_MINOR  13
-#define LIBSWRESAMPLE_VERSION_MICRO 100
+#define LIBSWRESAMPLE_VERSION_MICRO 101
 
 #define LIBSWRESAMPLE_VERSION_INT  AV_VERSION_INT(LIBSWRESAMPLE_VERSION_MAJOR, \
                                                   LIBSWRESAMPLE_VERSION_MINOR, \