Closed Bug 1221576 Opened 9 years ago Closed 8 years ago

Write a full-duplex PulseAudio cubeb backend

Categories

(Core :: Audio/Video: cubeb, defect, P1)

Product:
Core
Component:
Audio/Video: cubeb
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46

People

(Reporter: padenot, Assigned: achronop)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

cubeb-pulse-fd-updated3.patch
45.57 KB, patch
padenot
: review+
Details | Diff | Splinter Review
Full-duplex cubeb changes for gecko
80.19 KB, patch
Details | Diff | Splinter Review 
      No description provided.
Component: Audio/Video: MediaStreamGraph → Audio/Video: cubeb
Assignee: nobody → achronop
The progress is uploaded in the following branch. This is the first implementation with basic scenario working.

https://github.com/achronop/cubeb/tree/full-duplex-pulse
Rank: 15
Priority: -- → P1
This is the patch from cubeb git repo. It can be found on github: https://github.com/achronop/cubeb/tree/full-duplex-pulse
Attachment #8703676 - Flags: review?(padenot)
Comment on attachment 8703676 [details] [diff] [review]
Cubeb pulse implementation for full duplex

Review of attachment 8703676 [details] [diff] [review]:
-----------------------------------------------------------------

I have not yet ran this, but here are some comments.

::: include/cubeb/cubeb.h
@@ +322,5 @@
>  int cubeb_stream_init(cubeb * context,
>                        cubeb_stream ** stream,
>                        char const * stream_name,
> +                      cubeb_stream_params* input_stream_params,
> +                      cubeb_stream_params* output_stream_params,

nit: spaces around *.

::: src/cubeb-internal.h
@@ +23,4 @@
>                              cubeb_device_collection ** collection);
>    void (* destroy)(cubeb * context);
>    int (* stream_init)(cubeb * context, cubeb_stream ** stream, char const * stream_name,
> +                      cubeb_stream_params * input_stream_params, cubeb_stream_params * output_stream_params, unsigned int latency,

nit: hard wrap this.

::: src/cubeb_alsa.c
@@ +86,5 @@
>    snd_pcm_uframes_t last_position;
>    snd_pcm_uframes_t buffer_size;
>    snd_pcm_uframes_t period_size;
> +  cubeb_stream_params input_params;
> +  cubeb_stream_params output_params;

How are we going to know whether a particular backend supports full-duplex or not ?

::: src/cubeb_pulse.c
@@ +188,4 @@
>  }
>  
>  static void
> +write_to_output(pa_stream * s, void* input_data, size_t nbytes, cubeb_stream * stm)

We need a better name for this, since it also passes the input data.

@@ +216,5 @@
>        stm->shutdown = 1;
>        return;
>      }
> +    // If more iterations move offset of read buffer
> +    if (input_data){

nit: space between ) and {.

@@ +292,5 @@
> +  }
> +}
> +
> +static void
> +stream_read_callback(pa_stream * s, size_t nbytes, void * u)

Why have you decided to put everything in the read callback instead of the write callback ? I'm just curious.

@@ +311,5 @@
> +    size_t read_frames = read_size / in_frame_size;
> +
> +    cubeb_stream* stm = u;
> +    if (stm->output_stream) {
> +      // input/capture + output/record operation

s/record/playback/

@@ +316,5 @@
> +      void* read_buffer = (void*)read_data;
> +      // Write directly to output
> +      size_t out_frame_size = WRAP(pa_frame_size)(&stm->output_sample_spec);
> +      size_t write_size = read_frames * out_frame_size;
> +      // Write input buffer directly to output

Clean up the comments here, I don't think we're writing the input buffer to the output.

@@ +323,5 @@
> +      // input/capture only operation. Call callback directly
> +      void* read_buffer = (void*)read_data;
> +      if (stm->data_callback(stm, stm->user_ptr, read_buffer, NULL, read_frames) < 0) {
> +        WRAP(pa_stream_cancel_write)(s);
> +        stm->shutdown = 1;

Why do we need those two lines ?

@@ +327,5 @@
> +        stm->shutdown = 1;
> +        break;
> +      }
> +    }
> +    WRAP(pa_stream_drop)(s);

