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

[Zhao Zhili]

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.

>
>>> +    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