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
--
Best regards, Sergey.
