Re: [FFmpeg-devel] [PATCH 1/2] avformat/avformat: Introduced `AVInputFormat.read_timestamp2` to fix keyframe seeking for formats that rely on `read_timestamp` for seeking
On Sat, Sep 07, 2019 at 03:52:01PM -0700, Darren Mo wrote: > I think I understand some of what you are saying. You are saying that after > the bisection, continuously step backwards + read forward while doubling the > step size until the keyframe is found? yes > > Some questions: > - What is the difference between the lowercase k and the uppercase K in your > diagram? the K is the keyframe with largest timestamp before our target k is another keyframe which we encounter in the example, i added that to show that we cannot stop when finding a keyframe > And to make sure I didn’t misunderstand, v is the result of the bisection and > A/B/C indicate the steps? yes > - Why after finding the keyframe in step C, is there an extra -->? because we do not know if there are more keyframes in that area before looking and if there are they would be closer to our target timestamp > - Re: “there is code somewhere in git to do something similar already”, do > you have an idea of what I should search for to find this code? ff_find_last_ts() thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avformat/avformat: Introduced `AVInputFormat.read_timestamp2` to fix keyframe seeking for formats that rely on `read_timestamp` for seeking
I think I understand some of what you are saying. You are saying that after the bisection, continuously step backwards + read forward while doubling the step size until the keyframe is found? Some questions: - What is the difference between the lowercase k and the uppercase K in your diagram? And to make sure I didn’t misunderstand, v is the result of the bisection and A/B/C indicate the steps? - Why after finding the keyframe in step C, is there an extra -->? - Re: “there is code somewhere in git to do something similar already”, do you have an idea of what I should search for to find this code? Thanks! > On Sep 3, 2019, at 4:39 AM, Michael Niedermayer > wrote: > > stepwise searching for the keyframe in forward direction while taking steps > backward > ..k.K.v-start > A---> >B---> >C>k-K--> > > there is code somewhere in git to do something similar already > this should be faster than always searching for keyframes during bisection as > long as the initial step is reasonable in relation to the keyframe distance > > also as both approaches are quite similar one could even choose which > way to do it depending on the expected cost of seeks and bandwidth and > if some statistics on the keyframe distance are known and or the > search direction > > thx > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > There will always be a question for which you do not know the correct answer. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avformat/avformat: Introduced `AVInputFormat.read_timestamp2` to fix keyframe seeking for formats that rely on `read_timestamp` for seeking
On Mon, Sep 02, 2019 at 01:01:50PM -0700, Darren Mo wrote: > That’s true. However, I think your approach would produce incorrect results > when `AVSEEK_FLAG_BACKWARD` is used. > > The bisection would find the frame just before the target time and then we > would search for the next keyframe, which would be after the target time. > Instead, we want to return the keyframe just before the target time. > > Any ideas on how to do that more efficiently? stepwise searching for the keyframe in forward direction while taking steps backward ..k.K.v-start A---> B---> C>k-K--> there is code somewhere in git to do something similar already this should be faster than always searching for keyframes during bisection as long as the initial step is reasonable in relation to the keyframe distance also as both approaches are quite similar one could even choose which way to do it depending on the expected cost of seeks and bandwidth and if some statistics on the keyframe distance are known and or the search direction thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct answer. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avformat/avformat: Introduced `AVInputFormat.read_timestamp2` to fix keyframe seeking for formats that rely on `read_timestamp` for seeking
Yup! I just wanted to do the migration in phases rather than all at once so that I can do one demuxer at a time while gathering feedback. At the end of the migration, I’ll rename `read_timestamp2` to `read_timestamp`. > On Sep 2, 2019, at 1:07 PM, James Almer wrote: > > AVInputFormat.read_timestamp() is not a public field. There's no need to > deprecate it and add a replacement. Just change it as you see fit, > making sure to update all demuxers that may use it. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avformat/avformat: Introduced `AVInputFormat.read_timestamp2` to fix keyframe seeking for formats that rely on `read_timestamp` for seeking
On 8/31/2019 7:10 AM, fumoboy...@me.com wrote:
> From: fumoboy007
>
> If the user omits `AVSEEK_FLAG_ANY`, then the result of the seek should be a
> keyframe. `ff_gen_search` did not respect that contract because it had no
> good way to determine whether a timestamp returned by `read_timestamp` was
> for a keyframe packet.
>
> Therefore, we introduce `read_timestamp2`, which adds a new parameter named
> `prefer_keyframe`. This new parameter tells the input format whether to skip
> non-keyframe packets. The parameter is named `prefer_keyframe` instead of
> something like `keyframe_only` because some formats do not distinguish
> between keyframe and non-keyframe packets.
>
> This commit adds the new function and deprecates the old function. Subsequent
> commits will migrate input formats to the new function.
>
> Signed-off-by: fumoboy007
> ---
> libavformat/avformat.h | 9 +++
> libavformat/internal.h | 6 +++--
> libavformat/nutdec.c | 6 ++---
> libavformat/utils.c| 56 +-
> 4 files changed, 60 insertions(+), 17 deletions(-)
>
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 6eb329f13f..1db548663c 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -741,6 +741,7 @@ typedef struct AVInputFormat {
> * Get the next timestamp in stream[stream_index].time_base units.
> * @return the timestamp or AV_NOPTS_VALUE if an error occurred
> */
> +attribute_deprecated
> int64_t (*read_timestamp)(struct AVFormatContext *s, int stream_index,
>int64_t *pos, int64_t pos_limit);
>
> @@ -781,6 +782,14 @@ typedef struct AVInputFormat {
> * @see avdevice_capabilities_free() for more details.
> */
> int (*free_device_capabilities)(struct AVFormatContext *s, struct
> AVDeviceCapabilitiesQuery *caps);
> +
> +/**
> + * Get the next timestamp in stream[stream_index].time_base units.
> + * @param prefer_keyframe Whether to skip over non-keyframe packets (if
> possible).
> + * @return the timestamp or AV_NOPTS_VALUE if an error occurred
> + */
> +int64_t (*read_timestamp2)(struct AVFormatContext *s, int stream_index,
> + int64_t *pos, int64_t pos_limit, int
> prefer_keyframe);
> } AVInputFormat;
> /**
> * @}
AVInputFormat.read_timestamp() is not a public field. There's no need to
deprecate it and add a replacement. Just change it as you see fit,
making sure to update all demuxers that may use it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avformat/avformat: Introduced `AVInputFormat.read_timestamp2` to fix keyframe seeking for formats that rely on `read_timestamp` for seeking
That’s true. However, I think your approach would produce incorrect results when `AVSEEK_FLAG_BACKWARD` is used. The bisection would find the frame just before the target time and then we would search for the next keyframe, which would be after the target time. Instead, we want to return the keyframe just before the target time. Any ideas on how to do that more efficiently? > On Sep 2, 2019, at 7:33 AM, Michael Niedermayer > wrote: > > i think this is the wrong way to fix the issue unless iam missing something > > Bisecting on only keyframe packets needs to read > framesize * goplength * log(all frames) > Bisecting on any frame and only at the end searching for the next keyframe > needs > framesize * (log(all frames) + goplength) > that is more than an order of magnitude less data to read even with > small gop sizes > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > No human being will ever know the Truth, for even if they happen to say it > by chance, they would not even known they had done so. -- Xenophanes > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avformat/avformat: Introduced `AVInputFormat.read_timestamp2` to fix keyframe seeking for formats that rely on `read_timestamp` for seeking
On Sat, Aug 31, 2019 at 03:10:38AM -0700, fumoboy...@me.com wrote:
> From: fumoboy007
>
> If the user omits `AVSEEK_FLAG_ANY`, then the result of the seek should be a
> keyframe. `ff_gen_search` did not respect that contract because it had no
> good way to determine whether a timestamp returned by `read_timestamp` was
> for a keyframe packet.
>
> Therefore, we introduce `read_timestamp2`, which adds a new parameter named
> `prefer_keyframe`. This new parameter tells the input format whether to skip
> non-keyframe packets. The parameter is named `prefer_keyframe` instead of
> something like `keyframe_only` because some formats do not distinguish
> between keyframe and non-keyframe packets.
>
> This commit adds the new function and deprecates the old function. Subsequent
> commits will migrate input formats to the new function.
>
> Signed-off-by: fumoboy007
> ---
> libavformat/avformat.h | 9 +++
> libavformat/internal.h | 6 +++--
> libavformat/nutdec.c | 6 ++---
> libavformat/utils.c| 56 +-
> 4 files changed, 60 insertions(+), 17 deletions(-)
>
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 6eb329f13f..1db548663c 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -741,6 +741,7 @@ typedef struct AVInputFormat {
> * Get the next timestamp in stream[stream_index].time_base units.
> * @return the timestamp or AV_NOPTS_VALUE if an error occurred
> */
> +attribute_deprecated
> int64_t (*read_timestamp)(struct AVFormatContext *s, int stream_index,
>int64_t *pos, int64_t pos_limit);
>
> @@ -781,6 +782,14 @@ typedef struct AVInputFormat {
> * @see avdevice_capabilities_free() for more details.
> */
> int (*free_device_capabilities)(struct AVFormatContext *s, struct
> AVDeviceCapabilitiesQuery *caps);
> +
> +/**
> + * Get the next timestamp in stream[stream_index].time_base units.
> + * @param prefer_keyframe Whether to skip over non-keyframe packets (if
> possible).
> + * @return the timestamp or AV_NOPTS_VALUE if an error occurred
> + */
> +int64_t (*read_timestamp2)(struct AVFormatContext *s, int stream_index,
> + int64_t *pos, int64_t pos_limit, int
> prefer_keyframe);
> } AVInputFormat;
> /**
> * @}
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index d6a039c497..a45c538b21 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -443,7 +443,8 @@ int ff_seek_frame_binary(AVFormatContext *s, int
> stream_index,
> void ff_update_cur_dts(AVFormatContext *s, AVStream *ref_st, int64_t
> timestamp);
>
> int ff_find_last_ts(AVFormatContext *s, int stream_index, int64_t *ts,
> int64_t *pos,
> -int64_t (*read_timestamp)(struct AVFormatContext *, int
> , int64_t *, int64_t ));
> +int64_t (*read_timestamp)(struct AVFormatContext *, int,
> int64_t *, int64_t),
> +int64_t (*read_timestamp2)(struct AVFormatContext *,
> int, int64_t *, int64_t, int));
>
> /**
> * Perform a binary search using read_timestamp().
> @@ -456,7 +457,8 @@ int64_t ff_gen_search(AVFormatContext *s, int
> stream_index,
>int64_t pos_max, int64_t pos_limit,
>int64_t ts_min, int64_t ts_max,
>int flags, int64_t *ts_ret,
> - int64_t (*read_timestamp)(struct AVFormatContext *,
> int , int64_t *, int64_t ));
> + int64_t (*read_timestamp)(struct AVFormatContext *,
> int, int64_t *, int64_t),
> + int64_t (*read_timestamp2)(struct AVFormatContext *,
> int, int64_t *, int64_t, int));
>
> /**
> * Set the time base and wrapping info for a given stream. This will be used
> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
> index 979cb9a031..f28fb2297e 100644
> --- a/libavformat/nutdec.c
> +++ b/libavformat/nutdec.c
> @@ -653,7 +653,7 @@ static int64_t find_duration(NUTContext *nut, int64_t
> filesize)
> AVFormatContext *s = nut->avf;
> int64_t duration = 0;
>
> -ff_find_last_ts(s, -1, , NULL, nut_read_timestamp);
> +ff_find_last_ts(s, -1, , NULL, nut_read_timestamp, NULL);
>
> if(duration > 0)
> s->duration_estimation_method = AVFMT_DURATION_FROM_PTS;
> @@ -1251,7 +1251,7 @@ static int read_seek(AVFormatContext *s, int
> stream_index,
> pos = ff_gen_search(s, -1, dummy.ts, next_node[0]->pos,
> next_node[1]->pos, next_node[1]->pos,
> next_node[0]->ts, next_node[1]->ts,
> -AVSEEK_FLAG_BACKWARD, , nut_read_timestamp);
> +AVSEEK_FLAG_BACKWARD, , nut_read_timestamp,
> NULL);
> if (pos < 0)
> return pos;
>
> @@ -1263,7 +1263,7 @@ static int read_seek(AVFormatContext *s, int
>
[FFmpeg-devel] [PATCH 1/2] avformat/avformat: Introduced `AVInputFormat.read_timestamp2` to fix keyframe seeking for formats that rely on `read_timestamp` for seeking
From: fumoboy007
If the user omits `AVSEEK_FLAG_ANY`, then the result of the seek should be a
keyframe. `ff_gen_search` did not respect that contract because it had no good
way to determine whether a timestamp returned by `read_timestamp` was for a
keyframe packet.
Therefore, we introduce `read_timestamp2`, which adds a new parameter named
`prefer_keyframe`. This new parameter tells the input format whether to skip
non-keyframe packets. The parameter is named `prefer_keyframe` instead of
something like `keyframe_only` because some formats do not distinguish between
keyframe and non-keyframe packets.
This commit adds the new function and deprecates the old function. Subsequent
commits will migrate input formats to the new function.
Signed-off-by: fumoboy007
---
libavformat/avformat.h | 9 +++
libavformat/internal.h | 6 +++--
libavformat/nutdec.c | 6 ++---
libavformat/utils.c| 56 +-
4 files changed, 60 insertions(+), 17 deletions(-)
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 6eb329f13f..1db548663c 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -741,6 +741,7 @@ typedef struct AVInputFormat {
* Get the next timestamp in stream[stream_index].time_base units.
* @return the timestamp or AV_NOPTS_VALUE if an error occurred
*/
+attribute_deprecated
int64_t (*read_timestamp)(struct AVFormatContext *s, int stream_index,
int64_t *pos, int64_t pos_limit);
@@ -781,6 +782,14 @@ typedef struct AVInputFormat {
* @see avdevice_capabilities_free() for more details.
*/
int (*free_device_capabilities)(struct AVFormatContext *s, struct
AVDeviceCapabilitiesQuery *caps);
+
+/**
+ * Get the next timestamp in stream[stream_index].time_base units.
+ * @param prefer_keyframe Whether to skip over non-keyframe packets (if
possible).
+ * @return the timestamp or AV_NOPTS_VALUE if an error occurred
+ */
+int64_t (*read_timestamp2)(struct AVFormatContext *s, int stream_index,
+ int64_t *pos, int64_t pos_limit, int
prefer_keyframe);
} AVInputFormat;
/**
* @}
diff --git a/libavformat/internal.h b/libavformat/internal.h
index d6a039c497..a45c538b21 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -443,7 +443,8 @@ int ff_seek_frame_binary(AVFormatContext *s, int
stream_index,
void ff_update_cur_dts(AVFormatContext *s, AVStream *ref_st, int64_t
timestamp);
int ff_find_last_ts(AVFormatContext *s, int stream_index, int64_t *ts, int64_t
*pos,
-int64_t (*read_timestamp)(struct AVFormatContext *, int ,
int64_t *, int64_t ));
+int64_t (*read_timestamp)(struct AVFormatContext *, int,
int64_t *, int64_t),
+int64_t (*read_timestamp2)(struct AVFormatContext *, int,
int64_t *, int64_t, int));
/**
* Perform a binary search using read_timestamp().
@@ -456,7 +457,8 @@ int64_t ff_gen_search(AVFormatContext *s, int stream_index,
int64_t pos_max, int64_t pos_limit,
int64_t ts_min, int64_t ts_max,
int flags, int64_t *ts_ret,
- int64_t (*read_timestamp)(struct AVFormatContext *, int
, int64_t *, int64_t ));
+ int64_t (*read_timestamp)(struct AVFormatContext *, int,
int64_t *, int64_t),
+ int64_t (*read_timestamp2)(struct AVFormatContext *,
int, int64_t *, int64_t, int));
/**
* Set the time base and wrapping info for a given stream. This will be used
diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
index 979cb9a031..f28fb2297e 100644
--- a/libavformat/nutdec.c
+++ b/libavformat/nutdec.c
@@ -653,7 +653,7 @@ static int64_t find_duration(NUTContext *nut, int64_t
filesize)
AVFormatContext *s = nut->avf;
int64_t duration = 0;
-ff_find_last_ts(s, -1, , NULL, nut_read_timestamp);
+ff_find_last_ts(s, -1, , NULL, nut_read_timestamp, NULL);
if(duration > 0)
s->duration_estimation_method = AVFMT_DURATION_FROM_PTS;
@@ -1251,7 +1251,7 @@ static int read_seek(AVFormatContext *s, int stream_index,
pos = ff_gen_search(s, -1, dummy.ts, next_node[0]->pos,
next_node[1]->pos, next_node[1]->pos,
next_node[0]->ts, next_node[1]->ts,
-AVSEEK_FLAG_BACKWARD, , nut_read_timestamp);
+AVSEEK_FLAG_BACKWARD, , nut_read_timestamp,
NULL);
if (pos < 0)
return pos;
@@ -1263,7 +1263,7 @@ static int read_seek(AVFormatContext *s, int stream_index,
pos2 = ff_gen_search(s, -2, dummy.pos, next_node[0]->pos,
next_node[1]->pos, next_node[1]->pos,
next_node[0]->back_ptr,
next_node[1]->back_ptr,
- flags, ,
