[vlc-devel] [PATCH 5/9] demux: mkv: differentiate between internal and externally triggered seek
Steve Lhomme
robux4 at ycbcr.xyz
Mon Jul 16 14:57:19 CEST 2018
On 2018-07-16 14:35, Filip Roséen wrote:
>
> 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|
>
>
> 0001-demux-mkv-differentiate-between-internal-and-externa.patch
>
>
> 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;
> +
Did this come from another patch ? This new value is not used.
> 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 );
> +
This should not be done with hard-linked segments. This is the
equivalent of a file split in multiple parts. The timestamps in files
continue the ones from the previous Segment as if it was exactly the
same Segment. A reset PCR will flush the buffers/decoders resulting in
frames lost when transitioning from one Segment to the next.
> // 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);
See above why we don't want to reset the PCR unconditionally. We may
have a logic and define when we don't want a reset PCR and do it
otherwise on seeking. But there are internal cases where we don't want it.
> 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
>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list