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

[Alex Menkov]

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
>