On Tue, 13 May 2025 19:32:00 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8356049
>
> src/java.desktop/share/classes/javax/sound/SoundClip.java line 35:
>
>> 33: /**
>> 34: * The {@code SoundClip} class is a simple abstraction for playing a
>> sound clip.
>> 35: * It will play any format that is recognized by the {@code javax.sound}
>> API,
>
> I think we should mention in the doc that this is applicable for small audio
> files since we will load all data into the memory.
What is small ? I would not want to try to define that.
Since the application supplies the file it is in control of that and should
know what it is supplying.
Lots of APIs read files and don't mention that they need enough memory to hold
the data in the file.
The Applet case was historically even more constrained (64 Mb heap for applets
once upon a time) and
it seemed to be fine without this.
> src/java.desktop/share/classes/javax/sound/SoundClip.java line 45:
>
>> 43: * @since 25
>> 44: */
>> 45: public final class SoundClip {
>
> What about considering a different name, such as AudioClip with a static
> method like AudioClip.create(...)? or maybe Audio....Player?
I picked SoundClip because AudioClip over AudioClip for a few reasons. Mapping
to the Sound API name for one and because there's already an AudioClip.
"Player" might be construed as a UI component.
> src/java.desktop/share/classes/javax/sound/SoundClip.java line 63:
>
>> 61: public static SoundClip createSoundClip(File file) throws
>> IOException {
>> 62: if (file == null) {
>> 63: throw new IllegalArgumentException("file must not be null");
>
> Most of the APIs in javax.sound.* throw NullPointerException for null
> arguments and IllegalArgumentException for other invalid parameters.
Using NPE hadn't escaped me.
I think that was probably common practice back then, but I think IAE is better
here.
> src/java.desktop/share/classes/javax/sound/SoundClip.java line 78:
>
>> 76: * of this class is a no-op.
>> 77: */
>> 78: public synchronized boolean canPlay() {
>
> Should we perhaps document the multithreaded use of Clip? Do we really need
> all these synchronized keywords in the file?
The API calls are best protected this way just like the ones in AudioClip were.
There's no advantage in removing them. I don't think there's anything to
document.
> src/java.desktop/share/classes/javax/sound/package-info.java line 43:
>
>> 41: * @since 25
>> 42: */
>> 43: package javax.sound;
>
> Why javax.sound and not sampled? do we want to support midi files by
> SoundClip as well?
That and a top-level is easier to find. I did spend some time debating with
myself over whether to create a new package just for this class but it made the
most sense.
> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 34:
>
>> 32: * @key sound headful
>> 33: * @summary basic testing of javax.sound.SoundClip
>> 34: * @run main/othervm SoundClipTest javasound.wav
>
> What about a non-existent or broken file? Should we check for an IOException
> in that case?
I could do a minor test enhancement for this case. I don't think it would be an
IOException for a broken file, it would just not be playable.
> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 41:
>
>> 39: public static void main(String[] args) throws Exception {
>> 40:
>> 41: if (!isSoundcardInstalled()) {
>
> Why we cannot check that the playing sound is a no-op in this case, as
> specified?
The intent is that this should always run on a system with sound. It just might
happen that it isn't, so this isn't a case to start adding tests.
That might be better done by feeding it unplayable data.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087653970
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087618093
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087623944
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087626652
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087628133
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087631361
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087647725
