Summary: | [PATCH] sound task either does nothing or endlessly loops sound. | ||
---|---|---|---|
Product: | Ant | Reporter: | Ed Brannin <edbrannin> |
Component: | Optional Tasks | Assignee: | Ant Notifications List <notifications> |
Status: | RESOLVED FIXED | ||
Severity: | normal | ||
Priority: | P2 | ||
Version: | 1.7.1 | ||
Target Milestone: | 1.8.0 | ||
Hardware: | PC | ||
OS: | Windows XP | ||
Attachments: |
Proof-of-problem project.
Patch against 1.8.0RC1 to fix both issues (and fix some comment spelling) Patch against 1.8.0RC1 to fix both issues (and fix some comment spelling) - PLUS no more busy-waits. A test class that explains and demonstrates both issues (uses Windows sounds by default). |
Description
Ed Brannin
2010-01-28 14:32:25 UTC
Created attachment 24902 [details]
Proof-of-problem project.
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.
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.
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 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
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. svn revision 904546 Thanks! |