<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">
code{white-space: pre-wrap;}
span.smallcaps{font-variant: small-caps;}
span.underline{text-decoration: underline;}
div.column{display: inline-block; vertical-align: top; width: 50%;}
</style>
</head>
<body>
<p>Hi again,</p>
<p>Attached is an updated patch without the unconditional precise-bit set for internal seeks, as well as removal of the whitespace you mentioned in <code>##videolan</code> @ <code>freenode</code>.</p>
<p>On 2018-07-16 13:51, Steve Lhomme wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> On 2018-07-16 12:57, Filip Roséen wrote:</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
<p>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.</p>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
</blockquote>
<pre><code> Yes, GotAndPlay is meant to be precise.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
</blockquote>
<pre><code> 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.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> |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.</code></pre>
</blockquote>
<pre><code> You can try, but I think it's tricky.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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</code></pre>
</blockquote>
<pre><code> _______________________________________________
vlc-devel mailing list
To unsubscribe or modify your subscription options:
https://mailman.videolan.org/listinfo/vlc-devel</code></pre>
</blockquote>
</body>
</html>