<Sound Dev> [9] Review Request: 8152501 closed/javax/sound/sampled/FileWriter/WaveBigEndian.java failing

[Alex Menkov]

Looks ok to me.

--alex

On 30.03.2016 19:33, Sergey Bylokhov wrote:
> Hi, Alex.
> On 29.03.16 17:13, Alex Menkov wrote:
>>> - LSB: when the number of bits is not dividable by 8
>>
>> when sampleSize % 8 != 0 we need to clean unused bits, then we can
>> handle the data as 8/16/24/etc samples (least significant bits are
>> zeroes).
>>
>> 32+ bits - as far as I see the converter uses only 32 most significant
>> bits - you pointed that for 32bit <-> float conversion rounding error is
>> tiny, so for 32+ bit this way produces accurate enough results too.
>
> I spent some time to understand why it works differently in 32bits vs
> 32+bits, I finally realize that there is a typo :
> AudioFloatConversion32xUB.toByteArray {
> ....
>      int x = (int) (in_buff[ix++] * 2147483647.0);
> ....
> }
>
> Note that the literal is double, so
> -1.0 * 2147483647.0 is -2147483647, but in case of float literal
> -1.0 * 2147483647.0f is -2147483648 which is minimal int value.
>
> Here is a new version, 24 and 32+ bits were fixed:
> http://cr.openjdk.java.net/~serb/8152501/webrev.01
>
> I also tested the code when the bits are not dividable by 8, and I see
> that the current implementation uses the algorithm you described and
> this cause of some lost of precision (so I cannot convert float to
> integral then to float again and compare results) I can change the logic
> but in separate CR.
>
>>
>>>
>>> My experiments show that there are possible issues related to
>>> overflow/underflow. So I postpones that change until I'll prove that the
>>> current code is ok and only the same change (like in this fix) is
>>> necessary.
>>>
>>>> On 29.03.2016 14:33, Sergey Bylokhov wrote:
>>>>> Hello,
>>>>> Please review the fix for jdk9.
>>>>>
>>>>> Description:
>>>>>   After the jigsaw integration the closed test WaveBigEndian fails.
>>>>> The
>>>>> reason is that the order of service providers is not specified in the
>>>>> jigsaw(I guess it will be changed in the future).
>>>>> The test verify that the audio data(pcm signed) during conversion is
>>>>> not
>>>>> changed corrupted. The java sound has two codecs which can convert pcm
>>>>> signed: PCMtoPCMCodec(base on int) and AudioFloatConverter(based on
>>>>> float). It was found that the test fails when AudioFloatConverter is
>>>>> used. This is because PCMtoPCMCodec converts data of
>>>>> pcm_signed/pcm_unsigned directly, and AudioFloatConverter converts
>>>>> data
>>>>> to the intermediate floats[-1.0; 1.0] and then converts it to the
>>>>> target
>>>>> format.
>>>>>
>>>>> Example of formulas are used by AudioFloatConverter.
>>>>>   - Signed byte to/from float: byte / 127.0f <--> byte * 127. Has a
>>>>> problem with a minimum byte "-128". it is outside of "(float) -1"
>>>>>   - Signed byte to/from unsigned: byte + 127 <--> byte - 127. Converts
>>>>> "-128" to "-1" instead of zero, and 127 to "254" instead of 255.
>>>>>
>>>>> Similar issues exists for 16 bits/32bits.
>>>>> The code was change to:
>>>>>   - Signed byte to/from float: byte > 0 ? byte / 127.0f : byte /
>>>>> 128.0f
>>>>>   - Signed byte to/from unsigned: byte + 128 <--> byte - 128.
>>>>>
>>>>> Note that for 32 bits only the second step is changed,
>>>>> division/multiplication error is quite tiny in this case.
>>>>>
>>>>> Also note that PCMtoPCMCodec can be removed after this fix, I'll do
>>>>> that
>>>>> later if the performance of two codecs is similar. I guess other
>>>>> conversions(LSB, 24 bit, 32+) in AudioFloatConverter should be
>>>>> investigated also, I'll do that later.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8152501
>>>>> Webrev can be found at:
>>>>> http://cr.openjdk.java.net/~serb/8152501/webrev.00
>>>>>
>>>
>>>
>
>