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

[Alexey Ivanov]

On Wed, 7 May 2025 20:47:16 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

Looks good to me except for a few nits.

I guess you should also update the copyright years in modified files.

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

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

I still think the `isPlaying` should be declared `synchronized`.

You resolved my previous comment but nothing's changed in the code.

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

> 117:             clip.init(url.openStream());
> 118:         } catch (final Exception ignored) {
> 119:             // Playing the clip will be a no-op if an exception occurred in inititialization.

Suggestion:

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

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

> 50:      * Creates 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

If the two sentences are intended to be in one paragraph, I strongly suggest removing the blank line between them. Otherwise, the blank line in the javadoc source hints that the second sentence should be in its own paragraph.

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}.

The description fits into an unordered list.

Suggestion:

 * The API is divided into sub-packages:
 * <ul>
 *   <li>Capture, processing and playback of sampled audio data is under {@link javax.sound.sampled}.
 *   <li>Sequencing, and synthesis of MIDI (Musical Instrument Digital Interface) data is under {@link javax.sound.midi}.
 * </ul>


The resulting text could be clearer and easier to skim quickly:
> The API is divided into sub-packages:
> * Capture, processing and playback of sampled audio data is under `javax. sound. sampled`.
> * Sequencing, and synthesis of MIDI (Musical Instrument Digital Interface) data is under `javax. sound. midi`.

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

> 72:                 playing = false;
> 73:                 break;
> 74:             }

Suggestion:

            if (clip.isPlaying()) {
                playing = false;
            }

Since you removed the `break` statement from the loop above, this one needs to go too for consistency.

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

> 89:      * on the system.
> 90:      */
> 91:     public static boolean isSoundcardInstalled() {

Suggestion:

    /**
     * Returns true if at least one soundcard is correctly installed
     * on the system.
     */
    public static boolean isSoundcardInstalled() {

The start of the comment is misaligned.

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

Changes requested by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24991#pullrequestreview-2828804099
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081937814
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081976115
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081955900
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081966734
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081992607
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081993946