search

dubhater / vapoursynth-mvtools

Motion compensation and stuff
184 stars 28 forks source link

Fixed some possible overflows #22

Closed IFeelBloated closed 8 years ago

dubhater commented 8 years ago

Why do you mix C and C++ casts?

IFeelBloated commented 8 years ago

No particular reason, just tryna make things look pretty

The inner cast is inside an expression, so use a simpler C style cast to not make the expression look messy

The outside cast covers the whole expression up so use a C++ cast to make things clearer

dubhater commented 8 years ago

I see. Anyway, I'd rather cast to int only the result of the right shift. That definitely fits in int. The result of the addition may not, and I want the sanitizer to catch that. Same for the other pull request.

IFeelBloated commented 8 years ago

Anyway, I'd rather cast to int only the result of the right shift.

done

a few more questions tho,

  1. is it a good idea to use double to do all the intermediate calculations so things will never ever overflow?
  2. what is "left shift of negative value -1"? something fishy is going on with "ilog2"?
dubhater commented 8 years ago
  1. double doesn't have infinite precision either, and it just slows things down.
  2. In this case, a negative value was shifted left. (The same message could also mean a shift by a negative value.) Whatever the cause, it happens with 8 bit too, so it's probably a bug inherited from the Avisynth plugin.
IFeelBloated commented 8 years ago
  1. I just thought the precision loss of floating point calculation is somehow less fatal than integer overflow...
  2. think I fixed the weird negative value shifting stuff, the coordinates of the vector could be either >0 or 0>, the old mvtools just assumed them >0 anyways, so * pow(2,x) becomes <<x, but that doesn't work on negative values cuz the sign bit will be shifted away..
dubhater commented 8 years ago

The sign bit is fine, because the compiler will generate an arithmetic right shift (x86 instruction sar). This shifts in copies of the sign bit, not zero. It's not exactly the same as an integer division, but it's close enough. At least it's not completely wrong like a left shift of a negative value.

Anyway, this is more complicated than it needs to be and also I can't merge it anymore because of changes.