[vlc-devel] [PATCH 5/9] demux: mkv: differentiate between internal and externally triggered seek
Filip Roséen
filip at atch.se
Mon Jul 16 14:35:30 CEST 2018
Hi again,
Attached is an updated patch without the unconditional precise-bit set
for internal seeks, as well as removal of the whitespace you mentioned
in `##videolan` @ `freenode`.
On 2018-07-16 13:51, Steve Lhomme wrote:
> 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
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180716/f6434a18/attachment.html>
-------------- next part --------------
>From b3ee3b1c66680d68b2be72d3a8fc4a66387213df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Filip=20Ros=C3=A9en?= <filip at atch.se>
Date: Sun, 15 Jul 2018 05:03:59 +0200
Subject: [PATCH] demux: mkv: differentiate between internal and externally
triggered seek
When the core requests the demux to seek it will, as is expected and
most often required, reset the PCR associated with the current input.
The core can of course not help the demux with this if it seeks by
itself (as can happen when changing segment, or respecting a
GotoAndPlay-matroska command).
These changes allows for such differantiation, and will effectively
reset the PCR where required.
fixes: #20447
---
modules/demux/mkv/matroska_segment.cpp | 11 ++++++++---
modules/demux/mkv/matroska_segment.hpp | 2 +-
modules/demux/mkv/mkv.cpp | 7 ++++++-
modules/demux/mkv/mkv.hpp | 6 ++++++
modules/demux/mkv/virtual_segment.cpp | 9 ++-------
modules/demux/mkv/virtual_segment.hpp | 2 +-
6 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/modules/demux/mkv/matroska_segment.cpp b/modules/demux/mkv/matroska_segment.cpp
index 2919130bab..53f0bd9cce 100644
--- a/modules/demux/mkv/matroska_segment.cpp
+++ b/modules/demux/mkv/matroska_segment.cpp
@@ -791,7 +791,7 @@ bool matroska_segment_c::LoadSeekHeadItem( const EbmlCallbacks & ClassInfos, int
return true;
}
-bool matroska_segment_c::Seek( demux_t &demuxer, vlc_tick_t i_absolute_mk_date, vlc_tick_t i_mk_time_offset, bool b_accurate )
+bool matroska_segment_c::Seek( demux_t &demuxer, vlc_tick_t i_absolute_mk_date, vlc_tick_t i_mk_time_offset, unsigned i_flags )
{
SegmentSeeker::tracks_seekpoint_t seekpoints;
@@ -861,7 +861,7 @@ bool matroska_segment_c::Seek( demux_t &demuxer, vlc_tick_t i_absolute_mk_date,
}
// blocks that will be not be read until this fpos
- if ( b_accurate )
+ if ( i_flags & MKV_DEMUX_SEEK_PRECISE )
trackit->second->i_skip_until_fpos = it->second.fpos;
else
trackit->second->i_skip_until_fpos = std::numeric_limits<uint64_t>::max();
@@ -876,9 +876,11 @@ bool matroska_segment_c::Seek( demux_t &demuxer, vlc_tick_t i_absolute_mk_date,
// propogate seek information //
+ vlc_tick_t i_orig_pts = sys.i_pts;
+
sys.i_pcr = VLC_TICK_INVALID;
sys.i_pts = VLC_TICK_0 + i_mk_seek_time + i_mk_time_offset;
- if (b_accurate)
+ if( i_flags & MKV_DEMUX_SEEK_PRECISE )
sys.i_start_pts = VLC_TICK_0 + i_absolute_mk_date;
else
sys.i_start_pts = sys.i_pts;
@@ -892,6 +894,9 @@ bool matroska_segment_c::Seek( demux_t &demuxer, vlc_tick_t i_absolute_mk_date,
msg_Dbg( &sys.demuxer, "seek: preroll{ req: %" PRId64 ", start-pts: %" PRId64 ", start-fpos: %" PRIu64 "} ",
sys.i_start_pts, sys.i_pts, i_seek_position );
+ if( !( i_flags & MKV_DEMUX_SEEK_TRIGGERED_EXTERNALLY ) )
+ es_out_Control( sys.demuxer.out, ES_OUT_RESET_PCR );
+
// blocks that will be read and decoded but discarded until this pts
es_out_Control( sys.demuxer.out, ES_OUT_SET_NEXT_DISPLAY_TIME, sys.i_start_pts );
return true;
diff --git a/modules/demux/mkv/matroska_segment.hpp b/modules/demux/mkv/matroska_segment.hpp
index 20cfd594f3..9620ef3999 100644
--- a/modules/demux/mkv/matroska_segment.hpp
+++ b/modules/demux/mkv/matroska_segment.hpp
@@ -144,7 +144,7 @@ public:
bool PreloadClusters( uint64 i_cluster_position );
void InformationCreate();
- bool Seek( demux_t &, vlc_tick_t i_mk_date, vlc_tick_t i_mk_time_offset, bool b_accurate );
+ bool Seek( demux_t &, vlc_tick_t i_mk_date, vlc_tick_t i_mk_time_offset, unsigned i_seek_flags );
int BlockGet( KaxBlock * &, KaxSimpleBlock * &, bool *, bool *, int64_t *);
diff --git a/modules/demux/mkv/mkv.cpp b/modules/demux/mkv/mkv.cpp
index 996617b141..c6c43d4d4e 100644
--- a/modules/demux/mkv/mkv.cpp
+++ b/modules/demux/mkv/mkv.cpp
@@ -506,7 +506,12 @@ static int Seek( demux_t *p_demux, vlc_tick_t i_mk_date, double f_percent, virtu
{
i_mk_date = int64_t( f_percent * p_sys->f_duration * 1000.0 );
}
- return p_vsegment->Seek( *p_demux, i_mk_date, p_vchapter, b_precise ) ? VLC_SUCCESS : VLC_EGENERIC;
+
+ int i_seek_flags = MKV_DEMUX_SEEK_TRIGGERED_EXTERNALLY;
+ if( b_precise )
+ i_seek_flags |= MKV_DEMUX_SEEK_PRECISE;
+
+ return p_vsegment->Seek( *p_demux, i_mk_date, p_vchapter, i_seek_flags ) ? VLC_SUCCESS : VLC_EGENERIC;
}
/* Needed by matroska_segment::Seek() and Seek */
diff --git a/modules/demux/mkv/mkv.hpp b/modules/demux/mkv/mkv.hpp
index 17a3fd0e67..ae5fe6a323 100644
--- a/modules/demux/mkv/mkv.hpp
+++ b/modules/demux/mkv/mkv.hpp
@@ -111,6 +111,12 @@ enum
MATROSKA_ENCODING_SCOPE_NEXT = 4 /* unsupported */
};
+enum
+{
+ MKV_DEMUX_SEEK_PRECISE = 0x1,
+ MKV_DEMUX_SEEK_TRIGGERED_EXTERNALLY = 0x2,
+};
+
#define MKVD_TIMECODESCALE 1000000
#define MKV_IS_ID( el, C ) ( el != NULL && typeid( *el ) == typeid( C ) )
diff --git a/modules/demux/mkv/virtual_segment.cpp b/modules/demux/mkv/virtual_segment.cpp
index 47c0d50d08..4e867a5aa5 100644
--- a/modules/demux/mkv/virtual_segment.cpp
+++ b/modules/demux/mkv/virtual_segment.cpp
@@ -458,8 +458,6 @@ bool virtual_segment_c::UpdateCurrentToChapter( demux_t & demux )
( p_current_vchapter && &p_current_vchapter->segment != &p_cur_vchapter->segment ) ||
( p_current_vchapter->p_chapter->i_end_time != p_cur_vchapter->p_chapter->i_start_time ))
{
- /* Forcing reset pcr */
- es_out_Control( demux.out, ES_OUT_RESET_PCR);
Seek( demux, p_cur_vchapter->i_mk_virtual_start_time, p_cur_vchapter );
return true;
}
@@ -513,7 +511,7 @@ bool virtual_chapter_c::EnterAndLeave( virtual_chapter_c *p_leaving_vchapter, bo
}
bool virtual_segment_c::Seek( demux_t & demuxer, vlc_tick_t i_mk_date,
- virtual_chapter_c *p_vchapter, bool b_precise )
+ virtual_chapter_c *p_vchapter, unsigned i_flags )
{
demux_sys_t *p_sys = (demux_sys_t *)demuxer.p_sys;
@@ -545,9 +543,6 @@ bool virtual_segment_c::Seek( demux_t & demuxer, vlc_tick_t i_mk_date,
msg_Dbg( &demuxer, "SWITCH CHAPTER uid=%" PRId64, p_vchapter->p_chapter ? p_vchapter->p_chapter->i_uid : 0 );
p_current_vchapter = p_vchapter;
- /* only use for soft linking, hard linking should be continous */
- es_out_Control( demuxer.out, ES_OUT_RESET_PCR );
-
p_sys->PreparePlayback( *this, i_mk_date );
return true;
}
@@ -555,7 +550,7 @@ bool virtual_segment_c::Seek( demux_t & demuxer, vlc_tick_t i_mk_date,
{
p_current_vchapter = p_vchapter;
- return p_current_vchapter->segment.Seek( demuxer, i_mk_date, i_mk_time_offset, b_precise );
+ return p_current_vchapter->segment.Seek( demuxer, i_mk_date, i_mk_time_offset, i_flags );
}
}
return false;
diff --git a/modules/demux/mkv/virtual_segment.hpp b/modules/demux/mkv/virtual_segment.hpp
index d09913b689..44d9d577f6 100644
--- a/modules/demux/mkv/virtual_segment.hpp
+++ b/modules/demux/mkv/virtual_segment.hpp
@@ -162,7 +162,7 @@ public:
virtual_chapter_c * FindChapter( int64_t i_find_uid );
bool UpdateCurrentToChapter( demux_t & demux );
- bool Seek( demux_t & demuxer, vlc_tick_t i_mk_date, virtual_chapter_c *p_vchapter, bool b_precise = true );
+ bool Seek( demux_t & demuxer, vlc_tick_t i_mk_date, virtual_chapter_c *p_vchapter, unsigned i_flags = MKV_DEMUX_SEEK_PRECISE );
private:
void KeepTrackSelection( matroska_segment_c & old, matroska_segment_c & next );
};
--
2.18.0
More information about the vlc-devel
mailing list