diff mbox series

[FFmpeg-devel,v2] avformat/hls Implement support for using AVSEEK_FLAG_BACKWARD when seeking

Message ID 20210605204226.536754-1-gustav.grusell@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avformat/hls Implement support for using AVSEEK_FLAG_BACKWARD when seeking
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

Gustav Grusell June 5, 2021, 8:42 p.m. UTC
Before, seeking in hls streams would always seek to the next keyframe after the given timestamp.
With this fix, if AVSEEK_FLAG_BACKWARD is set, seeking will be to the first keyframe of the
segment containing the given timestamp. This fixes #7485.

Signed-off-by: Gustav Grusell <gustav.grusell@gmail.com>
---
 libavformat/hls.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Gustav Grusell June 11, 2021, 6:33 a.m. UTC | #1
On Sat, Jun 5, 2021 at 10:41 PM Gustav Grusell <gustav.grusell@gmail.com>
wrote:

> Before, seeking in hls streams would always seek to the next keyframe
> after the given timestamp.
> With this fix, if AVSEEK_FLAG_BACKWARD is set, seeking will be to the
> first keyframe of the
> segment containing the given timestamp. This fixes #7485.
>
> Signed-off-by: Gustav Grusell <gustav.grusell@gmail.com>
> ---
>  libavformat/hls.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 8fc6924c90..fb8c9b071a 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -2197,6 +2197,15 @@ static int hls_read_packet(AVFormatContext *s,
> AVPacket *pkt)
>                          break;
>                      }
>
> +                    /* If AVSEEK_FLAG_BACKWARD set and not
> AVSEEK_FLAG_ANY,
> +                     * we return the first keyframe encountered */
> +                    if (pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> +                        !(pls->seek_flags & AVSEEK_FLAG_ANY) &&
> +                        pls->pkt->flags & AV_PKT_FLAG_KEY) {
> +                        pls->seek_timestamp = AV_NOPTS_VALUE;
> +                        break;
> +                    }
> +
>                      tb = get_timebase(pls);
>                      ts_diff = av_rescale_rnd(pls->pkt->dts, AV_TIME_BASE,
>                                              tb.den, AV_ROUND_DOWN) -
> --
> 2.25.1
>
>
Any further comments on this?

Btw, it seems the v2 patch got picked up as a separate patch by patchwork.
Is this how it is supposed to work or did I miss something when doing git
send-email ?

cheers,
Gustav
Lingjiang Fang June 11, 2021, 4:28 p.m. UTC | #2
On Sat,  5 Jun 2021 22:42:26 +0200
Gustav Grusell <gustav.grusell@gmail.com> wrote:

> Before, seeking in hls streams would always seek to the next keyframe
> after the given timestamp. With this fix, if AVSEEK_FLAG_BACKWARD is
> set, seeking will be to the first keyframe of the segment containing
> the given timestamp. This fixes #7485.
> 
> Signed-off-by: Gustav Grusell <gustav.grusell@gmail.com>
> ---
>  libavformat/hls.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 8fc6924c90..fb8c9b071a 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -2197,6 +2197,15 @@ static int hls_read_packet(AVFormatContext *s,
> AVPacket *pkt) break;
>                      }
>  
> +                    /* If AVSEEK_FLAG_BACKWARD set and not
> AVSEEK_FLAG_ANY,
> +                     * we return the first keyframe encountered */
> +                    if (pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> +                        !(pls->seek_flags & AVSEEK_FLAG_ANY) &&
> +                        pls->pkt->flags & AV_PKT_FLAG_KEY) {
taking account of the case of using flag backward and flag any together,
a logic like this is better:

if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
    (pls->seek_flags & AVSEEK_FLAG_ANY || 
     pls->pkt->flags & AV_PKT_FLAG_KEY)) {
   ....
}

> +                        pls->seek_timestamp = AV_NOPTS_VALUE;
> +                        break;
> +                    }
> +
>                      tb = get_timebase(pls);
>                      ts_diff = av_rescale_rnd(pls->pkt->dts,
> AV_TIME_BASE, tb.den, AV_ROUND_DOWN) -

BTW, when I used seek backward before, what I want is to get exactly
packets of the specified seek time, not only the specified stream
but also all streams I need. To achieve this, I will put this patch
lines before this if block, like this:

