diff mbox series

[FFmpeg-devel,v3,7/7] avformat/audiointerleave: use a fixed frame duration for non-audio packets

Message ID 20200313013124.26322-1-cus@passwd.hu
State New
Headers show
Series Untitled series #523
Related show

Commit Message

Marton Balint March 13, 2020, 1:31 a.m. UTC
The packet durations might not be set properly which can cause the MXF muxer
to write more than one packet of a stream to an edit unit messing up the
constant byte per element index...

Also warn the user if the incoming DTS is not increasing by frame duration
because in that case A-V sync issues can occur because of the DTS rewriting.

Also use nb_samples directly to calculate dts of audio packets because adding
packet durations might not be precise.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/audiointerleave.c | 21 ++++++++++++++++++---
 libavformat/audiointerleave.h |  4 +++-
 libavformat/gxfenc.c          |  2 +-
 libavformat/mxfenc.c          |  2 +-
 4 files changed, 23 insertions(+), 6 deletions(-)

Comments

Tomas Härdin March 14, 2020, 10:31 a.m. UTC | #1
fre 2020-03-13 klockan 02:31 +0100 skrev Marton Balint:
> The packet durations might not be set properly which can cause the MXF muxer
> to write more than one packet of a stream to an edit unit messing up the
> constant byte per element index...

Shouldn't this be fixed "higher up"?

> Also warn the user if the incoming DTS is not increasing by frame duration
> because in that case A-V sync issues can occur because of the DTS rewriting.

I'm not a huge fan of this, shouldn't we investigate why this happens
instead? DTS being non-monotonic is a big deal

> Also use nb_samples directly to calculate dts of audio packets because adding
> packet durations might not be precise.

lavf is such a mess...

The patch itself looks fine, modulo the above concerns

/Tomas
Marton Balint March 14, 2020, 4:59 p.m. UTC | #2
On Sat, 14 Mar 2020, Tomas Härdin wrote:

> fre 2020-03-13 klockan 02:31 +0100 skrev Marton Balint:
>> The packet durations might not be set properly which can cause the MXF muxer
>> to write more than one packet of a stream to an edit unit messing up the
>> constant byte per element index...
>
> Shouldn't this be fixed "higher up"?

It should also be fixed higher up, but the MXF muxer should never write 
broken files, so it should be fixed here as well, either with a warning, 
and discarding input timestamps/durations or rejecting such attempts 
completely.

>
>> Also warn the user if the incoming DTS is not increasing by frame duration
>> because in that case A-V sync issues can occur because of the DTS rewriting.
>
> I'm not a huge fan of this, shouldn't we investigate why this happens
> instead? DTS being non-monotonic is a big deal

fate-copy-trac4914 is an example of this, it is a stream copy for video.

Regards,
Marton
Tomas Härdin March 16, 2020, 7:05 p.m. UTC | #3
lör 2020-03-14 klockan 17:59 +0100 skrev Marton Balint:
> 
> On Sat, 14 Mar 2020, Tomas Härdin wrote:
> 
> > fre 2020-03-13 klockan 02:31 +0100 skrev Marton Balint:
> > > The packet durations might not be set properly which can cause the MXF muxer
> > > to write more than one packet of a stream to an edit unit messing up the
> > > constant byte per element index...
> > 
> > Shouldn't this be fixed "higher up"?
> 
> It should also be fixed higher up, but the MXF muxer should never write 
> broken files, so it should be fixed here as well, either with a warning, 
> and discarding input timestamps/durations or rejecting such attempts 
> completely.
> 
> > > Also warn the user if the incoming DTS is not increasing by frame duration
> > > because in that case A-V sync issues can occur because of the DTS rewriting.
> > 
> > I'm not a huge fan of this, shouldn't we investigate why this happens
> > instead? DTS being non-monotonic is a big deal
> 
> fate-copy-trac4914 is an example of this, it is a stream copy for video.

Ugh. Well, OK then I guess.

/Tomas
diff mbox series

Patch

