I guess you're right and the second loop will never be executed if we
will always have the default mixer providers.
ok, we agreed that the second loop isn't necessary
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.
But if we discuss in this way we can get an assumption that any methods
of plugins can throw any type of exceptions and we should wrap them in
"catch (Throwable)". I agree that we should follow the backwards
compatibility as much as possible, but this case is related to "bug to
bug" compatibility. For sure if we fix a bug in jdk and if some code
relies on this behavior he will get an issue. But i think strategy of
"fail-fast" is better, than silently ignoring the problem since a
workaround should be simple as mixer removing, which is not used anyway.
Note that in case of jdk9 an additional check of application's sound
code will be needed, because the order of serviceloaders will be
different, modules which contain providers come to play, etc. So I guess
this is a good time to cleanup of our code from some workarounds which
were added in jdk 1.1.*.
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.
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
--
Best regards, Sergey.