if (pls->seek_timestamp == AV_NOPTS_VALUE)
    break;

if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
    (pls->seek_flags & AVSEEK_FLAG_ANY || 
     pls->pkt->flags & AV_PKT_FLAG_KEY)) {
   ....
}

if (pls->seek_stream_index < 0 ||
...


Regards,
Lingjiang Fang
Gustav Grusell June 12, 2021, 12:16 p.m. UTC | #3
On Fri, Jun 11, 2021 at 6:29 PM Lingjiang Fang <vacingfang@foxmail.com>
wrote:

> On Sat,  5 Jun 2021 22:42:26 +0200
> Gustav Grusell <gustav.grusell@gmail.com> wrote:
>
> > Before, seeking in hls streams would always seek to the next keyframe
> > after the given timestamp. With this fix, if AVSEEK_FLAG_BACKWARD is
> > set, seeking will be to the first keyframe of the segment containing
> > the given timestamp. This fixes #7485.
> >
> > Signed-off-by: Gustav Grusell <gustav.grusell@gmail.com>
> > ---
> >  libavformat/hls.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > index 8fc6924c90..fb8c9b071a 100644
> > --- a/libavformat/hls.c
> > +++ b/libavformat/hls.c
> > @@ -2197,6 +2197,15 @@ static int hls_read_packet(AVFormatContext *s,
> > AVPacket *pkt) break;
> >                      }
> >
> > +                    /* If AVSEEK_FLAG_BACKWARD set and not
> > AVSEEK_FLAG_ANY,
> > +                     * we return the first keyframe encountered */
> > +                    if (pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> > +                        !(pls->seek_flags & AVSEEK_FLAG_ANY) &&
> > +                        pls->pkt->flags & AV_PKT_FLAG_KEY) {
> taking account of the case of using flag backward and flag any together,
> a logic like this is better:
>
> if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
>     (pls->seek_flags & AVSEEK_FLAG_ANY ||
>      pls->pkt->flags & AV_PKT_FLAG_KEY)) {
>    ....
> }
>
>
Thanks for your feedback!
I'm not sure about this, I think it makes more sense if seeking with
AVSEEK_FLAG_BACKWARD and AVSEEK_FLAG_ANY behaves the same as when using
only AVSEEK_FLAG_ANY, that is to seek to as close as possible as the exact
timestamp, regardless if that is a keyframe or not. If I'm not mistaken,
with your proposed change, using both AVSEEK_FLAG_BACKWARD and
AVSEEK_FLAG_ANY would always seek to the first frame of the segment which
is always a keyframe, so behaviour would be the same as using only
AVSEEK_FLAG_BACKWARD.