diff --git a/libavformat/audiointerleave.c b/libavformat/audiointerleave.c
index 2e83031bd6..a235a4456a 100644
--- a/libavformat/audiointerleave.c
+++ b/libavformat/audiointerleave.c
@@ -40,6 +40,7 @@  void ff_audio_interleave_close(AVFormatContext *s)
 
 int ff_audio_interleave_init(AVFormatContext *s,
                              const int samples_per_frame,
+                             const int frame_duration,
                              AVRational time_base)
 {
     int i;
@@ -48,10 +49,15 @@  int ff_audio_interleave_init(AVFormatContext *s,
         av_log(s, AV_LOG_ERROR, "timebase not set for audio interleave\n");
         return AVERROR(EINVAL);
     }
+    if (!frame_duration) {
+        av_log(s, AV_LOG_ERROR, "frame_duration not set for audio interleave\n");
+        return AVERROR(EINVAL);
+    }
     for (i = 0; i < s->nb_streams; i++) {
         AVStream *st = s->streams[i];
         AudioInterleaveContext *aic = st->priv_data;
 
+        aic->start_dts = AV_NOPTS_VALUE;
         if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
             int max_samples = samples_per_frame ? samples_per_frame :
                               av_rescale_rnd(st->codecpar->sample_rate, time_base.num, time_base.den, AV_ROUND_UP);
@@ -67,6 +73,8 @@  int ff_audio_interleave_init(AVFormatContext *s,
             if (!(aic->fifo = av_fifo_alloc_array(100, max_samples)))
                 return AVERROR(ENOMEM);
             aic->fifo_size = 100 * max_samples;
+        } else {
+            aic->frame_duration = frame_duration;
         }
     }
 
@@ -94,10 +102,9 @@  static int interleave_new_audio_packet(AVFormatContext *s, AVPacket *pkt,
     if (size < pkt->size)
         memset(pkt->data + size, 0, pkt->size - size);
 
-    pkt->dts = pkt->pts = aic->dts;
+    pkt->dts = pkt->pts = av_rescale_q(aic->nb_samples, st->time_base, aic->time_base);
     pkt->duration = av_rescale_q(nb_samples, st->time_base, aic->time_base);
     pkt->stream_index = stream_index;
-    aic->dts += pkt->duration;
     aic->nb_samples += nb_samples;
     aic->n++;
 
@@ -122,9 +129,17 @@  int ff_audio_rechunk_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt
             }
             av_fifo_generic_write(aic->fifo, pkt->data, pkt->size, NULL);
         } else {
+            if (aic->start_dts == AV_NOPTS_VALUE)
+                aic->start_dts = pkt->dts;
+            if (pkt->dts != aic->dts + aic->start_dts) {
+                av_log(s, AV_LOG_WARNING, "Non-linear DTS in stream %d: got %"PRId64" instead of %"PRId64","
+                       "this can cause sync issues.\n", pkt->stream_index, pkt->dts, aic->dts + aic->start_dts);
+                aic->start_dts = pkt->dts - aic->dts;
+            }
+
             // rewrite pts and dts to be decoded time line position
             pkt->pts = pkt->dts = aic->dts;
-            aic->dts += pkt->duration;
+            aic->dts += aic->frame_duration;
             if ((ret = ff_interleave_add_packet(s, pkt, compare_ts)) < 0)
                 return ret;
         }
diff --git a/libavformat/audiointerleave.h b/libavformat/audiointerleave.h
index 0933310f4c..3ec4ed503a 100644
--- a/libavformat/audiointerleave.h
+++ b/libavformat/audiointerleave.h
@@ -31,13 +31,15 @@  typedef struct AudioInterleaveContext {
     unsigned fifo_size;           ///< size of currently allocated FIFO
     int64_t n;                    ///< number of generated packets
     int64_t nb_samples;           ///< number of generated samples
+    int64_t start_dts;            ///< start dts of a non-audio source stream
     uint64_t dts;                 ///< current dts
     int sample_size;              ///< size of one sample all channels included
     int samples_per_frame;        ///< samples per frame if fixed, 0 otherwise
+    int frame_duration;           ///< frame duration for non-audio data
     AVRational time_base;         ///< time base of output audio packets
 } AudioInterleaveContext;
 
-int ff_audio_interleave_init(AVFormatContext *s, const int samples_per_frame, AVRational time_base);
+int ff_audio_interleave_init(AVFormatContext *s, const int samples_per_frame, const int frame_duration, AVRational time_base);
 void ff_audio_interleave_close(AVFormatContext *s);
 
 /**
diff --git a/libavformat/gxfenc.c b/libavformat/gxfenc.c
index e7536a6a7e..552cc57a3f 100644
--- a/libavformat/gxfenc.c
+++ b/libavformat/gxfenc.c
@@ -818,7 +818,7 @@  static int gxf_write_header(AVFormatContext *s)
         sc->order = s->nb_streams - st->index;
     }
 
-    if (ff_audio_interleave_init(s, GXF_samples_per_frame, (AVRational){ 1, 48000 }) < 0)
+    if (ff_audio_interleave_init(s, GXF_samples_per_frame, 2, (AVRational){ 1, 48000 }) < 0)
         return -1;
 
     if (tcr && vsc)
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 6279ba9d6d..ac409d9ccf 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2646,7 +2646,7 @@  static int mxf_write_header(AVFormatContext *s)
         return AVERROR(ENOMEM);
     mxf->timecode_track->index = -1;
 
-    if (ff_audio_interleave_init(s, 0, av_inv_q(mxf->tc.rate)) < 0)
+    if (ff_audio_interleave_init(s, 0, 1, av_inv_q(mxf->tc.rate)) < 0)
         return -1;
 
     return 0;