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

[Sergey Bylokhov]

On 25.09.15 20:32, alexey menkov wrote:
> Ok, lets only clarify MixerProvider.getMixer(null) behavior.
> Actually AusioSystem.getMixer(null) looks unclear because unclear what
> is the "default mixer" in the case (we may have different "default
> playback mixer", "default recording mixer", "default port mixer").

Right, if to consider that the sequence of providers isn't specified, 
then it is unclear what this method should actually return. I do not 
understand the purpose to return some random mixer. I think that 
intention was to return "default playback mixer"?

>
> regards
> Alex
>
> On 25.09.2015 18:41, 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
>>>>
>>
>>


-- 
Best regards, Sergey.