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

[Marc-André Lureau]

Hi

----- Original Message -----
> > 
> > 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?

This fix is quite theorical, so is the limitation or new constrains you add.

I have 2 questions I would like to answer before accepting your change:
- is there a better way to handle time warping without involving integer arithmetic and adding new constrains
- is the current code good enough in case the arch has 32bit int? (no integer promotion)  
 
> 
> > > 
> > > > 
> > > > > 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
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>