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

[Sergey Bylokhov]

On 25.09.15 19:29, Phil Race wrote:
> 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.

yes, some update for jigsaw should be done.

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

Yes it can be updated by the users, and It is already in the conf directory.

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

I will create a ccc request after the technical review.

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


-- 
Best regards, Sergey.