[RFC][icedtea6-pulse-audio]: Start recording corked.

[Omair Majid]

On 06/16/2011 01:34 PM, Denis Lila wrote:
> Hi.
>
> This patch makes only one functional change:
> it starts recording streams corked (paused).
> We used to start them uncorked on a TargetDataLine.open(...)
> call, which was a waste because because a line
> cannot be read until TargetDataLine.start() is called
> on it. Therefore the start() corresponds to
> uncorking the stream (which is reflected in
> our implementation of start(), which just uncorks
> the already uncorked stream).
>
> Other than this, the patch also moves pa_stream_flag_t
> constants into java to give the java side control
> over how streams are started. The implementation of
> native_pa_stream_connect_playback no longer starts
> the stream corked explicitly. Rather, it just passes
> in the "flags" argument it received from its java
> caller (which has set it to CORKED). These changes
> are in line with the design of Stream.c, which has
> very little logic, and it's mostly just a way for
> the java code to make calls into pulse audio.
>
> ChangeLog:
> 2011-06-16  Denis Lila<dlila at redhat.com>
>
>      * pulseaudio/src/java/org/classpath/icedtea/pulseaudio/Stream.java
>      (NOFLAGS, START_CORKED, INTERPOLATE_TIMING, NOT_MONOTONIC,
>      AUTO_TIMING_UPDATE, NO_REMAP_CHANNELS, NO_REMIX_CHANNELS,
>      FIX_FORMAT, FIX_RATE, FIX_CHANNELS, DONT_MOVE, VARIABLE_RATE,
>      PEAK_DETECT, START_MUTED, ADJUST_LATENCY, EARLY_REQUESTS,
>      DONT_INHIBIT_AUTO_SUSPEND, START_UNMUTED, FAIL_ON_SUSPEND):
>      New static long variables mirroring pa_stream_flag_t values.
>      (native_pa_stream_connect_playback, native_pa_stream_connect_record):
>      Change flags parameter to long.
>      (connectForPlayback, connectForRecording): Start the stream corked.
>      Change formatting to make it more readable.
>      * pulseaudio/src/native/org_classpath_icedtea_pulseaudio_Stream.c
>      (SET_STREAM_ENUM): Renamed from SET_STREAM_STATE_ENUM, since the
>      macro could have been used for any PA_STREAM constants, not just
>      stream states (and indeed, we now use it for flag constants too).
>      (Java_org_classpath_icedtea_pulseaudio_Stream_init_1constants):
>      Initialize flag constants in addition to the stream states.
>      (Java_org_classpath_icedtea_pulseaudio_Stream_native_1pa_1stream_1connect_1playback):
>      Change flags parameter to jlong (from jint), remove commented out
>      dead code, remove obsolete comment, and start the stream with whatever
>      flags were passed in the flags parameter, instead of ignoring that
>      parameter and using PA_STREAM_START_CORKED.
>      (Java_org_classpath_icedtea_pulseaudio_Stream_native_1pa_1stream_1connect_1record):
>      Change flags parameter to jlong (from jint), remove commented out
>      dead code.
>
>

Comments inline, below.

>
> diff -r 2eef438960e4 pulseaudio/src/java/org/classpath/icedtea/pulseaudio/Stream.java
> --- a/pulseaudio/src/java/org/classpath/icedtea/pulseaudio/Stream.java	Thu Jun 16 11:11:35 2011 -0400
> +++ b/pulseaudio/src/java/org/classpath/icedtea/pulseaudio/Stream.java	Thu Jun 16 12:56:32 2011 -0400
> @@ -101,15 +101,12 @@
>
>       // see comments in ContextEvent.java and Operation.java
>       // These are the possible stream states.
> -    // TODO: perhaps we should do this for stream flags too.
>       public static long UNCONNECTED = -1,
>                          CREATING    = -1,
>                          READY       = -1,
>                          FAILED      = -1,
>                          TERMINATED  = -1;
>
> -    private static native void init_constants();
> -
>       // Throw an IllegalStateException if value is not one of the possible
>       // states. Otherwise return the input.
>       public static long checkNativeStreamState(long value) {
> @@ -120,6 +117,29 @@
>           return value;
>       }
>
> +    // These are stream flags.
> +    public static long NOFLAGS                   = -1,
> +                       START_CORKED              = -1,
> +                       INTERPOLATE_TIMING        = -1,
> +                       NOT_MONOTONIC             = -1,
> +                       AUTO_TIMING_UPDATE        = -1,
> +                       NO_REMAP_CHANNELS         = -1,
> +                       NO_REMIX_CHANNELS         = -1,
> +                       FIX_FORMAT                = -1,
> +                       FIX_RATE                  = -1,
> +                       FIX_CHANNELS              = -1,
> +                       DONT_MOVE                 = -1,
> +                       VARIABLE_RATE             = -1,
> +                       PEAK_DETECT               = -1,
> +                       START_MUTED               = -1,
> +                       ADJUST_LATENCY            = -1,
> +                       EARLY_REQUESTS            = -1,
> +                       DONT_INHIBIT_AUTO_SUSPEND = -1,
> +                       START_UNMUTED             = -1,
> +                       FAIL_ON_SUSPEND           = -1;
> +

We now have two sets of constants for different purposes. Have you 
thought about prefixing them to indicate if they are stream flags or 
stream states?

> diff -r 2eef438960e4 pulseaudio/src/native/org_classpath_icedtea_pulseaudio_Stream.c
> --- a/pulseaudio/src/native/org_classpath_icedtea_pulseaudio_Stream.c	Thu Jun 16 11:11:35 2011 -0400
> +++ b/pulseaudio/src/native/org_classpath_icedtea_pulseaudio_Stream.c	Thu Jun 16 12:56:32 2011 -0400
> @@ -453,14 +475,6 @@
>       buffer_attr.prebuf = (uint32_t) bufferPreBuffering;
>       buffer_attr.minreq = (uint32_t) bufferMinimumRequest;
>
> -    /*
> -     printf("buffer maxlength: %u\n", buffer_attr.maxlength);
> -     printf("buffer tlength: %u\n", buffer_attr.tlength);
> -     printf("buffer prebuf: %u\n", buffer_attr.prebuf);
> -     printf("buffer minreq: %u\n", buffer_attr.minreq);
> -     printf("buffer fragsize: %u\n", buffer_attr.fragsize);
> -     */
> -

Any reason you are removing this?

>       const char* dev = NULL;
>       if (device != NULL) {
>           dev = (*env)->GetStringUTFChars(env, device, NULL);
> @@ -468,10 +482,9 @@
>               return -1; // oome thrown
>           }
>       }
> -    /* Set flags to 0 to fix problem with draining before calling start, might need to
> -     be changed back to PA_STREAM_START_CORKED in the future, if we'll be able to implement
> -     synchronization*/

Ah, so someone had set flags to 0 instead of PA_STREAM_START_CORKED 
explicitly. Did you test if this is still a problem?

> -    int value = pa_stream_connect_playback(stream, dev,&buffer_attr, PA_STREAM_START_CORKED, NULL, sync_stream);
> +
> +    int value = pa_stream_connect_playback(stream, dev,&buffer_attr,
> +            (pa_stream_flags_t) flags, NULL, sync_stream);
>

Everything else looks fine.

Cheers,
Omair