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

[Marton Balint]

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