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

Florian Bomers javasound-dev at bome.com
Fri Oct 23 18:56:23 UTC 2015 

Hi Sergey,

I guess you're right and the second loop will never be executed if we
will always have the default mixer providers.

Removing the NPE catch clause, however, will still cause a backwards
incompatibility, because if a poorly programmed MixerProvider gets
installed which throws NPE for whatever reason (might also happen when
"info" is non-null), now AudioSystem.getMixer() will throw NPE, where
it previously worked.

I agree that it's harder for debugging mixer providers if NPE is
ignored. Other than that, I don't see any problem with keeping the NPE
catch for backwards compatibility's sake. Even if just theoretical...
But you never now, companies might be using poorly programmed in-house
software or the like.

Thanks,
Florian



On 23.10.2015 19:58, Sergey Bylokhov wrote:
> Hi, Florian.
> Thanks for review! see my comments inline.
> 
>> 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.
> 
> This is tricky place, I have some thoughts about this, which I would
> like to discuss.
> 
> I studied the history to find an answer why the catch of NPE and the
> second loop were added.
> 
> - The catch of the null was added in the 1999 because of "added a
> catch for an NPE -- Netscape tends to throw this if some strings
> haven't been set in our device provider info objects".
> It is actually a workaround. Because of this patch we did not catch
> some bugs in our MixerProviders. For example PortMixerProvider,
> SimpleInputDeviceProvider(old mixer) were thrown NPE, when they tried
> to throw IllegalArgumentException. In the same moment the
> DirectAudioDeviceProvider, HeadspaceMixerProvider(old mixer),
> SoftMixingMixerProvider are ready for null. I also checked the
> TMixerProvider from tritonus it also ready for null.
> 
> 
> - The second loop was added in "JDK-4834461: Linux: Applet hang when
> you load it during sound card is in use". This is also a workaround
> for a situation when we try to get a default mixer in the first loop
> but it was not available for some reason, in this case we will return
> first mixer from the first mixer provider. But the second loop will be
> run only if the first loop will not find the default mixer in some
> other providers.
> 
> So imagine this situation when some old 3rd party MixerProvider is used:
> - The user sets some provider, which throw the NPE on null in getMixer();
> - The first loop call this provider and skip it in catch block
> - The next bundled provider will be used instead of user's mixer, the
> second loop is not executed.
> - If we use this approach the user will not be able to check that
> wrong provider is in use, and in case of NPE the temporary workaround
> will be - removing of custom mixer provider which are not used anyway.
> 
> Note that in jdk9 the "META-INF/services" will not be used, so there
> will be no option to remove bundled providers via configs, and we will
> always iterate over the bundled providers in the first loop.
> 
> For the case of some other vendors of jdk, after the moment of
> specification clarification all default providers should not
> contradict the specification, and should not throw NPE in getMixer(),
> but return default mixer.
> 
> Does it make sense?
> 
>>
>>
>> [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