<Sound Dev> [9] Review Request: 8135100 Behavior of null arguments not specified in javax.sound.sampled.spi
Florian Bomers
javasound-dev at bome.com
Thu Oct 15 11:00:16 UTC 2015
Hi Sergey,
thank you for the changes and sorry for the late reply.
The changes look good to me, except this one:
> - I updated AudioSystem.getMixer():
> * Unnecessary try/catch are removed.
> * The second loop is removed. It was added as a fix for
> JDK-4834461 [1], but right now I do not see a reason for it. If our
> attempts in the first loop will fail, will mean that our providers
> do not have default mixers.
This is true for the included MixerProvders, but the requirement that
null will return the default Mixer is just made official. We should,
however, remain backwards compatible with 3rd party MixerProviders by
keeping that second loop. The old style is that a MixerProvider
returns its default Mixer as first element. Also, for backwards
compatibility, I'd also keep the catch clause for NPE in both loops.
Best regards,
Florian
On 08.10.2015 21:28, Sergey Bylokhov wrote:
> Hello.
>
> The new version of the fix:
> http://cr.openjdk.java.net/~serb/8135100/webrev.03/
>
> - I updated documentation of MixerProvider.getMixer() and described
> that the null is a valid value for requesting default mixer from this
> provider. I also added a notion that IllegalArgumentException will be
> thrown if provider does not have default mixer.
>
> - DirectAudioDeviceProvider.getMixer() was changed to do not throw
> NPE when info=null is passed and no mixers were found.
>
> - PortMixerProvider.getMixer() was changed to do not throw
> NPE(actually now it always throw IllegalArgumentException), when
> info=null is passed and no mixers were found. Note that in case of
> null the loop in this method do not throw NPE but find nothing,
> because the list of infos has no nulls.
>
> - I updated AudioSystem.getMixer():
> * Unnecessary try/catch are removed.
> * The second loop is removed. It was added as a fix for
> JDK-4834461 [1], but right now I do not see a reason for it. If our
> attempts in the first loop will fail, will mean that our providers do
> not have default mixers.
> Currently we have 3 providers:
> - PortMixerProvider will throw IAE for null
> - DirectAudioDeviceProvider returns something if atleast one
> mixer is supported.
> - SoftMixingMixerProvider always return SoftMixingMixer.
>
>
> [1] https://bugs.openjdk.java.net/browse/JDK-4834461
>
>
> On 08.10.15 11:46, Florian Bomers wrote:
>>> 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
More information about the sound-dev
mailing list