| Message ID | 20220803131859.167392-1-derek.buitenhuis@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 8/3/2022 2:18 PM, Derek Buitenhuis wrote: > Some muxers, such as GPAC, create files with only one sidx, but two streams > muxed into the same fragments pointed to by this sidx. > > Prevously, in such a case, when we seeked in such files, we fell back > to, for example, using the sidx associated with the video stream, to > seek the audio stream, leaving the seekhead in the wrong place. > > We can still do this, but we need to take care to compare timestamps > in the same time base. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/mov.c | 40 +++++++++++++++++++++------------------- > 1 file changed, 21 insertions(+), 19 deletions(-) Will push in a day or so if there are no objections. - Derek
> On Aug 3, 2022, at 9:18 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > Some muxers, such as GPAC, create files with only one sidx, but two streams > muxed into the same fragments pointed to by this sidx. > > Prevously, in such a case, when we seeked in such files, we fell back > to, for example, using the sidx associated with the video stream, to > seek the audio stream, leaving the seekhead in the wrong place. > > We can still do this, but we need to take care to compare timestamps > in the same time base. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/mov.c | 40 +++++++++++++++++++++------------------- > 1 file changed, 21 insertions(+), 19 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index a09a762d91..c101a1acdd 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -1271,15 +1271,18 @@ static int64_t get_stream_info_time(MOVFragmentStreamInfo * frag_stream_info) > return frag_stream_info->tfdt_dts; > } > > -static int64_t get_frag_time(MOVFragmentIndex *frag_index, > - int index, int track_id) > +static int64_t get_frag_time(AVFormatContext *s, AVStream *dst_st, > + MOVFragmentIndex *frag_index, int index) > { > MOVFragmentStreamInfo * frag_stream_info; > + MOVStreamContext *sc = dst_st->priv_data; > int64_t timestamp; > - int i; > + int i, j; > > - if (track_id >= 0) { > - frag_stream_info = get_frag_stream_info(frag_index, index, track_id); > + // If the stream is referenced by any sidx, limit the search > + // to fragments that referenced this stream in the sidx > + if (sc->has_sidx) { > + frag_stream_info = get_frag_stream_info(frag_index, index, dst_st->id); > if (frag_stream_info->sidx_pts != AV_NOPTS_VALUE) > return frag_stream_info->sidx_pts; > if (frag_stream_info->first_tfra_pts != AV_NOPTS_VALUE) > @@ -1288,28 +1291,27 @@ static int64_t get_frag_time(MOVFragmentIndex *frag_index, > } > > for (i = 0; i < frag_index->item[index].nb_stream_info; i++) { > + AVStream *frag_stream = NULL; > frag_stream_info = &frag_index->item[index].stream_info[i]; > + for (j = 0; j < s->nb_streams; j++) > + if (s->streams[j]->id == frag_stream_info->id) > + frag_stream = s->streams[j]; > timestamp = get_stream_info_time(frag_stream_info); > - if (timestamp != AV_NOPTS_VALUE) > - return timestamp; > + if (timestamp != AV_NOPTS_VALUE) { > + if (frag_stream) > + return av_rescale_q(timestamp, frag_stream->time_base, dst_st->time_base); > + else > + return timestamp; It’s suspicious to return a timestamp with unknown timebase. > + } > } > return AV_NOPTS_VALUE; > } > > -static int search_frag_timestamp(MOVFragmentIndex *frag_index, > +static int search_frag_timestamp(AVFormatContext *s, MOVFragmentIndex *frag_index, > AVStream *st, int64_t timestamp) > { > int a, b, m, m0; > int64_t frag_time; > - int id = -1; > - > - if (st) { > - // If the stream is referenced by any sidx, limit the search > - // to fragments that referenced this stream in the sidx > - MOVStreamContext *sc = st->priv_data; > - if (sc->has_sidx) > - id = st->id; > - } > > a = -1; > b = frag_index->nb_items; > @@ -1318,7 +1320,7 @@ static int search_frag_timestamp(MOVFragmentIndex *frag_index, > m0 = m = (a + b) >> 1; > > while (m < b && > - (frag_time = get_frag_time(frag_index, m, id)) == AV_NOPTS_VALUE) > + (frag_time = get_frag_time(s, st, frag_index, m)) == AV_NOPTS_VALUE) > m++; > > if (m < b && frag_time <= timestamp) > @@ -8806,7 +8808,7 @@ static int mov_seek_fragment(AVFormatContext *s, AVStream *st, int64_t timestamp > if (!mov->frag_index.complete) > return 0; > > - index = search_frag_timestamp(&mov->frag_index, st, timestamp); > + index = search_frag_timestamp(s, &mov->frag_index, st, timestamp); > if (index < 0) > index = 0; > if (!mov->frag_index.item[index].headers_read) > -- > 2.36.1 > > _______________________________________________ > 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".
On 8/9/2022 10:38 AM, "zhilizhao(赵志立)" wrote:
> It’s suspicious to return a timestamp with unknown timebase.
Would you suggest erroring in the case where there's no frag_stream? That could
make sense.
- Derek
On Fri, 2022-08-12 at 16:22 +0100, Derek Buitenhuis wrote: > On 8/9/2022 10:38 AM, "zhilizhao(赵志立)" wrote: > > It’s suspicious to return a timestamp with unknown timebase. > > Would you suggest erroring in the case where there's no frag_stream? > That could > make sense. Or just continue the loop, and print a warning message maybe? @@ -1288,28 +1291,25 @@ static int64_t get_frag_time(MOVFragmentIndex *frag_index, } for (i = 0; i < frag_index->item[index].nb_stream_info; i++) { + AVStream *frag_stream = NULL; frag_stream_info = &frag_index->item[index].stream_info[i]; + for (j = 0; j < s->nb_streams; j++) + if (s->streams[j]->id == frag_stream_info->id) + frag_stream = s->streams[j]; + if (!frag_stream) + continue; timestamp = get_stream_info_time(frag_stream_info); if (timestamp != AV_NOPTS_VALUE) - return timestamp; + return av_rescale_q(timestamp, frag_stream->time_base, dst_st->time_base); } return AV_NOPTS_VALUE; } > > - Derek > _______________________________________________ > 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".
On 8/14/2022 7:55 AM, Zhao Zhili wrote:
> Or just continue the loop, and print a warning message maybe?
Done, and v3 sent.
- Derek
diff --git a/libavformat/mov.c b/libavformat/mov.c index a09a762d91..c101a1acdd 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -1271,15 +1271,18 @@ static int64_t get_stream_info_time(MOVFragmentStreamInfo * frag_stream_info) return frag_stream_info->tfdt_dts; } -static int64_t get_frag_time(MOVFragmentIndex *frag_index, - int index, int track_id) +static int64_t get_frag_time(AVFormatContext *s, AVStream *dst_st, + MOVFragmentIndex *frag_index, int index) { MOVFragmentStreamInfo * frag_stream_info; + MOVStreamContext *sc = dst_st->priv_data; int64_t timestamp; - int i; + int i, j; - if (track_id >= 0) { - frag_stream_info = get_frag_stream_info(frag_index, index, track_id); + // If the stream is referenced by any sidx, limit the search + // to fragments that referenced this stream in the sidx + if (sc->has_sidx) { + frag_stream_info = get_frag_stream_info(frag_index, index, dst_st->id); if (frag_stream_info->sidx_pts != AV_NOPTS_VALUE) return frag_stream_info->sidx_pts; if (frag_stream_info->first_tfra_pts != AV_NOPTS_VALUE) @@ -1288,28 +1291,27 @@ static int64_t get_frag_time(MOVFragmentIndex *frag_index, } for (i = 0; i < frag_index->item[index].nb_stream_info; i++) { + AVStream *frag_stream = NULL; frag_stream_info = &frag_index->item[index].stream_info[i]; + for (j = 0; j < s->nb_streams; j++) + if (s->streams[j]->id == frag_stream_info->id) + frag_stream = s->streams[j]; timestamp = get_stream_info_time(frag_stream_info); - if (timestamp != AV_NOPTS_VALUE) - return timestamp; + if (timestamp != AV_NOPTS_VALUE) { + if (frag_stream) + return av_rescale_q(timestamp, frag_stream->time_base, dst_st->time_base); + else + return timestamp; + } } return AV_NOPTS_VALUE; } -static int search_frag_timestamp(MOVFragmentIndex *frag_index, +static int search_frag_timestamp(AVFormatContext *s, MOVFragmentIndex *frag_index, AVStream *st, int64_t timestamp) { int a, b, m, m0; int64_t frag_time; - int id = -1; - - if (st) { - // If the stream is referenced by any sidx, limit the search - // to fragments that referenced this stream in the sidx - MOVStreamContext *sc = st->priv_data; - if (sc->has_sidx) - id = st->id; - } a = -1; b = frag_index->nb_items; @@ -1318,7 +1320,7 @@ static int search_frag_timestamp(MOVFragmentIndex *frag_index, m0 = m = (a + b) >> 1; while (m < b && - (frag_time = get_frag_time(frag_index, m, id)) == AV_NOPTS_VALUE) + (frag_time = get_frag_time(s, st, frag_index, m)) == AV_NOPTS_VALUE) m++; if (m < b && frag_time <= timestamp) @@ -8806,7 +8808,7 @@ static int mov_seek_fragment(AVFormatContext *s, AVStream *st, int64_t timestamp if (!mov->frag_index.complete) return 0; - index = search_frag_timestamp(&mov->frag_index, st, timestamp); + index = search_frag_timestamp(s, &mov->frag_index, st, timestamp); if (index < 0) index = 0; if (!mov->frag_index.item[index].headers_read)
Some muxers, such as GPAC, create files with only one sidx, but two streams muxed into the same fragments pointed to by this sidx. Prevously, in such a case, when we seeked in such files, we fell back to, for example, using the sidx associated with the video stream, to seek the audio stream, leaving the seekhead in the wrong place. We can still do this, but we need to take care to compare timestamps in the same time base. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- libavformat/mov.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-)