[pulseaudio-discuss] [PATCH 04/10] remap: Add stereo to mono special case remapping

[Tanu Kaskinen]

On 04/01/2013 06:52 PM, Thomas Martitz wrote:
> Am 01.04.2013 17:11, schrieb Tanu Kaskinen:
>> On 04/01/2013 03:28 PM, Thomas Martitz wrote:
>>> Am 01.04.2013 13:13, schrieb Peter Meerwald:
>>>> Hello Tanu,
>>>>
>>>> thanks for reviewing!
>>>>
>>>>>>> + case PA_SAMPLE_S16NE:
>>>>>>> + {
>>>>>>> + int16_t *d = (int16_t *) dst, *s = (int16_t *) src;
>>>>>>> +
>>>>>>> + for (i = n>> 2; i> 0; i--) {
>>>>>>> + d[0] += (s[0] + s[1])/2;
>>>>>>> + d[1] += (s[2] + s[3])/2;
>>>>>>> + d[2] += (s[4] + s[5])/2;
>>>>>>> + d[3] += (s[6] + s[7])/2;
>>>>>> You probably meant '=', not '+='?
>>>>>>
>>>>>> Also, s[0] + s[1] can overflow, so the inputs should be divided
>>>>>> individually before summing.
>>>>> Or the inputs could be cast to uint32_t, which I guess would be better
>>>>> than dividing multiple times.
>>>>
>>>> I think (s[0] + s[1])/2 is correct; at least as long as sizeof(int) >
>>>> sizeof(short); short gets promoted to int -- see 'integer promotion in
>>>> C99'; also my compiler thinks above is correct :)
>>>>
>>>
>>>
>>> Why should int16+int16 be promoted to int32? Pretty sure this code is
>>> prone to overflow.
>>
>> I was pretty sure too, until I tried compiling a test program. I think
>> this text in the C99 standard[1] (section 6.3.1.1) means that shorts
>> are always promoted to ints, even if both operands of a calculation
>> are shorts:
>>
>> "The following may be used in an expression wherever an int or
>> unsigned int may be used:
>>
>> - An object or expression with an integer type whose integer
>> conversion rank is less than or equal to the rank of int and unsigned
>> int.
>>
>> - A bit-field of type _Bool, int, signed int, or unsigned int.
>>
>> If an int can represent all values of the original type, the value is
>> converted to an int; otherwise, it is converted to an unsigned int.
>> These are called the integer promotions. All other types are unchanged
>> by the integer promotions."
>>
>> I have trouble understanding that text, but that's the closest thing
>> that I found that would explain the observed behavior.
>
> Not sure it's related to our case. I have trouble understanding it too.
> Anyway, even if int16+int16 is promoted to int32+int32, the result still
> has to fit into an int16. If it doesn't (overflow) then undefined
> behavior happens.
>
> But since in this instance the result is divided by two before storing
> the result it should be fine.

Sure, the only question is whether the intermediate result of the sum 
will get corrupted. If not, there's no risk of overflow in the final result.

>>
>> There's also this example in section 5.1.2.3:
>>
>> "EXAMPLE 2
>>
>> In executing the fragment
>>
>> char c1, c2;
>> /* ... */
>> c1 = c1 + c2;
>>
>> the "integer promotions" require that the abstract machine promote the
>> value of each variable to int size and then add the two ints and
>> truncate the sum. Provided the addition of two chars can be done
>> without overflow, or with overflow wrapping silently to produce the
>> correct result, the actual execution need only produce the same
>> result, possibly omitting the promotions."
>
> I think the example is only valid when the result fits into a char.
> Overflow in signed arithmetic is explicitly undefined, unlike unsigned
> arithmetic, where wrap-to-zero is guaranteed.

The example seems to suggest that overflow is actually defined for 
signed chars.

-- 
Tanu