diff mbox series

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

Message ID 20201102114219.459524-1-matthieu.bouron@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/mov: adjust skip_samples according to seek timestamp
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Matthieu Bouron Nov. 2, 2020, 11:42 a.m. UTC
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 | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Matthieu Bouron Nov. 9, 2020, 5:26 p.m. UTC | #1
On Mon, Nov 02, 2020 at 12:42:19PM +0100, Matthieu Bouron wrote:
> 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 | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index dd0db6bca79..f99cb0df25a 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -8098,20 +8098,34 @@ static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
>          return sample;
>  
>      if (mc->seek_individually) {
> +        MOVStreamContext *sc = s->streams[stream_index]->priv_data;
> +
>          /* adjust seek timestamp to found sample timestamp */
> +        int64_t first_timestamp = st->internal->index_entries[0].timestamp;
>          int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
>  
> +        /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> +        int64_t skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> +        st->internal->skip_samples = skip_samples;
> +
>          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) {
> +                first_timestamp = st->internal->index_entries[0].timestamp;
> +                seek_timestamp = st->internal->index_entries[sample].timestamp;
> +
> +                /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> +                skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> +                st->internal->skip_samples = skip_samples;
> +            }
>          }
>      } else {
>          for (i = 0; i < s->nb_streams; i++) {
> -- 
> 2.29.2
> 

Ping.
Matthieu Bouron Nov. 17, 2020, 1:18 p.m. UTC | #2
On Mon, Nov 09, 2020 at 06:26:46PM +0100, Matthieu Bouron wrote:
> On Mon, Nov 02, 2020 at 12:42:19PM +0100, Matthieu Bouron wrote:
> > 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 | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index dd0db6bca79..f99cb0df25a 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -8098,20 +8098,34 @@ static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
> >          return sample;
> >  
> >      if (mc->seek_individually) {
> > +        MOVStreamContext *sc = s->streams[stream_index]->priv_data;
> > +
> >          /* adjust seek timestamp to found sample timestamp */
> > +        int64_t first_timestamp = st->internal->index_entries[0].timestamp;
> >          int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
> >  
> > +        /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> > +        int64_t skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> > +        st->internal->skip_samples = skip_samples;
> > +
> >          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) {
> > +                first_timestamp = st->internal->index_entries[0].timestamp;
> > +                seek_timestamp = st->internal->index_entries[sample].timestamp;
> > +
> > +                /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> > +                skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> > +                st->internal->skip_samples = skip_samples;
> > +            }
> >          }
> >      } else {
> >          for (i = 0; i < s->nb_streams; i++) {
> > -- 
> > 2.29.2
> > 
> 
> Ping.
> 

Ping.
Matthieu Bouron Nov. 23, 2020, 4:43 p.m. UTC | #3
On Tue, Nov 17, 2020 at 02:18:46PM +0100, Matthieu Bouron wrote:
> On Mon, Nov 09, 2020 at 06:26:46PM +0100, Matthieu Bouron wrote:
> > On Mon, Nov 02, 2020 at 12:42:19PM +0100, Matthieu Bouron wrote:
> > > 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 | 18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > index dd0db6bca79..f99cb0df25a 100644
> > > --- a/libavformat/mov.c
> > > +++ b/libavformat/mov.c
> > > @@ -8098,20 +8098,34 @@ static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
> > >          return sample;
> > >  
> > >      if (mc->seek_individually) {
> > > +        MOVStreamContext *sc = s->streams[stream_index]->priv_data;
> > > +
> > >          /* adjust seek timestamp to found sample timestamp */
> > > +        int64_t first_timestamp = st->internal->index_entries[0].timestamp;
> > >          int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
> > >  
> > > +        /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> > > +        int64_t skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> > > +        st->internal->skip_samples = skip_samples;
> > > +
> > >          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) {
> > > +                first_timestamp = st->internal->index_entries[0].timestamp;
> > > +                seek_timestamp = st->internal->index_entries[sample].timestamp;
> > > +
> > > +                /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> > > +                skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> > > +                st->internal->skip_samples = skip_samples;
> > > +            }
> > >          }
> > >      } else {
> > >          for (i = 0; i < s->nb_streams; i++) {
> > > -- 
> > > 2.29.2
> > > 
> > 
> > Ping.
> > 
> 
> Ping.
> 

Ping.
Matthieu Bouron Jan. 4, 2021, 9:32 a.m. UTC | #4
On Mon, Nov 23, 2020 at 05:43:06PM +0100, Matthieu Bouron wrote:
> On Tue, Nov 17, 2020 at 02:18:46PM +0100, Matthieu Bouron wrote:
> > On Mon, Nov 09, 2020 at 06:26:46PM +0100, Matthieu Bouron wrote:
> > > On Mon, Nov 02, 2020 at 12:42:19PM +0100, Matthieu Bouron wrote:
> > > > 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 | 18 ++++++++++++++++--
> > > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > > index dd0db6bca79..f99cb0df25a 100644
> > > > --- a/libavformat/mov.c
> > > > +++ b/libavformat/mov.c
> > > > @@ -8098,20 +8098,34 @@ static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
> > > >          return sample;
> > > >  
> > > >      if (mc->seek_individually) {
> > > > +        MOVStreamContext *sc = s->streams[stream_index]->priv_data;
> > > > +
> > > >          /* adjust seek timestamp to found sample timestamp */
> > > > +        int64_t first_timestamp = st->internal->index_entries[0].timestamp;
> > > >          int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
> > > >  
> > > > +        /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> > > > +        int64_t skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> > > > +        st->internal->skip_samples = skip_samples;
> > > > +
> > > >          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) {
> > > > +                first_timestamp = st->internal->index_entries[0].timestamp;
> > > > +                seek_timestamp = st->internal->index_entries[sample].timestamp;
> > > > +
> > > > +                /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> > > > +                skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> > > > +                st->internal->skip_samples = skip_samples;
> > > > +            }
> > > >          }
> > > >      } else {
> > > >          for (i = 0; i < s->nb_streams; i++) {
> > > > -- 
> > > > 2.29.2
> > > > 
> > > 
> > > Ping.
> > > 
> > 
> > Ping.
> > 
> 
> Ping.
> 

Ping.
Marton Balint Jan. 4, 2021, 8:04 p.m. UTC | #5
On Mon, 2 Nov 2020, Matthieu Bouron wrote:

> 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 | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index dd0db6bca79..f99cb0df25a 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -8098,20 +8098,34 @@ static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
>         return sample;
>
>     if (mc->seek_individually) {
> +        MOVStreamContext *sc = s->streams[stream_index]->priv_data;
> +
>         /* adjust seek timestamp to found sample timestamp */
> +        int64_t first_timestamp = st->internal->index_entries[0].timestamp;
>         int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
> 
> +        /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> +        int64_t skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> +        st->internal->skip_samples = skip_samples;
> +

Could you factorize this to a function? It seems you are doing exactly the 
same thing here and below.

Thanks,
Marton

>         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) {
> +                first_timestamp = st->internal->index_entries[0].timestamp;
> +                seek_timestamp = st->internal->index_entries[sample].timestamp;
> +
> +                /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> +                skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> +                st->internal->skip_samples = skip_samples;
> +            }
>         }
>     } else {
>         for (i = 0; i < s->nb_streams; i++) {
> -- 
> 2.29.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Matthieu Bouron Jan. 5, 2021, 9:25 a.m. UTC | #6
On Mon, Jan 04, 2021 at 09:04:05PM +0100, Marton Balint wrote:
> 
> 
> On Mon, 2 Nov 2020, Matthieu Bouron wrote:
> 
> > 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 | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index dd0db6bca79..f99cb0df25a 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -8098,20 +8098,34 @@ static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
> >         return sample;
> > 
> >     if (mc->seek_individually) {
> > +        MOVStreamContext *sc = s->streams[stream_index]->priv_data;
> > +
> >         /* adjust seek timestamp to found sample timestamp */
> > +        int64_t first_timestamp = st->internal->index_entries[0].timestamp;
> >         int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
> > 
> > +        /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
> > +        int64_t skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
> > +        st->internal->skip_samples = skip_samples;
> > +
> 
> Could you factorize this to a function? It seems you are doing exactly the
> same thing here and below.

New patch attached.

[...]

Thanks,
Marton Balint Jan. 5, 2021, 8:56 p.m. UTC | #7
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.

> +    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...

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

> +}
> +
>  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.

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

Regards,
Marton
Matthieu Bouron Jan. 6, 2021, 9:31 p.m. UTC | #8
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.

[...]
Marton Balint Jan. 9, 2021, 4:29 p.m. UTC | #9
On Wed, 6 Jan 2021, Matthieu Bouron wrote:

> 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.

Thanks, applied.

Regards,
Marton
diff mbox series

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index dd0db6bca79..f99cb0df25a 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -8098,20 +8098,34 @@  static int mov_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
         return sample;
 
     if (mc->seek_individually) {
+        MOVStreamContext *sc = s->streams[stream_index]->priv_data;
+
         /* adjust seek timestamp to found sample timestamp */
+        int64_t first_timestamp = st->internal->index_entries[0].timestamp;
         int64_t seek_timestamp = st->internal->index_entries[sample].timestamp;
 
+        /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
+        int64_t skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
+        st->internal->skip_samples = skip_samples;
+
         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) {
+                first_timestamp = st->internal->index_entries[0].timestamp;
+                seek_timestamp = st->internal->index_entries[sample].timestamp;
+
+                /* adjust skip samples according to stream start_pad, seek timestamp and first timestamp */
+                skip_samples = FFMAX(sc->start_pad - (seek_timestamp - first_timestamp), 0);
+                st->internal->skip_samples = skip_samples;
+            }
         }
     } else {
         for (i = 0; i < s->nb_streams; i++) {