> > +                        pls->seek_timestamp = AV_NOPTS_VALUE;
> > +                        break;
> > +                    }
> > +
> >                      tb = get_timebase(pls);
> >                      ts_diff = av_rescale_rnd(pls->pkt->dts,
> > AV_TIME_BASE, tb.den, AV_ROUND_DOWN) -
>
> BTW, when I used seek backward before, what I want is to get exactly
> packets of the specified seek time, not only the specified stream
> but also all streams I need. To achieve this, I will put this patch
> lines before this if block, like this:
>
> if (pls->seek_timestamp == AV_NOPTS_VALUE)
>     break;
>
> if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
>     (pls->seek_flags & AVSEEK_FLAG_ANY ||
>      pls->pkt->flags & AV_PKT_FLAG_KEY)) {
>    ....
> }
>
> if (pls->seek_stream_index < 0 ||
> ...
>
>
Not sure I'm following. If I'm not mistaken, the hls spec does not require
audio and video segments to be aligned. Given this, it seems to me that the
above proposed change could lead to incorrect seek for some streams when
using AVSEEK_FLAG_BACKWARDS, since we would get the first frame of the
segments for all streams, and for audio streams this might not match the
timestamp of the first segment of the video stream.

Cheers,
Gustav Grusell
Lingjiang Fang June 12, 2021, 4:51 p.m. UTC | #4
On Sat, 12 Jun 2021 14:16:22 +0200
Gustav Grusell <gustav.grusell@gmail.com> wrote:

> On Fri, Jun 11, 2021 at 6:29 PM Lingjiang Fang
> <vacingfang@foxmail.com> wrote:
> 
> > On Sat,  5 Jun 2021 22:42:26 +0200
> > Gustav Grusell <gustav.grusell@gmail.com> wrote:
> >  
> > > Before, seeking in hls streams would always seek to the next
> > > keyframe after the given timestamp. With this fix, if
> > > AVSEEK_FLAG_BACKWARD is set, seeking will be to the first
> > > keyframe of the segment containing the given timestamp. This
> > > fixes #7485.
> > >
> > > Signed-off-by: Gustav Grusell <gustav.grusell@gmail.com>
> > > ---
> > >  libavformat/hls.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > > index 8fc6924c90..fb8c9b071a 100644
> > > --- a/libavformat/hls.c
> > > +++ b/libavformat/hls.c
> > > @@ -2197,6 +2197,15 @@ static int hls_read_packet(AVFormatContext
> > > *s, AVPacket *pkt) break;
> > >                      }
> > >
> > > +                    /* If AVSEEK_FLAG_BACKWARD set and not
> > > AVSEEK_FLAG_ANY,
> > > +                     * we return the first keyframe encountered
> > > */
> > > +                    if (pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> > > +                        !(pls->seek_flags & AVSEEK_FLAG_ANY) &&
> > > +                        pls->pkt->flags & AV_PKT_FLAG_KEY) {  
> > taking account of the case of using flag backward and flag any
> > together, a logic like this is better:
> >
> > if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> >     (pls->seek_flags & AVSEEK_FLAG_ANY ||
> >      pls->pkt->flags & AV_PKT_FLAG_KEY)) {
> >    ....
> > }
> >
> >  
> Thanks for your feedback!
> I'm not sure about this, I think it makes more sense if seeking with
> AVSEEK_FLAG_BACKWARD and AVSEEK_FLAG_ANY behaves the same as when
> using only AVSEEK_FLAG_ANY, that is to seek to as close as possible
> as the exact timestamp, regardless if that is a keyframe or not. If
what do you mean "AVSEEK_FLAG_BACKWARD and AVSEEK_FLAGAV_ANY behaves the
same as when using only AVSEEK_FLAG_ANY"?
I think the default behavior of seek is seek to key frame, where
AVSEEK_FLAGAV_ANY means we can accept seeking to non-key frame.


> I'm not mistaken, with your proposed change, using both
> AVSEEK_FLAG_BACKWARD and AVSEEK_FLAG_ANY would always seek to the
> first frame of the segment which is always a keyframe, so behaviour
> would be the same as using only AVSEEK_FLAG_BACKWARD.
when the first packet of a segment is not key-frame(rarely seen, but
is possible), the behavior will be different.
- AVSEEK_FLAG_BACKWARD will go on seeking until find a key frame
- AVSEEK_FLAG_BACKWARD + AVSEEK_FLAG_BACKWARD should return immediatly

there is a similar logic at the end of this if{} block.

