Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH v3] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx
@ 2022-08-16 14:48 Derek Buitenhuis
  2022-08-16 16:21 ` Zhao Zhili
  0 siblings, 1 reply; 5+ messages in thread
From: Derek Buitenhuis @ 2022-08-16 14:48 UTC (permalink / raw)
  To: ffmpeg-devel

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 | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 31f3249ca6..1d8c5b2904 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];
+        if (!frag_stream) {
+            av_log(s, AV_LOG_WARNING, "No stream matching sidx ID found.\n");
+            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;
 }
 
-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)
@@ -8862,7 +8864,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".

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [FFmpeg-devel] [PATCH v3] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx
  2022-08-16 14:48 [FFmpeg-devel] [PATCH v3] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx Derek Buitenhuis
@ 2022-08-16 16:21 ` Zhao Zhili
  2022-08-16 16:32   ` Derek Buitenhuis
  0 siblings, 1 reply; 5+ messages in thread
From: Zhao Zhili @ 2022-08-16 16:21 UTC (permalink / raw)
  To: 'FFmpeg development discussions and patches'



> -----Original Message-----
> From: ffmpeg-devel-bounces@ffmpeg.org <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Derek Buitenhuis
> Sent: 2022年8月16日 22:49
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v3] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx
> 
> 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 | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 31f3249ca6..1d8c5b2904 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)

get_frag_time() can be called with an mp4 file which has no sidx at all. In that case,
dst_st should have a higher priority than other streams, even if sc->has_sidx is false.
And first_tfra_pts might be used here, which makes the check of sc->has_sidx unnatural.
So in my opinion, the check on sc->has_sidx should be removed.

Secondly, this part can be simplified with get_stream_info_time(), which is:

+    frag_stream_info = get_frag_stream_info(frag_index, index, dst_st->id);
+    timestamp = get_stream_info_time(frag_stream_info);
+    if (timestamp != AV_NOPTS_VALUE)
+        return timestamp;

Sorry I missed this part with patch v2, because I can't remember what did myself
mean in the replying to patch v1 😊

> @@ -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];
> +        if (!frag_stream) {
> +            av_log(s, AV_LOG_WARNING, "No stream matching sidx ID found.\n");

Neither frag_stream_info->id nor s->streams[j]->id comes from sidx ID.

> +            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;
>  }
> 
> -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)
> @@ -8862,7 +8864,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".

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [FFmpeg-devel] [PATCH v3] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx
  2022-08-16 16:21 ` Zhao Zhili
@ 2022-08-16 16:32   ` Derek Buitenhuis
  2022-08-17 10:29     ` Zhao Zhili
  0 siblings, 1 reply; 5+ messages in thread
From: Derek Buitenhuis @ 2022-08-16 16:32 UTC (permalink / raw)
  To: ffmpeg-devel

On 8/16/2022 5:21 PM, Zhao Zhili wrote:
> get_frag_time() can be called with an mp4 file which has no sidx at all. In that case,
> dst_st should have a higher priority than other streams, even if sc->has_sidx is false.
> And first_tfra_pts might be used here, which makes the check of sc->has_sidx unnatural.
> So in my opinion, the check on sc->has_sidx should be removed.

This seems like it should be in a separate patch, though - it is changing a different
behavior than what this patch does.

> +    frag_stream_info = get_frag_stream_info(frag_index, index, dst_st->id);
> +    timestamp = get_stream_info_time(frag_stream_info);
> +    if (timestamp != AV_NOPTS_VALUE)
> +        return timestamp;

I did look at that, but I do not think it can be.

get_stream_info_time is not equivalent to what is here. get_stream_info_time will
eventually fall back to frag_stream_info->tfdt_dts, where as this code falls back
to frag_stream_info->sidx_pts even if it is AV_NOPTS_VALUE. It would be a behavior
change do use get_stream_info_time here.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [FFmpeg-devel] [PATCH v3] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx
  2022-08-16 16:32   ` Derek Buitenhuis
@ 2022-08-17 10:29     ` Zhao Zhili
  2022-08-19 16:38       ` Derek Buitenhuis
  0 siblings, 1 reply; 5+ messages in thread
From: Zhao Zhili @ 2022-08-17 10:29 UTC (permalink / raw)
  To: ffmpeg-devel

On Tue, 2022-08-16 at 17:32 +0100, Derek Buitenhuis wrote:
> On 8/16/2022 5:21 PM, Zhao Zhili wrote:
> > get_frag_time() can be called with an mp4 file which has no sidx at
> > all. In that case,
> > dst_st should have a higher priority than other streams, even if
> > sc->has_sidx is false.
> > And first_tfra_pts might be used here, which makes the check of sc-
> > >has_sidx unnatural.
> > So in my opinion, the check on sc->has_sidx should be removed.
> 
> This seems like it should be in a separate patch, though - it is
> changing a different
> behavior than what this patch does.

OK.

> 
> > +    frag_stream_info = get_frag_stream_info(frag_index, index,
> > dst_st->id);
> > +    timestamp = get_stream_info_time(frag_stream_info);
> > +    if (timestamp != AV_NOPTS_VALUE)
> > +        return timestamp;
> 
> I did look at that, but I do not think it can be.
> 
> get_stream_info_time is not equivalent to what is here.
> get_stream_info_time will
> eventually fall back to frag_stream_info->tfdt_dts, where as this
> code falls back
> to frag_stream_info->sidx_pts even if it is AV_NOPTS_VALUE. It would
> be a behavior
> change do use get_stream_info_time here.

OK, it make sense together with `if (sc->has_sidx)`.

No more comments from me.

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [FFmpeg-devel] [PATCH v3] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx
  2022-08-17 10:29     ` Zhao Zhili
@ 2022-08-19 16:38       ` Derek Buitenhuis
  0 siblings, 0 replies; 5+ messages in thread
From: Derek Buitenhuis @ 2022-08-19 16:38 UTC (permalink / raw)
  To: ffmpeg-devel

On 8/17/2022 11:29 AM, Zhao Zhili wrote:
>> This seems like it should be in a separate patch, though - it is
>> changing a different
>> behavior than what this patch does.
> 
> OK.

Will send a 2nd patch with this in the next few days.


>> I did look at that, but I do not think it can be.
>>
>> get_stream_info_time is not equivalent to what is here.
>> get_stream_info_time will
>> eventually fall back to frag_stream_info->tfdt_dts, where as this
>> code falls back
>> to frag_stream_info->sidx_pts even if it is AV_NOPTS_VALUE. It would
>> be a behavior
>> change do use get_stream_info_time here.
> 
> OK, it make sense together with `if (sc->has_sidx)`.
> 
> No more comments from me.

Applied. Thank you for your reviews :).

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-08-19 16:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16 14:48 [FFmpeg-devel] [PATCH v3] mov: Compare frag times in correct time base when seeking a stream without a corresponding sidx Derek Buitenhuis
2022-08-16 16:21 ` Zhao Zhili
2022-08-16 16:32   ` Derek Buitenhuis
2022-08-17 10:29     ` Zhao Zhili
2022-08-19 16:38       ` Derek Buitenhuis

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git