<Sound Dev> [9] Review Request: 8135100 Behavior of null arguments not specified in javax.sound.sampled.spi

[Phil Race]

The specification of the sound properties file - and the system property -
seems to imply that you can configure javax.sound to use classes that are
external to the JDK. So far as I know this has not come up in looking into
things that may need some work for jigsaw.

Also the very location of the sound.properties file will need to be updated
to reflect the modular jdk layout. Is this file expected to be configured by
end-users ? If so it would end up in the "conf" directory.

Aside from all of that, the various spec clean up should have an approved
CCC before it can be pushed.

-phil.


On 09/25/2015 08:41 AM, Sergey Bylokhov wrote:
> Hi, Alexey.
> Thanks for review! see my comments inline.
>
> On 25.09.15 18:23, alexey menkov wrote:
>> Hi Sergey,
>>
>> Overall looks good, but I don't like change in MixerProvider.getMixer
>> and PortMixerProvider.getMixer.
>>
>> MixerProvider.getMixer(null) is used by AudioSystem.getMixer(null) (to
>> get the default mixer), but provide PortMixer as the default does not
>> look good, I'd expect AudioSystem.getMixer(null) returns some
>> playback-able device. (Note also that for Ports "SourceDataLine" means
>> controls for recording device)
>
>>
>> Also comment for MixerProvider.getMixer(Mixer.Info):
>>  > * @param  info an info object that describes the desired mixer,
>>  > *         or {@code null} for the system default mixer
>>
>> is unclear and is not consistent with the description above:
>>  > * The full set of the mixer info objects that represent the mixers
>>  > * supported by this {@code MixerProvider} may be obtained through the
>>  > * {@code getMixerInfo} method. Use the {@code isMixerSupported}
>> method to
>>  > * test whether this {@code MixerProvider} supports a particular 
>> mixer.
>> It looks like MixerProvider.getMixerInfo should add "null" to the
>> supported mixers and MixerProvider.isMixerSupported(null) should return
>> "true".
>
> In this case the null means that some default mixer will be returned. 
> I am not sure that isMixerSupported(null) should return true, instead 
> I can clarify the specification of getMixer(null), and mention that if 
> null is provided then this mixer will try to return some default( 
> supported) mixer if possible, otherwise IllegalArgumentException will 
> be thrown.
>
> For the case of PortMixerProvider and "null" I can throw a 
> IllegalArgumentException which will mean that this provider do not 
> have "default" mixer.
>
>>
>> For now I don't have a proposal how to fix this.
>> Maybe it would be better to fix behavior of AudioSystem.getMixer(null) -
>> now it ignores "sound.properties" file (see AudioSystem spec for
>> explanations).
>>
>> regards
>> Alex
>>
>> On 14.09.2015 16:29, Sergey Bylokhov wrote:
>>> Hello Audio Guru.
>>>
>>> Please review the fix for jdk9. This issue is a subtask of:
>>> 4912693: Behavior of null arguments not specified in Java Sound
>>>
>>> In this patch I cover the whole javax.sound.sampled.spi package.
>>>
>>> The small description of the fix:
>>> - I have checked all methods in the spi package and all related methods
>>> in AudioSystem class.
>>> - I have moved related tests to the folder corresponding the package 
>>> and
>>> class name.
>>> - I have written a tests for every method and class which I changed.
>>> Note that these classes related to the different service providers, 
>>> so I
>>> have covered all installed implementations of each provider.
>>>
>>>
>>> Long description.
>>> I splits the fix to 3 use cases:
>>> - If the method always throw a NPE, then I simply update a javadoc and
>>> write a small test.
>>> - If the method most of the time throw a NPE then I update a javadoc 
>>> and
>>> change the method to always throw a NPE. Also I write a test which 
>>> tries
>>> to emulate both cases when NPE was thrown and when not. For example
>>> AudioFileWriter.isFileTypeSupported(Type) always throws a NPE if at
>>> least one type is supported, but if the array is empty then false is
>>> returned.
>>> - If the method have a few parameters and throw a NPE for some set of
>>> them. For example AudioFloatFormatConverter.
>>> isConversionSupported(Encoding,AudioFormat), the appropriate test tries
>>> to cover these cases.
>>>
>>> It turned out that all methods throw a NPE except of one:
>>> AudioSystem.getMixer()(MixerProvider.getMixer()), but it was found that
>>> the specification of MixerProvider.getMixer has no information about 
>>> the
>>> null, so I copied it from the AudioSystem.getMixer(). Also one
>>> implementation of MixerProvider  - PortMixerProvider.getMixer() throws
>>> NPE, so updated its implementation to the same as
>>> DirectAudioDeviceProvider.getMixer();
>>>
>>> I have done all related regression/jck/sqe tests, and I found one issue
>>> in jck and regression tests. Both are related to JDK-4941629 [1] (see
>>> comments in this CR). The jck test assumes that the method
>>> AudioSystem.write(ais, null, stream) should throw
>>> IllegalArgumentException. But according to specification it should 
>>> throw
>>> IllegalArgumentException if the type is unsupported, but the related
>>> method  AudioSystem.isFileTypeSupported(Type) will always throw a NPE
>>> for null. I prefer to file a bug against jck for this case.
>>>
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-4941629
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8135100
>>> The new test: http://cr.openjdk.java.net/~serb/8135100/webrev.01
>>>
>
>