[vlc-devel] [PATCH 5/9] demux: mkv: differentiate between internal and externally triggered seek

[Filip Roséen]

Hi Steve,

Let me know and I adjust this patch to the way you imply, I have some
opinions on the matter, but the most important thing is fixing
`GotoAndPlay` and related, so I am very flexible.

Also, thanks for the very detailed review!

On 2018-07-16 12:34, Steve Lhomme wrote:

> > +    if( !( i_flags & MKV_DEMUX_SEEK_TRIGGERED_EXTERNALLY ) )
> > +        i_flags |= MKV_DEMUX_SEEK_PRECISE;
> > +
> 
> Are we sure about that ? We could decide to use fast-seeking internally as well.

Under what circumstances would we want to enable fast-seeking
internally? I thought about it, but it could cause a number of issues
due to it being inexact.

Just consider `GotoAndPlay` as an example, if there are such commands
in the file we could potentially end up in a state where we keep
jumping back and forth, and playback would break completely. Same
thing could happen for the user, but that is upon request, I don't
think we want to end up in such case implicitly while following the
spec.

Though, I could remove the `if`, maybe there is a case I have not
thought about where it would actually make sense.

> > +    if( !( i_flags & MKV_DEMUX_SEEK_TRIGGERED_EXTERNALLY ) )
> > +        es_out_Control( sys.demuxer.out, ES_OUT_RESET_PCR );
> > +
> 
> Is this replacing the call from UpdateCurrentToChapter() ? Do we want to reset the PCR
> when switching to the next hard-linked segment ? (ie basically the exact continuation of
> the previous segment). It *is* an internal seek. Will it cause buffer flushing which is
> definitely not wanted here.

I compared the playback experience from current `master` and could not
notice any negative changes, tried several files, and from various
mediums (to test IO related things).

The one thing I did find was that if it takes a while to open the new
segment (without PCR reset), we would end up in a state where blocks
would be coming in late (from the core/decoder's perspective), and we
would have issues with either drifting or drops.

> >               p_current_vchapter = p_vchapter;
> > -            /* only use for soft linking, hard linking should be continous */
> 
> I think this is this explanation on why we only do the RESET_PCR in some case when we
> seek internally but not others.

I started working on making things unconditionally smoother, but those patches are
currently in super-draft mode. If you want me to patch the patch and
retain the old behavior for this specific case, I'd be happy to do
that - I want to fix the issue in a more robust way anyhow.

I also originally had that we would only reset the PCR if we are
seeking backwards, as forward would be handled but setting the next
display time, but I felt it was too hackish.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180716/4e4d08dc/attachment.html>