Why do we need this?

@@ +435,5 @@
> +      r = operation_wait(stm->context, stm->output_stream, o);
> +      WRAP(pa_operation_unref)(o);
> +    }
> +    // if this fail abort
> +    if (r!=0){

Spaces around !=.

@@ +694,5 @@
> +
> +    // update buffer attributes
> +    battr.tlength = WRAP(pa_usec_to_bytes)(latency * PA_USEC_PER_MSEC, &stm->output_sample_spec);
> +    battr.minreq = battr.tlength / 4;
> +    battr.fragsize = battr.minreq;

Maybe we should consider lowering the latency even more.

@@ +717,4 @@
>    }
> +
> +  // Set up input stream
> +  if (input_stream_params){

nit: space between ) and { (there are other occurrences of this in this patch).

@@ +723,5 @@
> +    in_ss.rate = input_stream_params->rate;
> +    in_ss.format = cube_to_pulse_format(input_stream_params->format);
> +    stm->input_sample_spec = in_ss;
> +
> +    stm->input_stream = WRAP(pa_stream_new)(stm->context->context, "input stream", &in_ss, NULL);

We need to find a better name for this.

@@ +730,5 @@
> +      pulse_stream_destroy(stm);
> +      return CUBEB_ERROR;
> +    }
> +    WRAP(pa_stream_set_state_callback)(stm->input_stream, stream_state_callback, stm);
> +    WRAP(pa_stream_set_read_callback)(stm->input_stream, stream_read_callback, stm);

Here, we'll have a write AND a read callback if we're doing duplex. Is it necessary ? Harmful ?

@@ +738,3 @@
>                                     PA_STREAM_AUTO_TIMING_UPDATE | PA_STREAM_INTERPOLATE_TIMING |
> +                                   PA_STREAM_NOT_MONOTONIC |
> +                                   PA_STREAM_START_CORKED | PA_STREAM_ADJUST_LATENCY);

Here, we're using battr with all members set to -1 if we're only recording. Among other things, this means the input latency is set to 250ms, which is too high for us.

@@ +832,5 @@
>  
>    if (!in_thread) {
>      WRAP(pa_threaded_mainloop_lock)(stm->context->mainloop);
>    }
> +  r = WRAP(pa_stream_get_time)(stm->output_stream, &r_usec);

We could support the record position. We don't need it per se (because the MSG does not use it), but it's easy enough.

@@ +981,4 @@
>  pulse_get_state_from_sink_port(pa_sink_port_info * info)
>  {
>    if (info != NULL) {
> +#if PA_CHECK_VERSION(2, 0, 0)

We can leave this like this. This is used to know if a musician/pro audio soundcard has a jack plugged, that's a very rare case. Just file a followup.

::: src/cubeb_resampler.cpp
@@ +168,4 @@
>  {
> +	if (input_buffer) {
> +		return 0;
> +	}

This should not be necessary, Pulse does not use the resampler.

::: src/cubeb_resampler.h
@@ +53,5 @@
>   * @retval CUBEB_ERROR on error.
>   */
>  long cubeb_resampler_fill(cubeb_resampler * resampler,
> +                          void * input_buffer,
> +						  void * output_buffer, long frames_needed);

Hard wrap this.
Attachment #8703676 - Flags: review?(padenot)
> I have not yet ran this, but here are some comments.

Thanks for the comments. If you need to run it I can send you a tiny executable for the basic scenario.

> ::: src/cubeb_alsa.c
> @@ +86,5 @@
> >    snd_pcm_uframes_t last_position;
> >    snd_pcm_uframes_t buffer_size;
> >    snd_pcm_uframes_t period_size;
> > +  cubeb_stream_params input_params;
> > +  cubeb_stream_params output_params;
> 
> How are we going to know whether a particular backend supports full-duplex
> or not ?

I guess right now we don't, my opinion is that we should. If we agree to the interface, the change must be trivial. May be a good idea to open an issue in cubeb github repo.

> ::: src/cubeb_pulse.c
> @@ +188,4 @@
> >  }
> >  
> >  static void
> > +write_to_output(pa_stream * s, void* input_data, size_t nbytes, cubeb_stream * stm)
> 
> We need a better name for this, since it also passes the input data.

Rushing choice. What about write_full_duplex | full_duplex_output | full_duplex_write? I am not good in names ... 

> @@ +292,5 @@
> > +  }
> > +}
> > +
> > +static void
> > +stream_read_callback(pa_stream * s, size_t nbytes, void * u)
> 
> Why have you decided to put everything in the read callback instead of the. 
> write callback ? I'm just curious.