> 
> 
> > > +                        pls->seek_timestamp = AV_NOPTS_VALUE;
> > > +                        break;
> > > +                    }
> > > +
> > >                      tb = get_timebase(pls);
> > >                      ts_diff = av_rescale_rnd(pls->pkt->dts,
> > > AV_TIME_BASE, tb.den, AV_ROUND_DOWN) -  
> >
> > BTW, when I used seek backward before, what I want is to get exactly
> > packets of the specified seek time, not only the specified stream
> > but also all streams I need. To achieve this, I will put this patch
> > lines before this if block, like this:
> >
> > if (pls->seek_timestamp == AV_NOPTS_VALUE)
> >     break;
> >
> > if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> >     (pls->seek_flags & AVSEEK_FLAG_ANY ||
> >      pls->pkt->flags & AV_PKT_FLAG_KEY)) {
> >    ....
> > }
> >
> > if (pls->seek_stream_index < 0 ||
> > ...
> >
> >  
> Not sure I'm following. If I'm not mistaken, the hls spec does not
> require audio and video segments to be aligned. Given this, it seems
> to me that the above proposed change could lead to incorrect seek for
> some streams when using AVSEEK_FLAG_BACKWARDS, since we would get the
> first frame of the segments for all streams, and for audio streams
> this might not match the timestamp of the first segment of the video
> stream.
it's hard to seek precisely when we want to seek backward, we just
guarantee we seek to a place before the given time, like what this
patch does.
the way I proposed is to follow the flag as much as possible, and behave
similarly no matter audio and video are in different playlists or in
one, because it only affects the stream in the same playlist.

anyway, it's just a suggestion, hoping more discussion :)

> 
> Cheers,
> Gustav Grusell
> _______________________________________________
> 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".



Regards,
Lingjiang Fang
Gustav Grusell June 12, 2021, 9:14 p.m. UTC | #5
On Sat, Jun 12, 2021 at 6:51 PM Lingjiang Fang <vacingfang@foxmail.com>
wrote:

> On Sat, 12 Jun 2021 14:16:22 +0200
> Gustav Grusell <gustav.grusell@gmail.com> wrote:
>
> > On Fri, Jun 11, 2021 at 6:29 PM Lingjiang Fang
> > <vacingfang@foxmail.com> wrote:
> >
> > > On Sat,  5 Jun 2021 22:42:26 +0200
> > > Gustav Grusell <gustav.grusell@gmail.com> wrote:
> > >
> > > > Before, seeking in hls streams would always seek to the next
> > > > keyframe after the given timestamp. With this fix, if
> > > > AVSEEK_FLAG_BACKWARD is set, seeking will be to the first
> > > > keyframe of the segment containing the given timestamp. This
> > > > fixes #7485.
> > > >
> > > > Signed-off-by: Gustav Grusell <gustav.grusell@gmail.com>
> > > > ---
> > > >  libavformat/hls.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > > > index 8fc6924c90..fb8c9b071a 100644
> > > > --- a/libavformat/hls.c
> > > > +++ b/libavformat/hls.c
> > > > @@ -2197,6 +2197,15 @@ static int hls_read_packet(AVFormatContext
> > > > *s, AVPacket *pkt) break;
> > > >                      }
> > > >
> > > > +                    /* If AVSEEK_FLAG_BACKWARD set and not
> > > > AVSEEK_FLAG_ANY,
> > > > +                     * we return the first keyframe encountered
> > > > */
> > > > +                    if (pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> > > > +                        !(pls->seek_flags & AVSEEK_FLAG_ANY) &&
> > > > +                        pls->pkt->flags & AV_PKT_FLAG_KEY) {
> > > taking account of the case of using flag backward and flag any
> > > together, a logic like this is better:
> > >
> > > if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> > >     (pls->seek_flags & AVSEEK_FLAG_ANY ||
> > >      pls->pkt->flags & AV_PKT_FLAG_KEY)) {
> > >    ....
> > > }
> > >
> > >
> > Thanks for your feedback!
> > I'm not sure about this, I think it makes more sense if seeking with
> > AVSEEK_FLAG_BACKWARD and AVSEEK_FLAG_ANY behaves the same as when
> > using only AVSEEK_FLAG_ANY, that is to seek to as close as possible
> > as the exact timestamp, regardless if that is a keyframe or not. If
> what do you mean "AVSEEK_FLAG_BACKWARD and AVSEEK_FLAGAV_ANY behaves the
> same as when using only AVSEEK_FLAG_ANY"?
> I think the default behavior of seek is seek to key frame, where
> AVSEEK_FLAGAV_ANY means we can accept seeking to non-key frame.
>
>
Sorry I think you can disregard what I wrote. I was thinking of the case
where the user would set both BACKWARDS and ANY flags, and
forgot that if the user sets the BACKWARDS flag, the ANY flag will be added
for other playlists than the one containing the stream
searched in. I think this made me miss your point.
So what I meant was that if seeking with both AVSEEK_FLAG_BACKWARD and
AVSEEK_FLAG_ANY, optimally, I would expect the result to
be to search to the nearest frame before or exactly at the seek timestamp.
Note that this is not how i behaves with my patch, it will
seek to the first frame with a timestamp equal to or greater to the seek
timestamp. In my opinion this is probably good enough, and is
in any case the same behaviour as before the patch.
So if the user has etAVSEEK_FLAG_ANY we would not then want to search to
the first frame of the segment, even if we have the AVSEEK_FLAG_BACKWARD
set. But of course this is different if AVSEEK_FLAG_ANY has been set
because this is not the seek playlist.


