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

Thu Oct 8 08:46:07 UTC 2015

> For me the most logical is to return default playback mixer :)

yes, at the time it was most important to provide an easy way to get a
playback device.

Can you send a new webrev?
Thanks,
Florian

On 25.09.2015 20:33, alexey menkov wrote:
> 
> 
> On 25.09.2015 20:42, Sergey Bylokhov wrote:
>> 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"?
> 
> I don't know. I suppose this is ancient method (and most likely nobody
> use it) and the implementation was changed this way to get
> DirectAudioDevice as default (it supports both playback and recording).
> For me the most logical is to return default playback mixer :)
> 
> --alex
> 
>>
>>>
>>> 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
>>>>>>
>>>>
>>>>
>>
>>
> 

-- 
Florian Bomers
Bome Software

everything sounds.
http://www.bome.com
__________________________________________________________________
Bome Software GmbH & Co KG        Gesellschafterin:
Dachauer Str.187                  Bome Komplementär GmbH
80637 München, Germany            Geschäftsführung: Florian Bömers
Amtsgericht München HRA95502      Amtsgericht München HRB185574