[vlc-devel] [PATCH 4/9] demux: mkv: fix DTS of outgoing blocks
Steve Lhomme
robux4 at ycbcr.xyz
Mon Jul 16 13:39:09 CEST 2018
On 2018-07-16 13:04, Filip Roséen wrote:
>
> Hi Steve,
>
> On 2018-07-16 12:12, Steve Lhomme wrote:
>
> |On 2018-07-16 5:19, Filip Roséen wrote:|
>
> |The previous implementation would set the VLC block's DTS to
> VLC_TICK_INVALID for matroska-frames within a matroska block,
> which in|
>
> |I don't see that in the code. I only see the DTS set to Invalid
> for Cook/Atrac3. The rest of the time it's pts or last_dts +
> default_duration.|
>
> * https://git.videolan.org/?p=vlc.git;a=blob;f=modules/demux/mkv/mkv.cpp;h=c259ea630fb1c9ef256c1443f5e538d14cf08970;hb=HEAD#l666
>
> It is funny that it is on line |666|, in either case; |i_pts| is most
> often set to |VLC_TICK_INVALID| after the first frame, effectively
> making every other frame have |VLC_TICK_INVALID| as |i_dts|.
>
Matroska doesn't store the DTS so it would be normal to set it to
INVALID (technically it should be UNKNOWN but that's another subject)
when we don't know. And we don't know when it's a frame with references
as it may not be in display order (which normally doesn't happen for
audio). Also frames in a lace are a bit tricky, but that's not allowed
for video.
The PTS on the other hand is stored for each frame so it should never be
INVALID (except again when lacing is involved).
So when I read i_dts = i_pts I assume it's an actual value and not INVALID.
With this patch we get the audio/subtitle i_dts not set (so likely
remains INVALID). I don't think we need this change since we're pretty
sure of the DTS at least for the first frame of the lace (very likely
used when it's audio).
> There are exceptions, such as if we have /default-duration/, or if the
> blocks are not /packetized/; maybe I should have mentioned that in the
> commit message now when you pointed it out - but I really consider
> those to be exceptions and not the main case.
>
> Do you want me to rewrite the commit message?
>
> |turn would confuse the decoder as it would then have to assume
> that the block in question belonged to the most recently set
> PCR. This works fine in cases where the PCR is updated often
> enough to almost match the track's frames, but in cases where
> another track is preventing the PCR from updating frequently
> enough (video track with long blocks of many matroska-frames),
> the PCR would be too far in the past to base the outgoing
> block's DTS on. As a result, the decoder would think the
> outgoing blocks were coming in too early and employ counter
> meassures to prevent unwanted drift. By setting the DTS to the
> matroska-block's timestamp, we prevent the above from
> happening, resulting in smoother playback. Included in this
> patch is a rename of the former i_pts parameter to
> BlockDecode: "i_timecode" should make it more clear that we
> are dealing with the block's timestamp, which might or might
> not be the same as the outgoing frames' pts. fixes: #13757 ---
> modules/demux/mkv/mkv.cpp | 13 +++++++++----
> modules/demux/mkv/mkv.hpp | 2 +- 2 files changed, 10
> insertions(+), 5 deletions(-) diff --git
> a/modules/demux/mkv/mkv.cpp b/modules/demux/mkv/mkv.cpp index
> 220a061e57..23c1b39776 100644 --- a/modules/demux/mkv/mkv.cpp
> +++ b/modules/demux/mkv/mkv.cpp @@ -511,7 +511,7 @@ static int
> Seek( demux_t *p_demux, vlc_tick_t i_mk_date, double
> f_percent, virtu /* Needed by matroska_segment::Seek() and
> Seek */ void BlockDecode( demux_t *p_demux, KaxBlock *block,
> KaxSimpleBlock *simpleblock, - vlc_tick_t i_pts, vlc_tick_t
> i_duration, bool b_key_picture, + vlc_tick_t i_timecode,
> vlc_tick_t i_duration, bool b_key_picture, bool
> b_discardable_picture ) { demux_sys_t *p_sys = (demux_sys_t
> *)p_demux->p_sys; @@ -538,8 +538,6 @@ void BlockDecode(
> demux_t *p_demux, KaxBlock *block, KaxSimpleBlock *simpleblock
> return; } - i_pts -= track.i_codec_delay; - if (
> track.fmt.i_cat != DATA_ES ) { bool b; @@ -557,6 +555,10 @@
> void BlockDecode( demux_t *p_demux, KaxBlock *block,
> KaxSimpleBlock *simpleblock size_t block_size =
> internal_block.GetSize(); const unsigned i_number_frames =
> internal_block.NumberFrames(); + vlc_tick_t i_track_timecode =
> i_timecode - track.i_codec_delay; + vlc_tick_t i_dts =
> i_track_timecode; + vlc_tick_t i_pts = i_track_timecode; +
> for( unsigned int i_frame = 0; i_frame < i_number_frames;
> i_frame++ ) { block_t *p_block; @@ -639,6 +641,9 @@ void
> BlockDecode( demux_t *p_demux, KaxBlock *block, KaxSimpleBlock
> *simpleblock break; } + p_block->i_dts = i_dts; +
> p_block->i_pts = i_pts; + if( track.fmt.i_cat != VIDEO_ES ) {
> if ( track.fmt.i_cat == DATA_ES ) @@ -648,7 +653,7 @@ void
> BlockDecode( demux_t *p_demux, KaxBlock *block, KaxSimpleBlock
> *simpleblock block_Release( p_block ); return; } -
> p_block->i_dts = p_block->i_pts = i_pts; + p_block->i_pts =
> i_pts; } else { diff --git a/modules/demux/mkv/mkv.hpp
> b/modules/demux/mkv/mkv.hpp index 3e0ae211b9..17a3fd0e67
> 100644 --- a/modules/demux/mkv/mkv.hpp +++
> b/modules/demux/mkv/mkv.hpp @@ -120,7 +120,7 @@ enum using
> namespace LIBMATROSKA_NAMESPACE; void BlockDecode( demux_t
> *p_demux, KaxBlock *block, KaxSimpleBlock *simpleblock, -
> vlc_tick_t i_pts, vlc_tick_t i_duration, bool b_key_picture, +
> vlc_tick_t i_timecode, vlc_tick_t i_duration, bool
> b_key_picture, bool b_discardable_picture ); class
> attachment_c -- 2.18.0
> _______________________________________________ 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|
>
>
>
> _______________________________________________
> 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