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

[Steve Lhomme]

On 2018-07-16 12:57, Filip Roséen wrote:
>
> 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.
>
For now we don't need it. Although precise seeking is not exactly the 
opposite of fast-seeking. In some case we may non precise seeking (like 
seek to the next Cluster, doesn't matter when it starts) but involving 
some lookup. I just want to make sure we don't block a possibility and 
modify the code with this assumption and later regret it.

> 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 forh, 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.
>

Yes, GotAndPlay is meant to be precise.

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

Did you try hard-linking ? Because that's the tricky case here. You can 
create such files with mkvmerge (split in multiple parts). There's not 
supposed to be any drop in the decoder during playback. But a RESET PCR 
will flush some buffers, even though the user didn't do anything and the 
playback is supposed to be as if it was just one file.

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

You can try, but I think it's tricky.

> 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.
>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel