Bug 48637 - [PATCH] sound task either does nothing or endlessly loops sound.
Summary: [PATCH] sound task either does nothing or endlessly loops sound.
Status: RESOLVED FIXED
Alias: None
Product: Ant
Classification: Unclassified
Component: Optional Tasks (show other bugs)
Version: 1.7.1
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: 1.8.0
Assignee: Ant Notifications List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-28 14:32 UTC by Ed Brannin
Modified: 2010-01-29 08:26 UTC (History)
0 users



Attachments
Proof-of-problem project. (543.65 KB, application/x-zip-compressed)
2010-01-28 14:34 UTC, Ed Brannin
Details
Patch against 1.8.0RC1 to fix both issues (and fix some comment spelling) (1.19 KB, patch)
2010-01-29 07:44 UTC, Ed Brannin
Details | Diff
Patch against 1.8.0RC1 to fix both issues (and fix some comment spelling) - PLUS no more busy-waits. (1.43 KB, patch)
2010-01-29 08:02 UTC, Ed Brannin
Details | Diff
A test class that explains and demonstrates both issues (uses Windows sounds by default). (1.79 KB, text/x-java-source)
2010-01-29 08:12 UTC, Ed Brannin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Brannin 2010-01-28 14:32:25 UTC
This bug exists in 1.7.0 and 1.8.0RC1, and has existed since at least November 2008: http://bit.ly/d8ydaU

Using only the "source" attribute on <sound>'s <success> and <fail> elements, the sound task does nothing.

As the email thread above says, using the duration task causes the sound to loop forever.

I'm attaching a proof-of-concept, including some sounds and a tiny Java class that proves javax.sound works.
Comment 1 Ed Brannin 2010-01-28 14:34:42 UTC
Created attachment 24902 [details]
Proof-of-problem project.
Comment 2 Ed Brannin 2010-01-29 07:44:31 UTC
Created attachment 24905 [details]
Patch against 1.8.0RC1 to fix both issues (and fix some comment spelling)

I downloaded the 1.8.0RC1 source distribution and fixed both issues this morning.

===

In org.apache.tools.ant.taskdefs.optional.sound.AntSoundPlayer.playClip(Clip, int): 
There was a race condition between clip.loop() and clip.isRunning() where the system could exit before the clip started.

I don't like how this takes one busy-wait and turns it into two busy-waits, and am going to replace them with a do/try/sleep loop in a minute.  I'm uploading this patch now in case you prefer this version.

In org.apache.tools.ant.taskdefs.optional.sound.AntSoundPlayer.playClip(Clip, long):
clip.loop(Clip.LOOP_CONTINUOUSLY) and the ensuing Thread.sleep(duration) worked fine, but no one ever called clip.stop() at the end.
Comment 3 Ed Brannin 2010-01-29 08:02:54 UTC
Created attachment 24906 [details]
Patch against 1.8.0RC1 to fix both issues (and fix some comment spelling) - PLUS no more busy-waits.

The original code (and my original fix) would busy-wait for the sound to finish playing.  On my system, the "empty" loops (now with "waits += 1") ran about 940 million times.

This patch reduces the wait-count to around 2-20.  Higher wait-counts are from  the lag between clip.getMicrosecondLength() == clip.getMicrosecondPosition() and clip.isRunning() going back to false.
Comment 4 Ed Brannin 2010-01-29 08:12:16 UTC
Created attachment 24907 [details]
A test class that explains and demonstrates both issues (uses Windows sounds by default).

Here's a test class that reproduces the issue, directly using the AntSoundPlayer class.  It uses c:\windows\media\tada.wav by default, so you'll have to change the path if you're on OS X or Linux.

I'm obsoleting the attachment that reproduced the issues via an ant buildfile.

I didn't write it as a JUnit test because it requires some manual input and verification.
Comment 5 Ed Brannin 2010-01-29 08:16:53 UTC
Comment on attachment 24907 [details]
A test class that explains and demonstrates both issues (uses Windows sounds by default).

updating the MIIME type to text/x-java-source
Comment 6 Stefan Bodewig 2010-01-29 08:23:16 UTC
Actually I started looking into the issue just before you attached your first patch.  I'm about to commit a modified version of your second patch in a few minutes (by sheer luck I ran into a situation where timeLeft turned negative in my very first loop test).

The race condition never occurred with Java 1.4 for me, but I always heard the results you described when using 1.6.  The patched version works on either Java version.
Comment 7 Stefan Bodewig 2010-01-29 08:26:43 UTC
svn revision 904546

Thanks!