diff mbox

[FFmpeg-devel] libavformat/mux: Fix audio_preload

Message ID 20190624211618.39134-1-andreas.rheinhardt@gmail.com
State Accepted
Commit 1889e3166cc5780780d7f40ac2271e5308f32b8e
Headers show

Commit Message

Andreas Rheinhardt June 24, 2019, 9:16 p.m. UTC 
Commit 31f9032b added the audio_preload feature; its goal is to
interleave audio earlier than the rest. Unfortunately, it has never ever
worked, because the check for whether a packet should be interleaved
before or after another packet was completely wrong: When audio_preload
vanishes, interleave_compare_dts returns 1 if the new packet should be
interleaved earlier than the packet it is compared with and that is what
the rest of the code expects. But the codepath used when audio_preload is
set does the opposite.

Also fixes potential undefined behaviour (namely signed integer
overflow).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
avformat.h contains the following note about audio_preload: "not all
formats support this and unpredictable things may happen if it is used
when not supported." Has this been added because of unpredictable
results caused by the buggy check? This option seems to work fine as
long as audio_preload is not in the range of max_interleave_duration.

 libavformat/mux.c     | 22 ++++++++++++++--------
 libavformat/version.h |  2 +-
 2 files changed, 15 insertions(+), 9 deletions(-)

Comments

Michael Niedermayer June 26, 2019, 5:05 p.m. UTC | #1 
On Mon, Jun 24, 2019 at 11:16:18PM +0200, Andreas Rheinhardt wrote:
> Commit 31f9032b added the audio_preload feature; its goal is to
> interleave audio earlier than the rest. Unfortunately, it has never ever
> worked, because the check for whether a packet should be interleaved
> before or after another packet was completely wrong: When audio_preload
> vanishes, interleave_compare_dts returns 1 if the new packet should be
> interleaved earlier than the packet it is compared with and that is what
> the rest of the code expects. But the codepath used when audio_preload is
> set does the opposite.
> 
> Also fixes potential undefined behaviour (namely signed integer
> overflow).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> avformat.h contains the following note about audio_preload: "not all
> formats support this and unpredictable things may happen if it is used
> when not supported." Has this been added because of unpredictable
> results caused by the buggy check? This option seems to work fine as
> long as audio_preload is not in the range of max_interleave_duration.

audio preload will violate the spec of some containers which dont allow
any preload. Thats what the note was for IIRC

> 
>  libavformat/mux.c     | 22 ++++++++++++++--------
>  libavformat/version.h |  2 +-
>  2 files changed, 15 insertions(+), 9 deletions(-)

will apply

thanks

[...]
diff mbox

Patch

diff --git a/libavformat/mux.c b/libavformat/mux.c
index 83fe1de78f..5e1ecd8485 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1001,15 +1001,21 @@  static int interleave_compare_dts(AVFormatContext *s, AVPacket *next,
     AVStream *st2 = s->streams[next->stream_index];
     int comp      = av_compare_ts(next->dts, st2->time_base, pkt->dts,
                                   st->time_base);
-    if (s->audio_preload && ((st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) != (st2->codecpar->codec_type == AVMEDIA_TYPE_AUDIO))) {
-        int64_t ts = av_rescale_q(pkt ->dts, st ->time_base, AV_TIME_BASE_Q) - s->audio_preload*(st ->codecpar->codec_type == AVMEDIA_TYPE_AUDIO);
-        int64_t ts2= av_rescale_q(next->dts, st2->time_base, AV_TIME_BASE_Q) - s->audio_preload*(st2->codecpar->codec_type == AVMEDIA_TYPE_AUDIO);
-        if (ts == ts2) {
-            ts= ( pkt ->dts* st->time_base.num*AV_TIME_BASE - s->audio_preload*(int64_t)(st ->codecpar->codec_type == AVMEDIA_TYPE_AUDIO)* st->time_base.den)*st2->time_base.den
-               -( next->dts*st2->time_base.num*AV_TIME_BASE - s->audio_preload*(int64_t)(st2->codecpar->codec_type == AVMEDIA_TYPE_AUDIO)*st2->time_base.den)* st->time_base.den;
-            ts2=0;
+    if (s->audio_preload) {
+        int preload  = st ->codecpar->codec_type == AVMEDIA_TYPE_AUDIO;
+        int preload2 = st2->codecpar->codec_type == AVMEDIA_TYPE_AUDIO;
+        if (preload != preload2) {
+            preload  *= s->audio_preload;
+            preload2 *= s->audio_preload;
+            int64_t ts = av_rescale_q(pkt ->dts, st ->time_base, AV_TIME_BASE_Q) - preload;
+            int64_t ts2= av_rescale_q(next->dts, st2->time_base, AV_TIME_BASE_Q) - preload2;
+            if (ts == ts2) {
+                ts  = ((uint64_t)pkt ->dts*st ->time_base.num*AV_TIME_BASE - (uint64_t)preload *st ->time_base.den)*st2->time_base.den
+                    - ((uint64_t)next->dts*st2->time_base.num*AV_TIME_BASE - (uint64_t)preload2*st2->time_base.den)*st ->time_base.den;
+                ts2 = 0;
+            }
+            comp = (ts2 > ts) - (ts2 < ts);
         }
-        comp= (ts>ts2) - (ts<ts2);
     }
 
     if (comp == 0)
diff --git a/libavformat/version.h b/libavformat/version.h
index 52dd95f5c6..39b00f62ab 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -33,7 +33,7 @@ 
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  58
 #define LIBAVFORMAT_VERSION_MINOR  28
-#define LIBAVFORMAT_VERSION_MICRO 100
+#define LIBAVFORMAT_VERSION_MICRO 101
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
                                                LIBAVFORMAT_VERSION_MINOR, \