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

[Lingjiang Fang]

On Sat, 12 Jun 2021 23:14:16 +0200
Gustav Grusell <gustav.grusell at gmail.com> wrote:

> On Sat, Jun 12, 2021 at 6:51 PM Lingjiang Fang
> <vacingfang at foxmail.com> wrote:
> 
> > On Sat, 12 Jun 2021 14:16:22 +0200
> > Gustav Grusell <gustav.grusell at gmail.com> wrote:
> >  
> > > On Fri, Jun 11, 2021 at 6:29 PM Lingjiang Fang
> > > <vacingfang at foxmail.com> wrote:
> > >  
> > > > On Sat,  5 Jun 2021 22:42:26 +0200
> > > > Gustav Grusell <gustav.grusell at 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 at 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 :)

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.

> 
> 
> >
> > anyway, it's just a suggestion, hoping more discussion :)
> >
> >  
> Many thanks for your input, very helpful :) .
> 
> Cheers,
> Gustav
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".



Regards,
Lingjiang Fang