Write callback is a little stiff. If you do not write any data it is not repeated. So if I did it the other way around and read data was not available (this happen all the times just after init) I have to send silence in order to write something. On the other hand reading callback is repeated even if you ignore the data. 

> @@ +316,5 @@
> > +      void* read_buffer = (void*)read_data;
> > +      // Write directly to output
> > +      size_t out_frame_size = WRAP(pa_frame_size)(&stm->output_sample_spec);
> > +      size_t write_size = read_frames * out_frame_size;
> > +      // Write input buffer directly to output
> 
> Clean up the comments here, I don't think we're writing the input buffer to
> the output.

Yes we don't. What I mean is that we do not copy the data to an intermediate buffer before write it to the output. I had a version which used an intermediate buffer in some cases. These comments left from that version.
 
> @@ +323,5 @@
> > +      // input/capture only operation. Call callback directly
> > +      void* read_buffer = (void*)read_data;
> > +      if (stm->data_callback(stm, stm->user_ptr, read_buffer, NULL, read_frames) < 0) {
> > +        WRAP(pa_stream_cancel_write)(s);
> > +        stm->shutdown = 1;
> 
> Why do we need those two lines ?

We set the shutdown in order to stop looping and end the strean (since we got negative num of frames). This is a way to stop the stream from inside call back and it is there from previous version.
Method pa_stream_cancel_write is used because we called pa_stream_begin_write at the beginning of the while loop. This is required by PulseAudio doc if you call begin_write and you want to cancel it without writing anything.

> 
> @@ +327,5 @@
> > +        stm->shutdown = 1;
> > +        break;
> > +      }
> > +    }
> > +    WRAP(pa_stream_drop)(s);
> 
> Why do we need this?

It tells to Pulse that the read data is consumed and it can be removed from the  buffer in order to fetch the new data. If you do not use it the read buffer is getting larger and larger and and the read index is not updated. It's the standard flow for reading. Pulse documentation may explain it better than me. I copy here the important part. "Remove the current fragment on record streams ... Use pa_stream_drop() to actually remove the data from the buffer and move the read index forward." 

> @@ +694,5 @@
> > +
> > +    // update buffer attributes
> > +    battr.tlength = WRAP(pa_usec_to_bytes)(latency * PA_USEC_PER_MSEC, &stm->output_sample_spec);
> > +    battr.minreq = battr.tlength / 4;
> > +    battr.fragsize = battr.minreq;
> 
> Maybe we should consider lowering the latency even more.

I am not sure what you mean. Here we translate the latency to bytes in order to set the tlength. This is the only attribute affecting the latency. The other two are hints to Pulse and affect how often read/write callbacks circulate.

> @@ +723,5 @@
> > +    in_ss.rate = input_stream_params->rate;
> > +    in_ss.format = cube_to_pulse_format(input_stream_params->format);
> > +    stm->input_sample_spec = in_ss;
> > +
> > +    stm->input_stream = WRAP(pa_stream_new)(stm->context->context, "input stream", &in_ss, NULL);
> 
> We need to find a better name for this.

What about "CapturedStream" or "InputStream"?

> @@ +730,5 @@
> > +      pulse_stream_destroy(stm);
> > +      return CUBEB_ERROR;
> > +    }
> > +    WRAP(pa_stream_set_state_callback)(stm->input_stream, stream_state_callback, stm);
> > +    WRAP(pa_stream_set_read_callback)(stm->input_stream, stream_read_callback, stm);
> 
> Here, we'll have a write AND a read callback if we're doing duplex. Is it
> necessary ? Harmful ?

