pulse-audio patches

[Omair Majid]

On 06/14/2011 05:37 PM, Denis Lila wrote:
> Hi.
>
> I would like to (eventually) push the attached patches.
> These are not ready to go in yet, because they require
> the changeset in this e-mail
> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2011-June/014722.html
>
> Therefore, this is just a preview I'm sending out
> because I don't want to work too long without any
> reviews.
>

I have a few comments in-line below. But overall, the changes look fine.

> rev2597.patch replaces a few enums with static longs.
> This was done to make the java side more robust to
> changes in the native code or pulse audio. The longs
> allow us to initialize them (using the jni) to the
> values pulse audio uses for its own enums, so we no
> longer have to have switch statements that translate
> between java enum instances and ints.
>
> rev2598.patch removes a needless notifyAll()
> and fixes a couple of race conditions between
> read() and flush() in PulseAudioTargetDataLine.
>
>


>
> # HG changeset patch
> # User Denis Lila<dlila at redhat.com>
> # Date 1308086602 14400
> # Node ID b006f1d14da5e0254a2382e8f628a4d2b872fb90
> # Parent  25d50d852479eb2311da0deb3005fbfe9fb2ee15
> Concurrency problems and improvements.
>

... snip ...

>
> icedtea6_rev2597.patch
>

This patch looks fine to me.

>
> # HG changeset patch
> # User Denis Lila<dlila at redhat.com>
> # Date 1308081211 14400
> # Node ID 25d50d852479eb2311da0deb3005fbfe9fb2ee15
> # Parent  7e19bc7875bb8265f3fd1ca69cc89a40eaad664b
> Use longs instead of enums for pulse audio enums.
>
> diff -r 7e19bc7875bb -r 25d50d852479 pulseaudio/src/java/org/classpath/icedtea/pulseaudio/ContextEvent.java
> --- a/pulseaudio/src/java/org/classpath/icedtea/pulseaudio/ContextEvent.java	Mon Jun 13 12:02:38 2011 -0400
> +++ b/pulseaudio/src/java/org/classpath/icedtea/pulseaudio/ContextEvent.java	Tue Jun 14 15:53:31 2011 -0400
> @@ -37,32 +37,64 @@
>
>   package org.classpath.icedtea.pulseaudio;
>
> +import java.util.Arrays;
> +
>   /**
>    * This class encapsulates a change in the PulseAudio's connection context.
>    *
>    * When this event is fired, something has happened to the connection of this
>    * program to the PulseAudio server.
> - *
>    */
> -
> +// TODO: maybe make this class an enum with a field that indicates the kind of
> +// event?
>   class ContextEvent {
> -
> +    // There are certain enumerations from pulse audio that we need to use in
> +    // the java side. For all of these, we declare static longs in the proper
> +    // java classes and we use static native methods to initialize them to
> +    // the values used by pulse audio. This makes us immune to changes in
> +    // the integer values of the enum symbols in pulse audio.
>       /**
>        *  Basically, what is the new state of the context
> -     *
> +     *  These will be initialized to the proper values in the JNI.
>        */
> -    public static enum Type {
> -        UNCONNECTED, CONNECTING, AUTHORIZING, SETTING_NAME, READY, FAILED, TERMINATED
> +    static long UNCONNECTED  = -1,
> +                CONNECTING   = -1,
> +                AUTHORIZING  = -1,
> +                SETTING_NAME = -1,
> +                READY        = -1,
> +                FAILED       = -1,
> +                TERMINATED   = -1;
> +
> +    private static native void init_constants();
> +
> +    static {
> +        SecurityWrapper.loadNativeLibrary();
> +        init_constants();
>       }
>
> -    private Type type;
> -
> -    public ContextEvent(Type type) {
> -        this.type = type;
> +    // Throws an IllegalStateException if value is not one of the above
> +    // context events. We do this for all pulse audio enumerations that
> +    // we need to use in the java side. If pulse audio decides to add
> +    // new events/states, we need to add them to the classes. The exception
> +    // will let us know.
> +    // return the input if there is no error

Perhaps you should make this a javadoc?

> diff -r 7e19bc7875bb -r 25d50d852479 pulseaudio/src/java/org/classpath/icedtea/pulseaudio/PulseAudioMixer.java
> --- a/pulseaudio/src/java/org/classpath/icedtea/pulseaudio/PulseAudioMixer.java	Mon Jun 13 12:02:38 2011 -0400
> +++ b/pulseaudio/src/java/org/classpath/icedtea/pulseaudio/PulseAudioMixer.java	Tue Jun 14 15:53:31 2011 -0400
> @@ -701,7 +701,7 @@
>           try {
>               // System.out.println("waiting...");
>               ready.acquire();
> -            if (eventLoop.getStatus() != 4) {
> +            if (eventLoop.getStatus() != ContextEvent.READY) {

Wow, I cant believe I wrote this line. Thanks for fixing it!

> diff -r 7e19bc7875bb -r 25d50d852479 pulseaudio/src/native/jni-common.h
> --- a/pulseaudio/src/native/jni-common.h	Mon Jun 13 12:02:38 2011 -0400
> +++ b/pulseaudio/src/native/jni-common.h	Tue Jun 14 15:53:31 2011 -0400
> @@ -39,11 +39,20 @@
>   #define _JNI_COMMON_H
>
>   #include<jni.h>
> +#include<stdio.h>

Is this needed?

> diff -r 7e19bc7875bb -r 25d50d852479 pulseaudio/src/native/org_classpath_icedtea_pulseaudio_Stream.c
> --- a/pulseaudio/src/native/org_classpath_icedtea_pulseaudio_Stream.c	Mon Jun 13 12:02:38 2011 -0400
> +++ b/pulseaudio/src/native/org_classpath_icedtea_pulseaudio_Stream.c	Tue Jun 14 15:53:31 2011 -0400
> @@ -131,6 +131,7 @@
>       assert(context->env);
>       assert(context->obj);
>
> +    PA_SAMPLE_ALAW;

Stray line?

>       if (pa_stream_get_state(stream) == PA_STREAM_CREATING) {
>           callJavaVoidMethod(context->env, context->obj, "overflowCallback");
>       } else {
> @@ -240,6 +241,23 @@
>
>   }
>
> +#define SET_STREAM_STATE(env, clz, state_name) \
> +    SET_JAVA_STATIC_LONG_FIELD_TO_PA_ENUM(env, clz, STREAM, state_name)
> +

I think mirroring the other names, as in SET_STREAM_STATE_ENUM, might 
make the intent of this macro more clear.

Everything else looks fine.

Cheers,
Omair