[vlc-devel] [PATCH 4/9] demux: mkv: fix DTS of outgoing blocks

[Filip Roséen]

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

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180716/32875ace/attachment.html>