It is necessary in order to be able to read and write. In FD case we do not use write callback since we write data from read callback(see above). But even then wtite cb must also spin.

> @@ +738,3 @@
> >                                     PA_STREAM_AUTO_TIMING_UPDATE | PA_STREAM_INTERPOLATE_TIMING |
> > +                                   PA_STREAM_NOT_MONOTONIC |
> > +                                   PA_STREAM_START_CORKED | PA_STREAM_ADJUST_LATENCY);
> 
> Here, we're using battr with all members set to -1 if we're only recording.
> Among other things, this means the input latency is set to 250ms, which is
> too high for us.

You are right, nice catch. We need to update the the buffer attributes like we do it in the output.

> @@ +832,5 @@
> >  
> >    if (!in_thread) {
> >      WRAP(pa_threaded_mainloop_lock)(stm->context->mainloop);
> >    }
> > +  r = WRAP(pa_stream_get_time)(stm->output_stream, &r_usec);
> 
> We could support the record position. We don't need it per se (because the
> MSG does not use it), but it's easy enough.

Yes we can. We need another interface change though.

> 
> @@ +981,4 @@
> >  pulse_get_state_from_sink_port(pa_sink_port_info * info)
> >  {
> >    if (info != NULL) {
> > +#if PA_CHECK_VERSION(2, 0, 0)
> 
> We can leave this like this. This is used to know if a musician/pro audio
> soundcard has a jack plugged, that's a very rare case. Just file a followup.
 
You mean we can or cannot? My intention for that is to support systems that do not have an updated version of PulseAudio. The data member pa_source/sink_port_info::available is introduces in version 2. Even try testbeds use Pulse version < 2 and we would not be able to try-test without it.

For the rest of the comments the source have been updated.
Patch updated with review comments from padenot. Also pushed on github repo dev branch
Attachment #8703676 - Attachment is obsolete: true
(In reply to Alex Chronopoulos [:achronop] from comment #4)
> > ::: src/cubeb_pulse.c
> > @@ +188,4 @@
> > >  }
> > >  
> > >  static void
> > > +write_to_output(pa_stream * s, void* input_data, size_t nbytes, cubeb_stream * stm)
> > 
> > We need a better name for this, since it also passes the input data.
> 
> Rushing choice. What about write_full_duplex | full_duplex_output |
> full_duplex_write? I am not good in names ... 

Maybe something that mentions the fact that it calls the user callback ?

> 
> > @@ +292,5 @@
> > > +  }
> > > +}
> > > +
> > > +static void
> > > +stream_read_callback(pa_stream * s, size_t nbytes, void * u)
> > 
> > Why have you decided to put everything in the read callback instead of the. 
> > write callback ? I'm just curious.
> 
> Write callback is a little stiff. If you do not write any data it is not
> repeated. So if I did it the other way around and read data was not
> available (this happen all the times just after init) I have to send silence
> in order to write something. On the other hand reading callback is repeated
> even if you ignore the data. 

Ok.