> > I'm not mistaken, with your proposed change, using both
> > AVSEEK_FLAG_BACKWARD and AVSEEK_FLAG_ANY would always seek to the
> > first frame of the segment which is always a keyframe, so behaviour
> > would be the same as using only AVSEEK_FLAG_BACKWARD.
> when the first packet of a segment is not key-frame(rarely seen, but
> is possible), the behavior will be different.
> - AVSEEK_FLAG_BACKWARD will go on seeking until find a key frame
> - AVSEEK_FLAG_BACKWARD + AVSEEK_FLAG_BACKWARD should return immediatly
>
>
Ah yes you are correct. I was thinking that the HLS specification requires
an IDR frame at the start of every (video) segment, but now that I looked
it up that is not strictly the case.
Apples HLS authorign specification does require it though.


there is a similar logic at the end of this if{} block.
>
> >
> >
> > > > +                        pls->seek_timestamp = AV_NOPTS_VALUE;
> > > > +                        break;
> > > > +                    }
> > > > +
> > > >                      tb = get_timebase(pls);
> > > >                      ts_diff = av_rescale_rnd(pls->pkt->dts,
> > > > AV_TIME_BASE, tb.den, AV_ROUND_DOWN) -
> > >
> > > BTW, when I used seek backward before, what I want is to get exactly
> > > packets of the specified seek time, not only the specified stream
> > > but also all streams I need. To achieve this, I will put this patch
> > > lines before this if block, like this:
> > >
> > > if (pls->seek_timestamp == AV_NOPTS_VALUE)
> > >     break;
> > >
> > > if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> > >     (pls->seek_flags & AVSEEK_FLAG_ANY ||
> > >      pls->pkt->flags & AV_PKT_FLAG_KEY)) {
> > >    ....
> > > }
> > >
> > > if (pls->seek_stream_index < 0 ||
> > > ...
> > >
> > >
> > Not sure I'm following. If I'm not mistaken, the hls spec does not
> > require audio and video segments to be aligned. Given this, it seems
> > to me that the above proposed change could lead to incorrect seek for
> > some streams when using AVSEEK_FLAG_BACKWARDS, since we would get the
> > first frame of the segments for all streams, and for audio streams
> > this might not match the timestamp of the first segment of the video
> > stream.
> it's hard to seek precisely when we want to seek backward, we just
> guarantee we seek to a place before the given time, like what this
> patch does.
> the way I proposed is to follow the flag as much as possible, and behave
> similarly no matter audio and video are in different playlists or in
> one, because it only affects the stream in the same playlist.
>

Ah, it took me a while, but now I just realized you have a very valid
point. If audio is in different playlist than video and we seek in video
stream, the audio playlist will seek
with AVSEEK_FLAG_ANY and with my patch may seek to a timestamp that comes
quite some time (a few seconds or so) after the timestamp we seeked to in
the video stream. This seems undesirable, I will rethink and rewrite the
patch.


>
> anyway, it's just a suggestion, hoping more discussion :)
>
>
Many thanks for your input, very helpful :) .

Cheers,
Gustav
Gustav Grusell June 18, 2021, 8:16 p.m. UTC | #6
On Sun, Jun 13, 2021 at 5:14 PM Lingjiang Fang <vacingfang@hotmail.com>
wrote:

