[vlc-devel] [PATCH] demux: avformat: use native rescaling

[Francois Cartegnie]

Le 01/12/2020 à 12:09, Steve Lhomme a écrit :
> On 2020-11-30 11:25, Francois Cartegnie wrote:
>> Comparison & arithmetic through different time bases
>> are bogus (start_offset timebase vs streams pts timebase) when
>> rescaled using our own helpers.
>> We need to use the same rescaling functions to avoid different
>> roundings, then errors.
>> refs #25117
>> ---
>>   modules/codec/avcodec/avcommon.h |  5 +++++
>>   modules/demux/avformat/demux.c   | 33 ++++++++++----------------------
>>   2 files changed, 15 insertions(+), 23 deletions(-)
>>
>> diff --git a/modules/codec/avcodec/avcommon.h 
>> b/modules/codec/avcodec/avcommon.h
>> index acd4790651..0efc6dfc7c 100644
>> --- a/modules/codec/avcodec/avcommon.h
>> +++ b/modules/codec/avcodec/avcommon.h
>> @@ -42,9 +42,14 @@
>>   # include <libavutil/cpu.h>
>>   # include <libavutil/log.h>
>> +#define VLC_TIME_BASE_Q     (AVRational){1, CLOCK_FREQ}
>> +
>>   #if (CLOCK_FREQ == AV_TIME_BASE)
>>   #define FROM_AV_TS(x)  (x)
>>   #define TO_AV_TS(x)    (x)
>> +#elif defined(USE_AV_RESCALEQ) /* until we migrate all conversions */
>> +#define FROM_AV_TS(x)  av_rescale_q(x, AV_TIME_BASE_Q, VLC_TIME_BASE_Q)
>> +#define TO_AV_TS(x)    av_rescale_q(x, VLC_TIME_BASE_Q, AV_TIME_BASE_Q)
>>   #elif (CLOCK_FREQ % AV_TIME_BASE) == 0
>>   #define FROM_AV_TS(x)  ((x) * (CLOCK_FREQ / AV_TIME_BASE))
>>   #define TO_AV_TS(x)    ((x) / (CLOCK_FREQ / AV_TIME_BASE))
>> diff --git a/modules/demux/avformat/demux.c 
>> b/modules/demux/avformat/demux.c
>> index 91c4e005f4..834f672931 100644
>> --- a/modules/demux/avformat/demux.c
>> +++ b/modules/demux/avformat/demux.c
>> @@ -37,6 +37,8 @@
>>   #include <vlc_charset.h>
>>   #include <vlc_avcodec.h>
>> +#define USE_AV_RESCALEQ
>> +
>>   #include "../../codec/avcodec/avcodec.h"
>>   #include "../../codec/avcodec/chroma.h"
>>   #include "../../codec/avcodec/avcommon_compat.h"
>> @@ -748,10 +750,9 @@ int avformat_OpenDemux( vlc_object_t *p_this )
>>               EnsureUTF8( s->psz_name );
>>               msg_Dbg( p_demux, "    - chapter %d: %s", i, s->psz_name );
>>           }
>> -        s->i_time_offset = vlc_tick_from_samples( 
>> p_sys->ic->chapters[i]->start *
>> -            p_sys->ic->chapters[i]->time_base.num,
>> -            p_sys->ic->chapters[i]->time_base.den ) -
>> -            (i_start_time != VLC_TICK_INVALID ? i_start_time : 0 );
>> +        s->i_time_offset = av_rescale_q( p_sys->ic->chapters[i]->start,
>> +                                         
>> p_sys->ic->chapters[i]->time_base, VLC_TIME_BASE_Q )
> 
> I think you should do a macro/inline function that explicitely outputs a 
> vlc_tick_t using av_rescale_q(x,y,VLC_TIME_BASE_Q)
> 
>> +                 - (i_start_time != VLC_TICK_INVALID ? i_start_time : 
>> 0 );
>>           TAB_APPEND( p_sys->p_title->i_seekpoint, 
>> p_sys->p_title->seekpoint, s );
>>       }
>> @@ -859,7 +860,7 @@ static int Demux( demux_t *p_demux )
>>       /* Used to avoid timestamps overlow */
>>       if( p_sys->ic->start_time != (int64_t)AV_NOPTS_VALUE )
>>       {
>> -        i_start_time = vlc_tick_from_frac(p_sys->ic->start_time, 
>> AV_TIME_BASE);
>> +        i_start_time = FROM_AV_TS(p_sys->ic->start_time);
>>       }
>>       else
>>           i_start_time = 0;
>> @@ -868,7 +869,7 @@ static int Demux( demux_t *p_demux )
>>           p_frame->i_dts = VLC_TICK_INVALID;
>>       else
>>       {
>> -        p_frame->i_dts = vlc_tick_from_frac( pkt.dts * 
>> p_stream->time_base.num, p_stream->time_base.den )
>> +        p_frame->i_dts = av_rescale_q( pkt.dts, p_stream->time_base, 
>> VLC_TIME_BASE_Q )
>>                   - i_start_time + VLC_TICK_0;
>>       }
>> @@ -876,13 +877,11 @@ static int Demux( demux_t *p_demux )
>>           p_frame->i_pts = VLC_TICK_INVALID;
>>       else
>>       {
>> -        p_frame->i_pts = vlc_tick_from_frac( pkt.pts * 
>> p_stream->time_base.num, p_stream->time_base.den )
>> +        p_frame->i_pts = av_rescale_q( pkt.pts, p_stream->time_base, 
>> VLC_TIME_BASE_Q )
>>                   - i_start_time + VLC_TICK_0;
>>       }
>>       if( pkt.duration > 0 && p_frame->i_length <= 0 )
>> -        p_frame->i_length = vlc_tick_from_samples(pkt.duration *
>> -            p_stream->time_base.num,
>> -            p_stream->time_base.den );
>> +        p_frame->i_length = av_rescale_q( pkt.duration, 
>> p_stream->time_base, VLC_TIME_BASE_Q );
>>       /* Add here notoriously bugged file formats/samples */
>>       if( !strcmp( p_sys->fmt->name, "flv" ) )
>> @@ -970,21 +969,9 @@ static void ResetTime( demux_t *p_demux, int64_t 
>> i_time )
>>       vlc_tick_t t;
>>       if( p_sys->ic->start_time == (int64_t)AV_NOPTS_VALUE || i_time < 
>> 0 )
>> -    {
>>           t = VLC_TICK_INVALID;
>> -    }
>>       else
>> -    {
>> -#if CLOCK_FREQ == AV_TIME_BASE
>> -        t = FROM_AV_TS(i_time);
>> -#else
>> -        lldiv_t q = lldiv( i_time, AV_TIME_BASE );
>> -        t = vlc_tick_from_sec(q.quot) + FROM_AV_TS(q.rem);
>> -#endif
>> -
>> -        if( t == VLC_TICK_INVALID )
>> -            t = VLC_TICK_0;
>> -    }
>> +        t = VLC_TICK_0 + TO_AV_TS( i_time );
> 
> The original value was not shifted by VLC_TICK_0, is this change on 
> purpose ?

To me that's incorrect.

ResetTime is only called with 0, -1, or a - b, so it never contains TS_0 
offset, and needs to be applied.
Otherwise the lldiv( i_time, AV_TIME_BASE ) would also be wrong, 
dividing the TS_0

-- 
Francois Cartegnie
VideoLAN - VLC Developer