views:

58

answers:

4

I am studying a sound converting algorithm where an array of signed shorts is received.
At a given point in the algorithm it converts the samples from 16 bits to 14 bits, and it does it like this:

int16_t sample = (old_sample + 2) >> 2;

For me its clear that the shifting is needed as we want to get rid of the least 2 significant bits, but what about the +2 there?

+8  A: 

Shifting down loses the least significant two bits. If you just shift, then that will always round down, even if the bottom two bits are both set. Adding the 2 rounds up if the bigger of the bits being lost is set.

(Also worth noting that a better method of reducing the number of bits is to use dithering, i.e. adding a random (and very small) amount of noise before the reduction in sample size; this avoids the problem that, since the sounds are periodic, the rounding can often end up going consistently up or consistently down for a particular frequency, resulting in perceptible distortion in the sound. The wikipedia article linked explains it better than I can!)

psmears
Jaysus, the rounding, of course.
Fail
+1  A: 

I guess its intended to have the effect of rounding? I just hope that they conisdered the case of old_sample being more than MAX_INT16 - 2. Otherwise there might be issues when it overflows.

torak
if it is MAX_INT16 - 2, it still is ok ... I think old_sample should be casted to a 32bit int before adding 2, and then the shift will assure that the next casting to 16bit won't do harm... no wait, it could... it should be an arithmetic shift, not a logical one (if the sample is considered signed, as it seems); does >> perform an arithmetic shift by standard?
ShinTakezou
Yep, should read more than MAX_INT16, will edit to fix. I'm pretty sure that its an arithmetic shift for signed numbers and a logical shift for unsigned ones, though I could once again be wrong. Hopefully someone will point it out if I am.
torak
@ShinTakezou: The behaviour of shifting negative integers is implementation-defined, which in practice means either arithmetic or logical shift depending on what is easiest on the architecture in question.
caf
@torak: `int` isn't guaranteed to be any bigger than 16 bits, so you're right - it could overflow. Using `2L` as the constant fixes that problem, since `long` is at least 32 bits.
caf
@caf thanks. @torak @fail so moreover, the code can't assure the sign is preserved by the shift! (No harm if the signed int is intended as unsigned sample...)
ShinTakezou
A: 

The intent of that code is probably rounding as is indicated in the other replies. But this is certainly a very bad example of it. There are two things going on here that the original programmer probably didn't intend:

  • promotion to int and re-assignment to int16_t
  • right shift of a signed value

Promotion to int (because of the +2 which is just an int) is bad here because you don't know what the precision of int on any random platform is that you happen to land on.

Right shift of signed values is compiler dependent if the value is negative, so the outcome may vary from platform to platform, too.

Jens Gustedt
The promotion to `int` isn't too bad, because `int` must be *at least* 16 bits. It would, however, be better to promote to `long`, to avoid the problem of overflow if the input sample is 32766 or 32767.
caf
@caf: I don't agree with that view, in the contrary this is a very nasty type of bug. It makes you think that everything in the code works fine, until... The day somebody compiles it on a 16 bit platform (yes, I know rare these days) *and* encounters the corner case of an overflow in the `+2` you have a severe crash. Very nasty, impossible to debug. So a cast is definitively necessary here, and I would do it to `int32_t` to be consistent. (Better as I said, change the types to `unit..`)
Jens Gustedt
Neither the promotion nor the right shift issues matter when the result is being truncated to 14 bits. The bits that would be preserved by using a longer datatype are discarded anyway, as are the undefined bits.I don't see a "severe crash" as possible; the issue will be that the sample will effectively change sign. This could be audible in the output, but if the sample is that big then it is probably clipping anyway.Of course, dithering is the right answer.
janm
+1  A: 

As others have noted, the +2 is an attempt to make the right shift perform a round-to-nearest division. However, there are two problems:

  • input samples of 32766 or 32767 may overflow int when 2 is added (int is only guaranteed to be able to represent numbers up to 32767);

  • The behaviour of a right shift of a negative number is implementation-defined.

To avoid these issues, it should be:

int16_t sample = (old_sample > 0 ? old_sample + 2L : old_sample - 2L) / 4;

(Unlike the shift operator, the division operator in C99 is defined to round-towards-zero).

caf