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
