[FFmpeg-devel] [PATCH] avformat/mov: adjust skip_samples according to seek timestamp

[Matthieu Bouron]

On Tue, Jan 05, 2021 at 09:56:39PM +0100, Marton Balint wrote:
> 
> On Tue, 5 Jan 2021, Matthieu Bouron wrote:
> 
> > > Could you factorize this to a function? It seems you are doing exactly the
> > > same thing here and below.
> > 
> > New patch attached.
> 
> [...]
> 
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index c6a2d9c388d..e6d0de38c6f 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -8122,6 +8122,15 @@ static int mov_seek_stream(AVFormatContext *s, AVStream *st, int64_t timestamp,
> >      return sample;
> >  }
> > 
> > +static int64_t mov_get_skip_samples(AVStream *st, int64_t seek_ts)
> > +{
> 
> Maybe cleaner to make "sample" a parameter instead of seek_ts.

Done.

> 
> > +    MOVStreamContext *sc = st->priv_data;
> > +    int64_t first_ts = st->internal->index_entries[0].timestamp;
> > +
> > +    /* compute skip samples according to stream start_pad, seek ts and first ts */
> > +    return FFMAX(sc->start_pad - (seek_ts - first_ts), 0);
> 
> Are you sure you don't need any time base correction here? Difference of
> timestamps is in stream time base, but skip_samples is the number of samples
> is in 1/sample_rate time base...

Done.

> 
> Also you are setting skip_samples for all streams, shouldn't you restrict it
> to audio only, so return 0 for non-audio streams?

I agree, it should only makes sense for audio streams. Updated.

> 
> > +}
> > +
> >  static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_time, int flags)
> >  {
> >      MOVContext *mc = s->priv_data;
> > @@ -8140,18 +8149,21 @@ static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
> >      if (mc->seek_individually) {
> >          /* adjust seek timestamp to found sample timestamp */
> >          int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
> > +        st->internal->skip_samples = mov_get_skip_samples(st, seek_timestamp);
> > 
> >          for (i = 0; i < s->nb_streams; i++) {
> >              int64_t timestamp;
> > -            MOVStreamContext *sc = s->streams[i]->priv_data;
> >              st = s->streams[i];
> > -            st->internal->skip_samples = (sample_time <= 0) ? sc->start_pad : 0;
> > 
> >              if (stream_index == i)
> >                  continue;
> > 
> >              timestamp = av_rescale_q(seek_timestamp, s->streams[stream_index]->time_base, st->time_base);
> > -            mov_seek_stream(s, st, timestamp, flags);
> > +            sample = mov_seek_stream(s, st, timestamp, flags);
> > +            if (sample >= 0) {
> > +                seek_timestamp = st->internal->index_entries[sample].timestamp;
> 
> This is destorying seek_timestamp, which seems unintended.

It was unintended. Fixed.

> 
> > +                st->internal->skip_samples = mov_get_skip_samples(st, seek_timestamp);
> > +            }

New patch attached.

[...]

-- 
Matthieu B.
-------------- next part --------------
>From 82ed68085304ef98215bc1189d09f85c7c70fc0c Mon Sep 17 00:00:00 2001
From: Matthieu Bouron <matthieu.bouron at gmail.com>
Date: Fri, 30 Oct 2020 15:38:51 +0100
Subject: [PATCH] avformat/mov: adjust skip_samples according to seek timestamp

Currently skip_samples is set to start_pad if sample_time is lesser or
equal to 0. This can cause issues if the stream starts with packets that
have negative pts. Calling avformat_seek_file() with ts set to 0 on such
streams makes the mov demuxer return the right corresponding packets
(near the 0 timestamp) but set skip_samples to start_pad which is
incorrect as the audio decoder will discard the returned samples
according to skip_samples from the first packet it receives (which has
its timestamp near 0).

For example, considering the following audio stream with start_pad=1344:

 [PKT pts=-1344] [PKT pts=-320] [PKT pts=704] [PKT pts=1728] [...]

Calling avformat_seek_file() with ts=0 makes the next call to
av_read_frame() return the packet with pts=-320 and a skip samples
side data set to 1344 (start_pad). This makes the audio decoder
incorrectly discard (1344 - 320) samples.

This commit makes the move demuxer adjust skip_samples according to the
stream start_pad, seek timestamp and first sample timestamp.

The above example will now result in av_read_frame() still returning the
packet with pts=-320 but with a skip samples side data set to 320
(src_pad - (seek_timestamp - first_timestamp)). This makes the audio
decoder only discard 320 samples (from pts=-320 to pts=0).
---
 libavformat/mov.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index c6a2d9c388d..3215b536362 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -8122,6 +8122,22 @@ static int mov_seek_stream(AVFormatContext *s, AVStream *st, int64_t timestamp,
     return sample;
 }
 
+static int64_t mov_get_skip_samples(AVStream *st, int sample)
+{
+    MOVStreamContext *sc = st->priv_data;
+    int64_t first_ts = st->internal->index_entries[0].timestamp;
+    int64_t ts = st->internal->index_entries[sample].timestamp;
+    int64_t off;
+
+    if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
+        return 0;
+
+    /* compute skip samples according to stream start_pad, seek ts and first ts */
+    off = av_rescale_q(ts - first_ts, st->time_base,
+                       (AVRational){1, st->codecpar->sample_rate});
+    return FFMAX(sc->start_pad - off, 0);
+}
+
 static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_time, int flags)
 {
     MOVContext *mc = s->priv_data;
@@ -8140,18 +8156,19 @@ static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
     if (mc->seek_individually) {
         /* adjust seek timestamp to found sample timestamp */
         int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
+        st->internal->skip_samples = mov_get_skip_samples(st, sample);
 
         for (i = 0; i < s->nb_streams; i++) {
             int64_t timestamp;
-            MOVStreamContext *sc = s->streams[i]->priv_data;
             st = s->streams[i];
-            st->internal->skip_samples = (sample_time <= 0) ? sc->start_pad : 0;
 
             if (stream_index == i)
                 continue;
 
             timestamp = av_rescale_q(seek_timestamp, s->streams[stream_index]->time_base, st->time_base);
-            mov_seek_stream(s, st, timestamp, flags);
+            sample = mov_seek_stream(s, st, timestamp, flags);
+            if (sample >= 0)
+                st->internal->skip_samples = mov_get_skip_samples(st, sample);
         }
     } else {
         for (i = 0; i < s->nb_streams; i++) {
-- 
2.30.0