> 
> > @@ +316,5 @@
> > > +      void* read_buffer = (void*)read_data;
> > > +      // Write directly to output
> > > +      size_t out_frame_size = WRAP(pa_frame_size)(&stm->output_sample_spec);
> > > +      size_t write_size = read_frames * out_frame_size;
> > > +      // Write input buffer directly to output
> > 
> > Clean up the comments here, I don't think we're writing the input buffer to
> > the output.
> 
> Yes we don't. What I mean is that we do not copy the data to an intermediate
> buffer before write it to the output. I had a version which used an
> intermediate buffer in some cases. These comments left from that version.
>  
> > @@ +323,5 @@
> > > +      // input/capture only operation. Call callback directly
> > > +      void* read_buffer = (void*)read_data;
> > > +      if (stm->data_callback(stm, stm->user_ptr, read_buffer, NULL, read_frames) < 0) {
> > > +        WRAP(pa_stream_cancel_write)(s);
> > > +        stm->shutdown = 1;
> > 
> > Why do we need those two lines ?
> 
> We set the shutdown in order to stop looping and end the strean (since we
> got negative num of frames). This is a way to stop the stream from inside
> call back and it is there from previous version.
> Method pa_stream_cancel_write is used because we called
> pa_stream_begin_write at the beginning of the while loop. This is required
> by PulseAudio doc if you call begin_write and you want to cancel it without
> writing anything.

Thanks.

> 
> > 
> > @@ +327,5 @@
> > > +        stm->shutdown = 1;
> > > +        break;
> > > +      }
> > > +    }
> > > +    WRAP(pa_stream_drop)(s);
> > 
> > Why do we need this?
> 
> It tells to Pulse that the read data is consumed and it can be removed from
> the  buffer in order to fetch the new data. If you do not use it the read
> buffer is getting larger and larger and and the read index is not updated.
> It's the standard flow for reading. Pulse documentation may explain it
> better than me. I copy here the important part. "Remove the current fragment
> on record streams ... Use pa_stream_drop() to actually remove the data from
> the buffer and move the read index forward." 

Thanks, it all makes sense now.

> 
> > @@ +694,5 @@
> > > +
> > > +    // update buffer attributes
> > > +    battr.tlength = WRAP(pa_usec_to_bytes)(latency * PA_USEC_PER_MSEC, &stm->output_sample_spec);
> > > +    battr.minreq = battr.tlength / 4;
> > > +    battr.fragsize = battr.minreq;
> > 
> > Maybe we should consider lowering the latency even more.
> 
> I am not sure what you mean. Here we translate the latency to bytes in order
> to set the tlength. This is the only attribute affecting the latency. The
> other two are hints to Pulse and affect how often read/write callbacks
> circulate.

For now, Pulse latency is fixed to 40ms (see the latency method). From my testing, we can go lower, we should try it.

> 
> > @@ +723,5 @@
> > > +    in_ss.rate = input_stream_params->rate;
> > > +    in_ss.format = cube_to_pulse_format(input_stream_params->format);
> > > +    stm->input_sample_spec = in_ss;
> > > +
> > > +    stm->input_stream = WRAP(pa_stream_new)(stm->context->context, "input stream", &in_ss, NULL);
> > 
> > We need to find a better name for this.
> 
> What about "CapturedStream" or "InputStream"?

Put "cubeb input" for now, there is another bug on updating the names.

> 
> > @@ +730,5 @@
> > > +      pulse_stream_destroy(stm);
> > > +      return CUBEB_ERROR;
> > > +    }
> > > +    WRAP(pa_stream_set_state_callback)(stm->input_stream, stream_state_callback, stm);
> > > +    WRAP(pa_stream_set_read_callback)(stm->input_stream, stream_read_callback, stm);
> > 
> > Here, we'll have a write AND a read callback if we're doing duplex. Is it
> > necessary ? Harmful ?
> 
> It is necessary in order to be able to read and write. In FD case we do not
> use write callback since we write data from read callback(see above). But
> even then wtite cb must also spin.

Ok, it's the same with WASAPI, I just wanted to make sure.

> > @@ +738,3 @@
> > >                                     PA_STREAM_AUTO_TIMING_UPDATE | PA_STREAM_INTERPOLATE_TIMING |
> > > +                                   PA_STREAM_NOT_MONOTONIC |
> > > +                                   PA_STREAM_START_CORKED | PA_STREAM_ADJUST_LATENCY);
> > 
> > Here, we're using battr with all members set to -1 if we're only recording.
> > Among other things, this means the input latency is set to 250ms, which is
> > too high for us.
> 
> You are right, nice catch. We need to update the the buffer attributes like
> we do it in the output.
> 
> > @@ +832,5 @@
> > >  
> > >    if (!in_thread) {
> > >      WRAP(pa_threaded_mainloop_lock)(stm->context->mainloop);
> > >    }
> > > +  r = WRAP(pa_stream_get_time)(stm->output_stream, &r_usec);
> > 
> > We could support the record position. We don't need it per se (because the
> > MSG does not use it), but it's easy enough.
> 
> Yes we can. We need another interface change though.

