[Spice-devel] [spice-gtk 1/2] Fix possible multimedia time overflow

[Frediano Ziglio]

> 
> Hi
> 
> ----- Original Message -----
> > > > > > 
> > > > > > 3 - 4294967295 == 4 is not a portable assumption. Neither is
> > > > > > (uint32_t) 3 - (uint32_t) 4294967295 == 4
> > > > > 
> > > > > Ok, I am not an expert in portability, do you have an example where
> > > > > this
> > > > > would give different results?
> > > > > 
> > > > 
> > > > If int is 64 bit (uint32_t) 3 - (uint32_t) 4294967295 == -4294967292.
> > > > It seems weird but is due to the integral promotion rules.
> > > 
> > > 
> > > According to C standard, there is no integer promotion when both operands
> > > are
> > > of the same type:
> > > http://c0x.coding-guidelines.com/6.3.1.8.html:
> > >  712 If both operands have the same type, then no further conversion is
> > >  needed.
> > > 
> > 
> > You are skipping 710 and 711.
> 
> right, but then all platforms we care about use 32bit int afaik, so no
> promotion occurs. I think we could add a strict check in configure, that
> would make it clear. Then we don't have to half the max delay. But couldn't
> we find a more solid solution to handle time warp, by remembering the last
> time?
> 

Yes, currently all platforms we support have int of 32 bit and ILP64/SILP64
platforms are quite rare however being client is more likely to require
a port to new systems in the future.
Usually integers are not supposed to overflow (if an error does not happen)
so it's not a problem to port to such platforms unless, like in this case,
you have overflows. I don't see much issue to maintain the patch I proposed,
is enough documented (and if not comments can be added) and not so big.
Creating a monotonic multimedia time can be a bit challenging as the time
can drift a bit so potentially you could have sequences like 1, 5, 2, 8, 11.
The time depends on a server monotonic timer and a client monotonic one,
if you have large drifts (days) you are having some severe system bugs in
either the server or the client. In case of migration the reference get
updated so it's not an issue (the frame timers get dropped during VM migration),
in case of suspend/resume the client get disconnected and the timers on
the server frames get dropped.
I actually was thinking you were joking about the 42/24 days... why would
we need such constraint to require such changes?

> > 
> > > 
> > > > Similar to
> > > >    printf("%d\n", (uint16_t) 10 - (uint16_t) 16);
> > > > which give -6 (but maybe some people could point out that this result
> > > > is
> > > > not portable either... from my knowledge it is).
> > > > 
> > 
> > This would be a gcc bug then, right ?
> > I personally tested with different compilers too. Unless they all are
> > not following C rules integer promotion happens.
> > 
> > > > > > 
> > > > > > if time1 == UINT32_MAX and time2 == 3 I expect 4 while if
> > > > > > time2 == UINT32_MAX and time2 == 3 (this can happen for different
> > > > > > reasons)
> > > > > > I expect -4. This make computation easier.
> > > > > > 
> > > > > > I'll add this example.
> > > > > 
> > > > > Yeah, some test cases would be useful to understand the problem and
> > > > > the
> > > > > solution.
> > > > > 
> > > > 
> > > > Done
> > > > 
> > > > > Thanks
> > > > > 
> > > > > > 
> > > > > > > ts before the last frame received to check if we overlapped, I
> > > > > > > think
> > > > > > > (otherwise we should consider this frame as a late frame)
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > 
> > 

Frediano