<Sound Dev> [9] Review request for 8168751: Two "Direct Clip" threads are created to play the same "AudioClip" object, what makes clip sound corrupted

[Sergey Bylokhov]

+1

> 
> Looks good to me.
> 
> --alex
> 
> On 20.01.2017 16:35, Anton Litvinov wrote:
>> Hello Sergey,
>> 
>> Thank you for review of this fix. Yes, I agree that 4 fields of
>> "DirectAudioDevice.DirectClip" class which you specified
>> ("loopStartFrame", "loopEndFrame", "newFramePosition",
>> "clipBytePosition") are also not thread-safe and should be marked with
>> "volatile" modifier, though they are not directly related to this bug.
>> 
>> The second version of the fix, which addresses your remark, was created.
>> In the second version of the fix "volatile" modifier was added to the
>> next 8 fields of "DirectClip" class, all of which are accessed both from
>> "DirectClip.run()" method and other methods of this class: "audioData",
>> "frameSize", "m_lengthInFrames", "loopCount", "clipBytePosition",
>> "newFramePosition", "loopStartFrame", "loopEndFrame". Could you please
>> review the second version of the fix.
>> 
>> Webrev: http://cr.openjdk.java.net/~alitvinov/8168751/jdk9/webrev.01
>> 
>> Thank you,
>> Anton
>> 
>> On 1/19/2017 12:34 AM, Sergey Bylokhov wrote:
>>> Hi, Anton.
>>> Probably some other fields in the DirectClip should be marked as
>>> volatile? For example loopStartFrame/loopEndFrame/ are used in the
>>> run() but assigned in the setLoopPoints
>>> () w/o any synchronization?(the same setFramePosition
>>> +newFramePosition/clipBytePosition)
>>> 
>>>> Hello,
>>>> 
>>>> Could you please review the following fix for the bug.
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8168751
>>>> Webrev: http://cr.openjdk.java.net/~alitvinov/8168751/jdk9/webrev.00
>>>> 
>>>> The bug consists in the fact that after many repetitive sequential
>>>> calls to the methods "loop()", "stop()" on the same instance of
>>>> "java.applet.AudioClip" at some moment two threads with the name
>>>> "Direct Clip", which play this "AudioClip" instance, are created and
>>>> start existing in parallel, what makes the being played clip sound as
>>>> corrupted.
>>>> 
>>>> "Direct Clip" thread is represented by the instance variable
>>>> "com.sun.media.sound.DirectAudioDevice.DirectClip.thread", is created
>>>> in the method
>>>> "com.sun.media.sound.DirectAudioDevice.DirectClip.open()" and is
>>>> stopped by assigning "null" value to "DirectClip.thread" instance
>>>> variable in the method
>>>> "com.sun.media.sound.DirectAudioDevice.DirectClip.implClose()".
>>>> 
>>>> THE DEFINED ROOT CAUSES OF THE BUG:
>>>> 
>>>> 1. The fact that "DirectClip.thread" instance variable is not
>>>> thread-safe. There is a possibility that null value assigned to it in
>>>> one thread through the former mentioned "DirectClip.implClose()"
>>>> method does not become visible for the running "Direct Clip" thread
>>>> in the method "DirectClip.run()".
>>>> 
>>>> 2. In the method "DirectClip.run()" not taking into account the fact
>>>> that a new "Direct Clip" thread could have already been started and
>>>> assigned to "DirectClip.thread" instance variable after the call to
>>>> "DirectClip.implClose()", by the moment when "DirectClip.thread"
>>>> value is checked for null in "DirectClip.run()" method, or by the
>>>> moment when the previous "Direct Clip" thread is awakened and
>>>> continues execution after the following code block in
>>>> "DirectClip.run()" from the file
>>>> "jdk/src/java.desktop/share/classes/com/sun/media/sound/DirectAudioDevice.java".
>>>> 
>>>> 
>>>> 1354                 try {
>>>> 1355                     lock.wait();
>>>> 
>>>> 1356                 } catch(InterruptedException ie) {}
>>>> 
>>>> THE SOLUTION:
>>>> The solution is based on correction of the listed above 2 root causes
>>>> of the bug.
>>>> 
>>>> Thank you,
>>>> Anton
>>