I would not bother with that right now.

> 
> > 
> > @@ +981,4 @@
> > >  pulse_get_state_from_sink_port(pa_sink_port_info * info)
> > >  {
> > >    if (info != NULL) {
> > > +#if PA_CHECK_VERSION(2, 0, 0)
> > 
> > We can leave this like this. This is used to know if a musician/pro audio
> > soundcard has a jack plugged, that's a very rare case. Just file a followup.
>  
> You mean we can or cannot? My intention for that is to support systems that
> do not have an updated version of PulseAudio. The data member
> pa_source/sink_port_info::available is introduces in version 2. Even try
> testbeds use Pulse version < 2 and we would not be able to try-test without
> it.

I meant you code is good as-is.

> For the rest of the comments the source have been updated.
Attachment #8705653 - Flags: review?(padenot)
Comment on attachment 8705653 [details] [diff] [review]
cubeb-Cubeb pulse implementation for full duplex updated after comments

Review of attachment 8705653 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Can you find a name for the function that calls the user callback, address the other comments, clean up the style and figure out the issue user rr ? Sometimes I find that stressing my system with:

> while :; do; stress -c 8 -i 8 -m 8 -d 8 -t 2; sleep 1; done

helps me reproducing issues I see rarely.

::: src/cubeb_pulse.c
@@ +188,4 @@
>  }
>  
>  static void
> +write_to_output(pa_stream * s, void* input_data, size_t nbytes, cubeb_stream * stm)

nit: spaces around *.

@@ +206,5 @@
>      size = towrite;
>      r = WRAP(pa_stream_begin_write)(s, &buffer, &size);
> +    if (r < 0) {
> +      // Never get here in normal scenario
> +      LOG("Unexpected error. Debugging with rr causes it\n");

I believe this is when you start writing and the stream is not yet finished initializing? Maybe there is a race somewhere? If you grab a PulseAudio package with debug symbols, rr should be able to tell you what's up.

rr serializes the program, and slows it down a bit, so this might be a real bug that is hard to catch.

@@ +220,1 @@
>      if (got < 0) {

We should replace this by `if (got != size / frame_size) {`. This might be the cause for some weirdness I witnessed on try.

@@ +270,4 @@
>  }
>  
>  static int
> +read_from_input(pa_stream* s, const void** buffer, size_t* size)

nit: spaces around * and **, here and everywhere else.

@@ +326,5 @@
> +      write_to_output(stm->output_stream, read_buffer, write_size, stm);
> +    } else {
> +      // input/capture only operation. Call callback directly
> +      void* read_buffer = (void*)read_data;
> +      if (stm->data_callback(stm, stm->user_ptr, read_buffer, NULL, read_frames) < 0) {

The semantic of the return value of the user callback in cubeb is the following: https://dxr.mozilla.org/mozilla-central/source/media/libcubeb/include/cubeb.h?from=cubeb_data_callback#228

This should be `if (stm->data_callback(...) != read_frames) {`.

@@ +440,5 @@
> +      r = operation_wait(stm->context, stm->output_stream, o);
> +      WRAP(pa_operation_unref)(o);
> +    }
> +    // if this fail abort
> +    if (r != 0){

nit: space between ) and {.

@@ +738,5 @@
> +    if (!stm->input_stream) {
> +      WRAP(pa_threaded_mainloop_unlock)(stm->context->mainloop);
> +      pulse_stream_destroy(stm);
> +      return CUBEB_ERROR;
> +    }

Can you factor part of the input and output part into a function, so we don't repeat ourselves here ? I believe you can factor the first part of each if() until this point.
Attachment #8705653 - Flags: review?(padenot)
(In reply to Paul Adenot (:padenot) from comment #6 and 7)

> Maybe something that mentions the fact that it calls the user callback ?

I put trigger_user_callback. If you come up with something better let me know.

> For now, Pulse latency is fixed to 40ms (see the latency method). From my
> testing, we can go lower, we should try it.

Latency was one of my bigger problems at the beginning. Now after a lot try and errors I get a good output (without underuns) for latency > 5ms.

The source has been updated. I will upload the new file for review.
Attached patch cubeb-pulse-fd-updated2.patch (obsolete) — — Splinter Review 
Update 2 with the comments from padenot
Attachment #8705653 - Attachment is obsolete: true
Attachment #8706433 - Flags: review?(padenot)
Comment on attachment 8706433 [details] [diff] [review]
cubeb-pulse-fd-updated2.patch

Review of attachment 8706433 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. I'm anxious about the rr problem though, I'm afraid it hides a real problem.

::: src/cubeb_pulse.c
@@ +664,5 @@
> +  return WRAP(pa_stream_new)(stm->context->context, stream_name, &ss, NULL);
> +}
> +
> +static pa_buffer_attr
> +get_beffer_attr(unsigned int latency, pa_sample_spec * sample_spec)

I'd call that `set_buffering_attribute` or something.

@@ +744,5 @@
> +      pulse_stream_destroy(stm);
> +      return CUBEB_ERROR;
> +    }
> +
> +    stm->input_sample_spec = *(WRAP(pa_stream_get_sample_spec)(stm->output_stream));

s/stm->output_stream/stm->input_stream/, even though it should be the same.

@@ +747,5 @@
> +
> +    stm->input_sample_spec = *(WRAP(pa_stream_get_sample_spec)(stm->output_stream));
> +
> +    WRAP(pa_stream_set_state_callback)(stm->input_stream, stream_state_callback, stm);
> +    WRAP(pa_stream_set_read_callback)(stm->input_stream, stream_read_callback, stm);

That can be factored more with a function like that:

rv = create_pa_stream(&stream, params, name, state_cb, data_cb, &battr);
Attachment #8706433 - Flags: review?(padenot)
> Almost there. I'm anxious about the rr problem though, I'm afraid it hides a
> real problem.

I know I feel the same way. The problem is I do not have a fix yet. I am working to repro or find any hint in the code but no luck yet ...

> That can be factored more with a function like that:
> 
> rv = create_pa_stream(&stream, params, name, state_cb, data_cb, &battr);

data_cb is used in different methods pa_stream_set_read/write_callback for input/output stream. So if I factor them I need to differentiate the input and output case. This means one more argument an additional enum maybe. I am not sure if this really helps in clarity. Also I do not want to set the buffattr inside that method because it is used in the code that follows and thus I would prefer to link it with the lines after. Now setting the state cb could be moved inside create_pa_stream but it would not make big difference and might be confusing for the reader since it will not be grouped with the rest of stream configuration.

The code has been updated for the other comments.
This is the updated patch from latest comments. I know that it does not address the rr issue since I do not have a fix yet. I set it as r? but feel free to remove it or handle it at will.
Attachment #8706433 - Attachment is obsolete: true
Attachment #8706956 - Flags: review?(padenot)
Comment on attachment 8706956 [details] [diff] [review]
cubeb-pulse-fd-updated3.patch

Review of attachment 8706956 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks !
Attachment #8706956 - Flags: review?(padenot) → review+
We can try to land this, but we should keep an eye out for the issue rr revealed.
> @@ +220,1 @@
> >      if (got < 0) {
> 
> We should replace this by `if (got != size / frame_size) {`. This might be
> the cause for some weirdness I witnessed on try.

If I check here for (got != size / frame_size) cubeb sanity test - drain_test break. This is because if "0 < got < (size / frame)" we try to write what we got before we go to drain state and shutdown. This check start in if branch at line 252. Unfortunately I did not notice during comment update and I found out the hard way ... I am gonna leave this one as it is.
Can we open a followup and fix it ?
If we leave it as it is works just fine. I did fix it before send the PR.
This patch contains all fd changes for libcubeb including pull reuest #64:

https://github.com/kinetiknz/cubeb/pull/64

It does require additional patches on the top of it, in order to enable fd functionality inside gecko.
Replace the previous one since created accidentally on the wrong branch
Attachment #8709429 - Attachment is obsolete: true
Landed with a cubeb import
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: