[vlc-devel] [PATCH 2/4] demux: mkv: truncate length on preskip, don't set negative pts

[Steve Lhomme]

On 2020-10-01 8:10, Steve Lhomme wrote:
> On 2020-09-30 17:44, Francois Cartegnie wrote:
>> Same as other demuxers do with preskip
>>
>> refs #25129
>> ---
>>   modules/demux/mkv/mkv.cpp | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/modules/demux/mkv/mkv.cpp b/modules/demux/mkv/mkv.cpp
>> index 82f464a74a..2af7d5c87b 100644
>> --- a/modules/demux/mkv/mkv.cpp
>> +++ b/modules/demux/mkv/mkv.cpp
>> @@ -547,8 +547,6 @@ void BlockDecode( demux_t *p_demux, KaxBlock 
>> *block, KaxSimpleBlock *simpleblock
>>           return;
>>       }
>> -    i_pts -= track.i_codec_delay;
> 
> The PTS delay should be applied no matter what.

"CodecDelay is The codec-built-in delay in nanoseconds. This value MUST 
be subtracted from each block timestamp in order to get the actual 
timestamp."

>> -
>>       if ( track.fmt.i_cat != DATA_ES )
>>       {
>>           bool b;
>> @@ -695,6 +693,24 @@ void BlockDecode( demux_t *p_demux, KaxBlock 
>> *block, KaxSimpleBlock *simpleblock
>>           }
>>           else
>>           {
>> +            if( track.i_codec_delay ) /* Handle pre-skip */
>> +            {
>> +                if( i_pts > track.i_codec_delay )
>> +                {
>> +                    p_block->i_length -= track.i_codec_delay;
> 
> That looks suspicious. The delay should influence the PTS/DTS, not the 
> duration of a block. If you wanted to skip a part at the beginning of a 
> block you would also need to specify the offset to skip.
> 
>> +                }
>> +                else
>> +                {
>> +                    vlc_tick_t skipped = i_pts - track.i_codec_delay;
> 
> Here i_pts <= track.i_codec_delay. So this value is negative.
> 
>> +                    if( p_block->i_length <= skipped )
> 
> So this is always false.

OK it's not negative if i_pts == track.i_codec_delay. I think in this 
case we should not consider it as pre-roll. And CodecDelay is *not* a 
preroll anyway.

> 
>> +                    {
>> +                        p_block->i_length = 0;
>> +                        p_block->i_flags |= BLOCK_FLAG_PREROLL;
>> +                    }
>> +                    else p_block->i_length -= skipped;
> 
> The block length is always expanded.
> 
>> +                }
>> +            }
>> +
>>               // correct timestamping when B frames are used
>>               if( track.b_dts_only )
>>               {
> 
> Also I did not receive patches [3/4] and [4/4].
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel