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

[Zhao Zhili]

On 2018年09月27日 14:36, Steve Lhomme wrote:
> 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.

Apple doc gives scary example:

"For example, if the numerator returned is 1,000,000,000 and the 
denominator is 6,000,000, the frequency is 6,000,000ths of a second. 
Every unit of absolute time would be 166.67 nanoseconds and host time 
frequency in ticks per seconds may be calculated like this, |1.0e9 * 
(denom / numer)"

|https://developer.apple.com/library/archive/qa/qa1643/_index.html

Good luck and hope they didn't really do that.

>
>>
>>>
>>>>> +    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
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel