RFR: 8356049: Need a simple way to play back a sound clip [v2]

Alexey Ivanov aivanov at openjdk.org
Wed May 7 17:03:31 UTC 2025 

On Fri, 2 May 2025 23:00:32 GMT, Phil Race <prr at openjdk.org> wrote:

>> A simple API to replace java.applet.AudioClip
>> 
>> CSR is now created : https://bugs.openjdk.org/browse/JDK-8356200
>
> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8356049

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/com/sun/media/sound/DataPusher.java line 97:

> 95:     public boolean isPlaying() {
> 96:         return threadState == STATE_PLAYING;
> 97:     }

I think this method should be `synchronized`.

(The `threadState` field is modified in `run()` without synchronization, which isn't guaranteed to be visible by other threads. But it's a different issue.)

src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java line 104:

> 102:                 throw e;
> 103:             }
> 104:         }

Can any other (checked) exception be thrown?

I can't see that it's possible, then catching it isn't necessary.

src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java line 113:

> 111:             clip.init(uc.getInputStream());
> 112:         } catch (final Exception ignored) {
> 113:             // Playing the clip will be a no-op if an exception occured in inititialization.

Suggestion:

            // Playing the clip will be a no-op if an exception occurred in inititialization.

src/java.desktop/share/classes/javax/sound/SoundClip.java line 40:

> 38:  * Typically this means running the application in a desktop environment.
> 39:  *
> 40:  * Multiple {@code SoundClip} items can be playing at the same time, and

Suggestion:

 * Typically this means running the application in a desktop environment.
 *
 * <p>
 * Multiple {@code SoundClip} items can be playing at the same time, and

I assume a new paragraph starts with _“Multiple…”_.

src/java.desktop/share/classes/javax/sound/SoundClip.java line 47:

> 45: public final class SoundClip {
> 46: 
> 47:     private JavaSoundAudioClip clip;

Suggestion:

    private final JavaSoundAudioClip clip;

The value doesn't change after an instance is created, so it can and should be final.

src/java.desktop/share/classes/javax/sound/SoundClip.java line 50:

> 48: 
> 49:     /**
> 50:      * Create a {@code SoundClip} instance which will play a clip from the supplied file.

Suggestion:

     * Creates a {@code SoundClip} instance which will play a clip from the supplied file.

src/java.desktop/share/classes/javax/sound/SoundClip.java line 52:

> 50:      * Create a {@code SoundClip} instance which will play a clip from the supplied file.
> 51:      *
> 52:      * If the file does not contain recognizable and supported sound data, or

Suggestion:

     * <p>
     * If the file does not contain recognizable and supported sound data, or

Without `<p>`, this block will be rendered right after the first sentence above without a paragraph break. If it's intended, I propose removing a seemingly empty line.

src/java.desktop/share/classes/javax/sound/SoundClip.java line 56:

> 54:      * playing the clip will be a no-op.
> 55:      *
> 56:      * @param file from which to obtain the sound data

Suggestion:

     * @param file the file from which to obtain the sound data

src/java.desktop/share/classes/javax/sound/SoundClip.java line 69:

> 67: 
> 68:     private SoundClip() {
> 69:     }

If the no-arg constructor is never used, then it's not needed and it should be removed.

src/java.desktop/share/classes/javax/sound/SoundClip.java line 78:

> 76:      * Returns whether this is a playable sound clip.
> 77:      * A value of {@code false} means that calling any of the other methods
> 78:      * of this class is a no-op

Suggestion:

     * of this class is a no-op.

src/java.desktop/share/classes/javax/sound/SoundClip.java line 90:

> 88:      * @return whether sound is currently playing
> 89:      */
> 90:     public boolean isPlaying() {

Suggestion:

    /**
     * {@return whether sound is currently playing}
     */
    public boolean isPlaying() {

src/java.desktop/share/classes/javax/sound/package-info.java line 32:

> 30:  * Capture, processing and playback of sampled audio data is under {@link javax.sound.sampled}
> 31:  * <p>
> 32:  * Sequencing, and synthesis of MIDI (Musical Instrument Digital Interface) data is under {@link javax.sound.midi}

Suggestion:

 * Capture, processing and playback of sampled audio data is under {@link javax.sound.sampled}.
 * <p>
 * Sequencing, and synthesis of MIDI (Musical Instrument Digital Interface) data is under {@link javax.sound.midi}.

test/jdk/javax/sound/SoundClip/SoundClipTest.java line 43:

> 41:         if (!isSoundcardInstalled()) {
> 42:             return;
> 43:         }

It should throw `jtreg.SkippedException` instead of pretending the test passes.

test/jdk/javax/sound/SoundClip/SoundClipTest.java line 60:

> 58:             if (clip.isPlaying()) {
> 59:                 playing = true;
> 60:                 break;

`break` is somewhat redundant since the loop has `!playing` as the condition for executing the body.

test/jdk/javax/sound/SoundClip/SoundClipTest.java line 101:

> 99:         } catch (Exception e) {
> 100:             System.err.println("Exception occurred: "+e);
> 101:         }

The stack trace of the exception could be crucial to understanding what went wrong, so printing the stack trace could be a better approach than just the error message.

I guess you want to pass the test (or as I suggest throw `jtreg.SkippedException`) if anything goes wrong.

`AudioSystem.getSourceDataLine` throws `LineUnavailableException` which is checked exception, and if the exception

-------------

PR Review: https://git.openjdk.org/jdk/pull/24991#pullrequestreview-2822446607
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078030777
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078040610
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078061539
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078065416
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078047776
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078048603
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078050809
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078051444
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078053524
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078054567
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078057503
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078067119
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078070770
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078072903
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078084485

More information about the client-libs-dev mailing list