> On Sat, 12 Jun 2021 23:14:16 +0200
> Gustav Grusell <gustav.grusell@gmail.com> wrote:
>
> > On Sat, Jun 12, 2021 at 6:51 PM Lingjiang Fang
> > <vacingfang@foxmail.com> wrote:
> >
> > > On Sat, 12 Jun 2021 14:16:22 +0200
> > > Gustav Grusell <gustav.grusell@gmail.com> wrote:
> > >
> > > > On Fri, Jun 11, 2021 at 6:29 PM Lingjiang Fang
> > > > <vacingfang@foxmail.com> wrote:
> > > >
> > > > > On Sat,  5 Jun 2021 22:42:26 +0200
> > > > > Gustav Grusell <gustav.grusell@gmail.com> wrote:
> > > > >
> > > > > > Before, seeking in hls streams would always seek to the next
> > > > > > keyframe after the given timestamp. With this fix, if
> > > > > > AVSEEK_FLAG_BACKWARD is set, seeking will be to the first
> > > > > > keyframe of the segment containing the given timestamp. This
> > > > > > fixes #7485.
> > > > > >
> > > > > > Signed-off-by: Gustav Grusell <gustav.grusell@gmail.com>
> > > > > > ---
> > > > > >  libavformat/hls.c | 9 +++++++++
> > > > > >  1 file changed, 9 insertions(+)
> > > > > >
> > > > > > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > > > > > index 8fc6924c90..fb8c9b071a 100644
> > > > > > --- a/libavformat/hls.c
> > > > > > +++ b/libavformat/hls.c
> > > > > > @@ -2197,6 +2197,15 @@ static int
> > > > > > hls_read_packet(AVFormatContext *s, AVPacket *pkt) break;
> > > > > >                      }
> > > > > >
> > > > > > +                    /* If AVSEEK_FLAG_BACKWARD set and not
> > > > > > AVSEEK_FLAG_ANY,
> > > > > > +                     * we return the first keyframe
> > > > > > encountered */
> > > > > > +                    if (pls->seek_flags &
> > > > > > AVSEEK_FLAG_BACKWARD &&
> > > > > > +                        !(pls->seek_flags & AVSEEK_FLAG_ANY)
> > > > > > &&
> > > > > > +                        pls->pkt->flags & AV_PKT_FLAG_KEY) {
> > > > > >
> > > > > taking account of the case of using flag backward and flag any
> > > > > together, a logic like this is better:
> > > > >
> > > > > if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> > > > >     (pls->seek_flags & AVSEEK_FLAG_ANY ||
> > > > >      pls->pkt->flags & AV_PKT_FLAG_KEY)) {
> > > > >    ....
> > > > > }
> > > > >
> > > > >
> > > > Thanks for your feedback!
> > > > I'm not sure about this, I think it makes more sense if seeking
> > > > with AVSEEK_FLAG_BACKWARD and AVSEEK_FLAG_ANY behaves the same as
> > > > when using only AVSEEK_FLAG_ANY, that is to seek to as close as
> > > > possible as the exact timestamp, regardless if that is a keyframe
> > > > or not. If
> > > what do you mean "AVSEEK_FLAG_BACKWARD and AVSEEK_FLAGAV_ANY
> > > behaves the same as when using only AVSEEK_FLAG_ANY"?
> > > I think the default behavior of seek is seek to key frame, where
> > > AVSEEK_FLAGAV_ANY means we can accept seeking to non-key frame.
> > >
> > >
> > Sorry I think you can disregard what I wrote. I was thinking of the
> > case where the user would set both BACKWARDS and ANY flags, and
> > forgot that if the user sets the BACKWARDS flag, the ANY flag will be
> > added for other playlists than the one containing the stream
> > searched in. I think this made me miss your point.
> > So what I meant was that if seeking with both AVSEEK_FLAG_BACKWARD and
> > AVSEEK_FLAG_ANY, optimally, I would expect the result to
> > be to search to the nearest frame before or exactly at the seek
> > timestamp. Note that this is not how i behaves with my patch, it will
> > seek to the first frame with a timestamp equal to or greater to the
> > seek timestamp. In my opinion this is probably good enough, and is
> > in any case the same behaviour as before the patch.
> > So if the user has etAVSEEK_FLAG_ANY we would not then want to search
> > to the first frame of the segment, even if we have the
> > AVSEEK_FLAG_BACKWARD set. But of course this is different if
> > AVSEEK_FLAG_ANY has been set because this is not the seek playlist.
> >
> >
> > > > I'm not mistaken, with your proposed change, using both
> > > > AVSEEK_FLAG_BACKWARD and AVSEEK_FLAG_ANY would always seek to the
> > > > first frame of the segment which is always a keyframe, so
> > > > behaviour would be the same as using only AVSEEK_FLAG_BACKWARD.
> > > when the first packet of a segment is not key-frame(rarely seen, but
> > > is possible), the behavior will be different.
> > > - AVSEEK_FLAG_BACKWARD will go on seeking until find a key frame
> > > - AVSEEK_FLAG_BACKWARD + AVSEEK_FLAG_BACKWARD should return
> > > immediatly
> > >
> > >
> > Ah yes you are correct. I was thinking that the HLS specification
> > requires an IDR frame at the start of every (video) segment, but now
> > that I looked it up that is not strictly the case.
> > Apples HLS authorign specification does require it though.
> >
> >
> > there is a similar logic at the end of this if{} block.
> > >
> > > >
> > > >
> > > > > > +                        pls->seek_timestamp = AV_NOPTS_VALUE;
> > > > > > +                        break;
> > > > > > +                    }
> > > > > > +
> > > > > >                      tb = get_timebase(pls);
> > > > > >                      ts_diff = av_rescale_rnd(pls->pkt->dts,
> > > > > > AV_TIME_BASE, tb.den, AV_ROUND_DOWN) -
> > > > >
> > > > > BTW, when I used seek backward before, what I want is to get
> > > > > exactly packets of the specified seek time, not only the
> > > > > specified stream but also all streams I need. To achieve this,
> > > > > I will put this patch lines before this if block, like this:
> > > > >
> > > > > if (pls->seek_timestamp == AV_NOPTS_VALUE)
> > > > >     break;
> > > > >
> > > > > if ( pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
> > > > >     (pls->seek_flags & AVSEEK_FLAG_ANY ||
> > > > >      pls->pkt->flags & AV_PKT_FLAG_KEY)) {
> > > > >    ....
> > > > > }
> > > > >
> > > > > if (pls->seek_stream_index < 0 ||
> > > > > ...
> > > > >
> > > > >
> > > > Not sure I'm following. If I'm not mistaken, the hls spec does not
> > > > require audio and video segments to be aligned. Given this, it
> > > > seems to me that the above proposed change could lead to
> > > > incorrect seek for some streams when using AVSEEK_FLAG_BACKWARDS,
> > > > since we would get the first frame of the segments for all
> > > > streams, and for audio streams this might not match the timestamp
> > > > of the first segment of the video stream.
> > > it's hard to seek precisely when we want to seek backward, we just
> > > guarantee we seek to a place before the given time, like what this
> > > patch does.
> > > the way I proposed is to follow the flag as much as possible, and
> > > behave similarly no matter audio and video are in different
> > > playlists or in one, because it only affects the stream in the same
> > > playlist.
> >
> > Ah, it took me a while, but now I just realized you have a very valid
> > point. If audio is in different playlist than video and we seek in
> > video stream, the audio playlist will seek
> > with AVSEEK_FLAG_ANY and with my patch may seek to a timestamp that
> > comes quite some time (a few seconds or so) after the timestamp we
> > seeked to in the video stream. This seems undesirable, I will rethink
> > and rewrite the patch.
> sorry, I didn't catch your point, can you give an example :)
>
>
Suppose you have a hls manifest with separate playlists for video and
audio. Video and audio are not aligned, suppose video segments have a
duration of 4s and audio segments have a duration of 3.97s. Then say you
want to seek to 163s from the start of the video. For the video, this would
be in the 41th segment, that starts at 160s. For audio, the seek point
would be in the 42nd segment, that starts at 162.77. With my patch for
seeking (with or without your amendments) both video and audio would seek
to the beginning of the segment containing the timestamp 163s from start.
This would mean that after seeking the video stream would be at 160s, and
the audio stream at 162.77, so we would miss some audio packages.


> BTW, summary of my view:
> if I have more data, I can drop it; but if I missed something, I can't
> make up it.
>
> Ah yes, that makes sense.

Cheers,
Gustav Grusell
diff mbox series

Patch

diff --git a/libavformat/hls.c b/libavformat/hls.c
index 8fc6924c90..fb8c9b071a 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -2197,6 +2197,15 @@  static int hls_read_packet(AVFormatContext *s, AVPacket *pkt)
                         break;
                     }
 
+                    /* If AVSEEK_FLAG_BACKWARD set and not AVSEEK_FLAG_ANY,
+                     * we return the first keyframe encountered */
+                    if (pls->seek_flags & AVSEEK_FLAG_BACKWARD &&
+                        !(pls->seek_flags & AVSEEK_FLAG_ANY) &&
+                        pls->pkt->flags & AV_PKT_FLAG_KEY) {
+                        pls->seek_timestamp = AV_NOPTS_VALUE;
+                        break;
+                    }
+
                     tb = get_timebase(pls);
                     ts_diff = av_rescale_rnd(pls->pkt->dts, AV_TIME_BASE,
                                             tb.den, AV_ROUND_DOWN) -