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

[Phil Race]

On 09/25/2015 09:43 AM, Sergey Bylokhov wrote:
> 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.

Ah. I looked at the JDK 8 docs ! JDK 9 is already updated.

-phil.

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