[vlc-devel] [PATCH] core: fix vlc_tick_now on darwin platform

[Steve Lhomme]

On 27/09/2018 08:25, Zhao Zhili wrote:
>
>
> On 2018年09月27日 14:01, Steve Lhomme wrote:
>> Not having a mac like most people I can only make changes blindly and 
>> hope it works. I did take some time on these changes to make sure I 
>> didn't overlook something, but it wasn't enough.
>
> I appreciate your work on the timestamps issue, and hope the evil 
> VLC_TICK_INVALID finally get fixed.
>
>>
>> On 27/09/2018 06:00, Zhao Zhili wrote:
>>> I didn't build on MacOS for a few days, so I didn't know from when 
>>> does the MacOS version was broken. Debugging on this issue really 
>>> takes some time including slow rebuild on old MacBook. So please do 
>>> some basic test before push. I can help on the test if you don't 
>>> have the environment.
>>>
>>> On 2018年09月27日 11:42, Zhao Zhili wrote:
>>>> ---
>>>>   src/darwin/thread.c | 10 +++-------
>>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/src/darwin/thread.c b/src/darwin/thread.c
>>>> index f5eb4f2..0dd1e3c 100644
>>>> --- a/src/darwin/thread.c
>>>> +++ b/src/darwin/thread.c
>>>> @@ -516,14 +516,10 @@ vlc_tick_t vlc_tick_now (void)
>>>>       vlc_clock_setup();
>>>>       uint64_t date = mach_absolute_time();
>>>>   -    /* denom is uint32_t, switch to 64 bits to prevent overflow. */
>>>> -    uint64_t denom = vlc_clock_conversion_factor.denom;
>>>> -
>>>> -    /* Switch to microsecs */
>>>> -    denom *= UINT64_C(1000);
>>>> -
>>>>       /* Split the division to prevent overflow */
>>>> -    return vlc_tick_from_frac( date * 
>>>> vlc_clock_conversion_factor.numer, denom );
>>>> +    lldiv_t d = lldiv (vlc_clock_conversion_factor.numer, 
>>>> vlc_clock_conversion_factor.denom);
>>>> +    date = (d.quot * date) + ((d.rem * date) / 
>>>> vlc_clock_conversion_factor.denom);
>>
>> Here the (date * d.rem) might overflow. The original 1000 
>> multiplication was lowering the d.rem value which was less bound to 
>> overflow.
>
> Before the patch:
>     d.rem <= denom - 1
>
> In the patch:
>     d.rem <= vlc_clock_conversion_factor.denom - 1
>
> And
>     denom = 1000 * vlc_clock_conversion_factor.denom
>
> So it doesn't make it worse.
>
> I don't think the 1000 multiplication has another purpose other than 
> /* Switch to microsecs */
>
>>
>> In practice it really depends on the common values of 
>> vlc_clock_conversion_factor.numer / 
>> vlc_clock_conversion_factor.denom. I suspect they are both small values.
>
> In my test case, numer and denom are both 1.

I'd be curious about the value on iPhones as well.

If the actual clock is in milliseconds that would put the 
vlc_clock_conversion_factor. to 1000000/1 to get from milliseconds to 
nanoseconds. But that means the date value will not grow very big so it 
won't overflow (and if it didn't dividing by 1 won't make a difference).

If they have more fun things like 1,000,000,000/1,001 then we have a 
chance of overflowing.

>
>>
>>>> +    return VLC_TICK_FROM_NS(date);
>>>>   }
>>>>     #undef vlc_tick_wait
>>>
>>>
>>>
>>> _______________________________________________
>>> 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