<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Sergey,<br>
<br>
> But if we discuss in this way we can get an assumption that any
<br>
> methods of plugins can throw any type of exceptions and we <br>
> should wrap them in "catch (Throwable)".<br>
<br>
That's not true, because if a plugin throws e.g. "RuntimeException",
AudioSystem would have always passed it on. So continuing to pass it
on is OK.<br>
<br>
I agree that the code would be cleaner without that catch clause.<br>
<br>
But I don't agree with "bug to bug compatiblity" and "fail fast":
IMHO, backwards compatibility is much more important. The only
exception is if you cannot align an important bug fix with 100%
backwards compatibility. But removing this "catch NPE clause" is not
an important bug fix.<br>
<br>
Of course, this is a rather unimportant case. Chances are that there
does not exist a broken MixerProvider out in the wild. But the
discussion is worthwhile for setting the priorities for other bug
fixes with similar trade-offs.<br>
<br>
Arguing that users can and should just remove the failing plugin
ignores many other deployment possibilities of Java code. For
example, code can be deployed centrally in binary form, and users
don't have a choice of adding, updating, or removing components on
their own. Another example is where users are stuck with binary code
which they need but cannot change anymore. For the user, knowing
that a plugin is broken, is not of much help when she cannot fix the
plugin... Also, of course, it is possible that a MixerProvider only
throws NPE's under certain conditions and works fine under other
conditions. So removing that MixerProvider would remove its
legitimate function.<br>
<br>
In my opinion, updating to a new JRE should never break existing
applications -- if at all possible. Of course, this is my view. I
don't know what Oracle's current policy is on this. And I don't know
what other incompatibilities JDK 9 will bring. But it does look to
me that existing service providers will still be supported.<br>
<br>
Thanks,<br>
Florian<br>
<br>
<br>
<blockquote cite="mid:562E4028.2080703@oracle.com" type="cite">
<blockquote type="cite">
<br>
Removing the NPE catch clause, however, will still cause a
backwards
<br>
incompatibility, because if a poorly programmed MixerProvider
gets
<br>
installed which throws NPE for whatever reason (might also
happen when
<br>
"info" is non-null), now AudioSystem.getMixer() will throw NPE,
where
<br>
it previously worked.
<br>
</blockquote>
<br>
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.
<br>
<br>
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.*.
<br>
<br>
<blockquote type="cite">
<br>
I agree that it's harder for debugging mixer providers if NPE is
<br>
ignored. Other than that, I don't see any problem with keeping
the NPE
<br>
catch for backwards compatibility's sake. Even if just
theoretical...
<br>
But you never now, companies might be using poorly programmed
in-house
<br>
software or the like.
<br>
</blockquote>
<br>
<blockquote type="cite">Hi, Florian.
<br>
Thanks for review! see my comments inline.
<br>
<br>
<blockquote type="cite">This is true for the included
MixerProvders, but the requirement that
<br>
null will return the default Mixer is just made official. We
should,
<br>
however, remain backwards compatible with 3rd party
MixerProviders by
<br>
keeping that second loop. The old style is that a
MixerProvider
<br>
returns its default Mixer as first element. Also, for
backwards
<br>
compatibility, I'd also keep the catch clause for NPE in both
loops.
<br>
</blockquote>
<br>
This is tricky place, I have some thoughts about this, which I
would
<br>
like to discuss.
<br>
<br>
I studied the history to find an answer why the catch of NPE and
the
<br>
second loop were added.
<br>
<br>
- The catch of the null was added in the 1999 because of "added
a catch
<br>
for an NPE -- Netscape tends to throw this if some strings
haven't been
<br>
set in our device provider info objects".
<br>
It is actually a workaround. Because of this patch we did not
catch some
<br>
bugs in our MixerProviders. For example PortMixerProvider,
<br>
SimpleInputDeviceProvider(old mixer) were thrown NPE, when they
tried to
<br>
throw IllegalArgumentException. In the same moment the
<br>
DirectAudioDeviceProvider, HeadspaceMixerProvider(old mixer),
<br>
SoftMixingMixerProvider are ready for null. I also checked the
<br>
TMixerProvider from tritonus it also ready for null.
<br>
<br>
<br>
- The second loop was added in "JDK-4834461: Linux: Applet hang
when you
<br>
load it during sound card is in use". This is also a workaround
for a
<br>
situation when we try to get a default mixer in the first loop
but it
<br>
was not available for some reason, in this case we will return
first
<br>
mixer from the first mixer provider. But the second loop will be
run
<br>
only if the first loop will not find the default mixer in some
other
<br>
providers.
<br>
<br>
So imagine this situation when some old 3rd party MixerProvider
is used:
<br>
- The user sets some provider, which throw the NPE on null in
getMixer();
<br>
- The first loop call this provider and skip it in catch block
<br>
- The next bundled provider will be used instead of user's
mixer, the
<br>
second loop is not executed.
<br>
- If we use this approach the user will not be able to check
that wrong
<br>
provider is in use, and in case of NPE the temporary workaround
will be
<br>
- removing of custom mixer provider which are not used anyway.
<br>
<br>
Note that in jdk9 the "META-INF/services" will not be used, so
there
<br>
will be no option to remove bundled providers via configs, and
we will
<br>
always iterate over the bundled providers in the first loop.
<br>
<br>
For the case of some other vendors of jdk, after the moment of
<br>
specification clarification all default providers should not
contradict
<br>
the specification, and should not throw NPE in getMixer(), but
return
<br>
default mixer.
<br>
<br>
Does it make sense?
<br>
<br>
<blockquote type="cite">
<br>
<br>
[1] <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-4834461">https://bugs.openjdk.java.net/browse/JDK-4834461</a>
<br>
<br>
<br>
On 08.10.15 11:46, Florian Bomers wrote:
<br>
<blockquote type="cite">
<blockquote type="cite">For me the most logical is to return
default playback mixer :)
<br>
</blockquote>
<br>
yes, at the time it was most important to provide an easy
way to get a
<br>
playback device.
<br>
<br>
Can you send a new webrev?
<br>
Thanks,
<br>
Florian
<br>
<br>
<br>
On 25.09.2015 20:33, alexey menkov wrote:
<br>
<blockquote type="cite">
<br>
<br>
On 25.09.2015 20:42, Sergey Bylokhov wrote:
<br>
<blockquote type="cite">On 25.09.15 20:32, alexey menkov
wrote:
<br>
<blockquote type="cite">Ok, lets only clarify
MixerProvider.getMixer(null) behavior.
<br>
Actually AusioSystem.getMixer(null) looks unclear
because unclear
<br>
what
<br>
is the "default mixer" in the case (we may have
different "default
<br>
playback mixer", "default recording mixer", "default
port mixer").
<br>
</blockquote>
<br>
Right, if to consider that the sequence of providers
isn't specified,
<br>
then it is unclear what this method should actually
return. I do not
<br>
understand the purpose to return some random mixer. I
think that
<br>
intention was to return "default playback mixer"?
<br>
</blockquote>
<br>
I don't know. I suppose this is ancient method (and most
likely nobody
<br>
use it) and the implementation was changed this way to get
<br>
DirectAudioDevice as default (it supports both playback
and recording).
<br>
For me the most logical is to return default playback
mixer :)
<br>
<br>
--alex
<br>
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">
<br>
regards
<br>
Alex
<br>
<br>
On 25.09.2015 18:41, Sergey Bylokhov wrote:
<br>
<blockquote type="cite">Hi, Alexey.
<br>
Thanks for review! see my comments inline.
<br>
<br>
On 25.09.15 18:23, alexey menkov wrote:
<br>
<blockquote type="cite">Hi Sergey,
<br>
<br>
Overall looks good, but I don't like change in
<br>
MixerProvider.getMixer
<br>
and PortMixerProvider.getMixer.
<br>
<br>
MixerProvider.getMixer(null) is used by
<br>
AudioSystem.getMixer(null) (to
<br>
get the default mixer), but provide PortMixer as
the default does
<br>
not
<br>
look good, I'd expect AudioSystem.getMixer(null)
returns some
<br>
playback-able device. (Note also that for Ports
"SourceDataLine"
<br>
means
<br>
controls for recording device)
<br>
</blockquote>
<br>
<blockquote type="cite">
<br>
Also comment for
MixerProvider.getMixer(Mixer.Info):
<br>
> * @param info an info object that
describes the desired mixer,
<br>
> * or {@code null} for the system
default mixer
<br>
<br>
is unclear and is not consistent with the
description above:
<br>
> * The full set of the mixer info objects
that represent the
<br>
mixers
<br>
> * supported by this {@code MixerProvider}
may be obtained
<br>
through
<br>
the
<br>
> * {@code getMixerInfo} method. Use the
{@code isMixerSupported}
<br>
method to
<br>
> * test whether this {@code MixerProvider}
supports a particular
<br>
mixer.
<br>
It looks like MixerProvider.getMixerInfo should
add "null" to the
<br>
supported mixers and
MixerProvider.isMixerSupported(null) should
<br>
return
<br>
"true".
<br>
</blockquote>
<br>
In this case the null means that some default mixer
will be
<br>
returned. I
<br>
am not sure that isMixerSupported(null) should
return true,
<br>
instead I
<br>
can clarify the specification of getMixer(null), and
mention that if
<br>
null is provided then this mixer will try to return
some default(
<br>
supported) mixer if possible, otherwise
IllegalArgumentException
<br>
will be
<br>
thrown.
<br>
<br>
For the case of PortMixerProvider and "null" I can
throw a
<br>
IllegalArgumentException which will mean that this
provider do not
<br>
have
<br>
"default" mixer.
<br>
<br>
<blockquote type="cite">
<br>
For now I don't have a proposal how to fix this.
<br>
Maybe it would be better to fix behavior of
<br>
AudioSystem.getMixer(null) -
<br>
now it ignores "sound.properties" file (see
AudioSystem spec for
<br>
explanations).
<br>
<br>
regards
<br>
Alex
<br>
<br>
On 14.09.2015 16:29, Sergey Bylokhov wrote:
<br>
<blockquote type="cite">Hello Audio Guru.
<br>
<br>
Please review the fix for jdk9. This issue is a
subtask of:
<br>
4912693: Behavior of null arguments not
specified in Java Sound
<br>
<br>
In this patch I cover the whole
javax.sound.sampled.spi package.
<br>
<br>
The small description of the fix:
<br>
- I have checked all methods in the spi package
and all related
<br>
methods
<br>
in AudioSystem class.
<br>
- I have moved related tests to the folder
corresponding the
<br>
package
<br>
and
<br>
class name.
<br>
- I have written a tests for every method and
class which I
<br>
changed.
<br>
Note that these classes related to the different
service
<br>
providers,
<br>
so I
<br>
have covered all installed implementations of
each provider.
<br>
<br>
<br>
Long description.
<br>
I splits the fix to 3 use cases:
<br>
- If the method always throw a NPE, then I
simply update a
<br>
javadoc and
<br>
write a small test.
<br>
- If the method most of the time throw a NPE
then I update a
<br>
javadoc
<br>
and
<br>
change the method to always throw a NPE. Also I
write a test which
<br>
tries
<br>
to emulate both cases when NPE was thrown and
when not. For
<br>
example
<br>
AudioFileWriter.isFileTypeSupported(Type) always
throws a NPE
<br>
if at
<br>
least one type is supported, but if the array is
empty then
<br>
false is
<br>
returned.
<br>
- If the method have a few parameters and throw
a NPE for some
<br>
set of
<br>
them. For example AudioFloatFormatConverter.
<br>
isConversionSupported(Encoding,AudioFormat), the
appropriate test
<br>
tries
<br>
to cover these cases.
<br>
<br>
It turned out that all methods throw a NPE
except of one:
<br>
AudioSystem.getMixer()(MixerProvider.getMixer()),
but it was found
<br>
that
<br>
the specification of MixerProvider.getMixer has
no information
<br>
about
<br>
the
<br>
null, so I copied it from the
AudioSystem.getMixer(). Also one
<br>
implementation of MixerProvider -
PortMixerProvider.getMixer()
<br>
throws
<br>
NPE, so updated its implementation to the same
as
<br>
DirectAudioDeviceProvider.getMixer();
<br>
<br>
I have done all related regression/jck/sqe
tests, and I found one
<br>
issue
<br>
in jck and regression tests. Both are related to
JDK-4941629 [1]
<br>
(see
<br>
comments in this CR). The jck test assumes that
the method
<br>
AudioSystem.write(ais, null, stream) should
throw
<br>
IllegalArgumentException. But according to
specification it should
<br>
throw
<br>
IllegalArgumentException if the type is
unsupported, but the
<br>
related
<br>
method AudioSystem.isFileTypeSupported(Type)
will always throw
<br>
a NPE
<br>
for null. I prefer to file a bug against jck for
this case.
<br>
<br>
<br>
[1]
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-4941629">https://bugs.openjdk.java.net/browse/JDK-4941629</a>
<br>
<br>
Bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8135100">https://bugs.openjdk.java.net/browse/JDK-8135100</a>
<br>
The new test:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~serb/8135100/webrev.01">http://cr.openjdk.java.net/~serb/8135100/webrev.01</a>
<br>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
<br>
<br>
<pre class="moz-signature" cols="70">--
Florian Bomers
Bome Software
everything sounds.
<a class="moz-txt-link-freetext" href="http://www.bome.com">http://www.bome.com</a>
__________________________________________________________________
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
</pre>
</blockquote>
</body>
</html>