1221575 - Write a full-duplex OSX cubeb backend

Status: RESOLVED FIXED

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

Comment 1

[Alex Chronopoulos [:achronop]]

Attachment: audiounit-full-duplex.patch

This is the full-duplex implementation for for AudioUnit. Contains the full duplex functionality and device selection. The diff comes from github cubeb repo. Set for an initial review before create a Pull Request.

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8720867 [details] [diff] [review]
audiounit-full-duplex.patch

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

::: src/cubeb.c
@@ +425,4 @@
>  
>  int cubeb_device_info_destroy(cubeb_device_info * info)
>  {
> +  free(info->devid);

Is that not a double free in some cases ? Not in this backend but maybe in others ?

::: src/cubeb_audiounit.c
@@ +180,3 @@
>  
> +  /* Input only. Call the user callback through resampler.
> +   * Resampler will deliver input buffer in the correct rate*/

nit: . and space please.

@@ +200,5 @@
> +
> +static void
> +audiounit_make_silent (AudioBufferList * ioData)
> +{
> +  for(UInt32 i=0; i<ioData->mNumberBuffers;i++)

Always { and } even for single line for, and space around =.

@@ +257,5 @@
> +      return noErr;
> +    }
> +    /* Input samples stored from input callback */
> +    input_buffer = stream->input_buffer_list.mBuffers[0].mData;
> +    input_frames = stream->input_buffer_frames;

Do we have the guarantee that we have interleaved input and output callback ? On what thread are those called, is it the same thread ? If yes, we might be able to not use the mutex.

In any case, we should document the threading: what should be protected by mutex, etc.

You can use a getter with a threading assert (assert_current_thread_owns...) on the object that need locking. It's a bit more verbose in C than in C++, but maybe it's feasible.  That's even better than a comment.

@@ +291,5 @@
> +  /* Post process output samples*/
> +  if ((UInt32) outframes < output_frames) {
> +    /* Clear missing frames (silence) */
> +    memset(output_buffer + outframes * outbpf, 0, (output_frames - outframes) * outbpf);
> +  }

Hm, in what situation would that happen ? This can include a discontinuity in the stream. I think this only happens when draining, right ?

@@ +737,5 @@
> +  enable = 1;
> +  if (AudioUnitSetProperty(*unit, kAudioOutputUnitProperty_EnableIO,
> +        is_input ? kAudioUnitScope_Input : kAudioUnitScope_Output,
> +        is_input ? AU_IN_BUS : AU_OUT_BUS, &enable, sizeof(UInt32)) != noErr)
> +    return CUBEB_ERROR;

{ } on if.

@@ +865,5 @@
> +                              kAudioDevicePropertyBufferFrameSize,
> +                              kAudioUnitScope_Input,
> +                              AU_IN_BUS,
> +                              &stm->input_buffer_frames,
> +                              &size) != 0) {

nit: indent, here and on other functions.

@@ +894,5 @@
> +      return CUBEB_ERROR;
> +    }
> +    stm->input_hw_rate = input_hw_desc.mSampleRate;
> +
> +    /* Set format description according to the input params */

. at the end of comments.

@@ +1050,5 @@
> +      output_stream_params,
> +      target_sample_rate,
> +      stm->data_callback,
> +      stm->user_ptr,
> +      CUBEB_RESAMPLER_QUALITY_DESKTOP);

nit: indent

::: src/cubeb_resampler.cpp
@@ +63,4 @@
>           !output_buffer && output_frames== 0);
>  
> +  if (output_buffer == nullptr)
> +    output_frames = *input_frames_count;

Always { } even for single line ifs.

Comment 3

[Alex Chronopoulos [:achronop]]

 
> Is that not a double free in some cases ? Not in this backend but maybe in
> others ?

It depends on us. We can decide that will always allocate new memory and copy the value of devid to avoid the double free. The problem is that cubeb_device_info_destroy method is not available in the backend so we cannot have separate handling. Here I need to allocate new memory for devid and otherwise I am not sure which could be an alternative way to clean it in the right moment. Feel free to advice.

> Do we have the guarantee that we have interleaved input and output callback
> ? On what thread are those called, is it the same thread ? If yes, we might
> be able to not use the mutex.
> 
> In any case, we should document the threading: what should be protected by
> mutex, etc.
> 
> You can use a getter with a threading assert (assert_current_thread_owns...)
> on the object that need locking. It's a bit more verbose in C than in C++,
> but maybe it's feasible.  That's even better than a comment.
> 
By testing, input and output buffer are interleaved. I add the appropriate flag on the format specifier to guarantee that.

The threads are not the same. Every callback is executed in separate thread. I am not sure how should I use the getter and the threading asserts. Can you please explain?

> Hm, in what situation would that happen ? This can include a discontinuity
> in the stream. I think this only happens when draining, right ?
> 
It is in draining. In line 284 we set the drain flag using the same condition. I updated to if (draining) in order to make it clear

All other comments updated

Comment 4

[Alex Chronopoulos [:achronop]]

Attachment: audiounit-full-duplex-updated.patch

Updated patch according to the comments. I ask for a few clarification. Nevertheless I set it for r?. Feel free to cancel the review.

Comment 5

[Paul Adenot (:padenot)]

(In reply to Alex Chronopoulos [:achronop] from comment #3)
>  
> > Is that not a double free in some cases ? Not in this backend but maybe in
> > others ?
> 
> It depends on us. We can decide that will always allocate new memory and
> copy the value of devid to avoid the double free. The problem is that
> cubeb_device_info_destroy method is not available in the backend so we
> cannot have separate handling. Here I need to allocate new memory for devid
> and otherwise I am not sure which could be an alternative way to clean it in
> the right moment. Feel free to advice.

I think it's better to always allocate and always free, we should document it somewhere. This is not very high traffic methods anyways.
 
> > Do we have the guarantee that we have interleaved input and output callback
> > ? On what thread are those called, is it the same thread ? If yes, we might
> > be able to not use the mutex.
> > 
> > In any case, we should document the threading: what should be protected by
> > mutex, etc.
> > 
> > You can use a getter with a threading assert (assert_current_thread_owns...)
> > on the object that need locking. It's a bit more verbose in C than in C++,
> > but maybe it's feasible.  That's even better than a comment.
> > 
> By testing, input and output buffer are interleaved. I add the appropriate
> flag on the format specifier to guarantee that.

Sorry this was unclear. I meant: do the input and output callback always come like this:

input; output; input; output; input; output

Or can something like this happen:

> input; output; output; input; input; output;

is there a guarantee about this ?

> The threads are not the same. Every callback is executed in separate thread.
> I am not sure how should I use the getter and the threading asserts. Can you
> please explain?

If an object needs locking, you can use the following pattern:

object * get_object(cubeb_stream * stm, lock * aLock) {
  aLock->assert_current_thread_owns();
  return stm->object;
}

and always use this getter in place of accessing the attribute directly.

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8721313 [details] [diff] [review]
audiounit-full-duplex-updated.patch

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

More questions !

::: src/cubeb_audiounit.c
@@ +143,5 @@
> +                        AudioUnitRenderActionFlags * flags,
> +                        AudioTimeStamp const * tstamp,
> +                        UInt32 bus,
> +                        UInt32 input_frames,
> +                        AudioBufferList * bufs)

Indent is off here.

@@ +165,5 @@
> +                                flags,
> +                                tstamp,
> +                                bus,
> +                                input_frames,
> +                                &stream->input_buffer_list);

Indent is off here.

@@ +198,5 @@
> +  return noErr;
> +}
> +
> +static void
> +audiounit_make_silent (AudioBufferList * ioData)

No space before (.

@@ +266,5 @@
> +  outframes = cubeb_resampler_fill(stream->resampler,
> +                                   input_buffer,
> +                                   &input_frames,
> +                                   output_buffer,
> +                                   output_frames);

Maybe we should clear out the input_buffer here, to be able to assert that we have fresh data.

@@ +295,5 @@
> +    memset(output_buffer + outframes * outbpf, 0, (output_frames - outframes) * outbpf);
> +  }
> +  /* Pan stereo */
> +  if (panning != 0.0f) {
> +    if (outaff & kAudioFormatFlagIsFloat)

Always put { } on ifs.

@@ +722,5 @@
> +  AudioComponent comp;
> +  UInt32 enable;
> +  AudioDeviceID devid;
> +
> +  desc.componentType = kAudioUnitType_Output;

Is that always _Output ? Seems strange.

@@ +728,5 @@
> +  desc.componentManufacturer = kAudioUnitManufacturer_Apple;
> +  desc.componentFlags = 0;
> +  desc.componentFlagsMask = 0;
> +  comp = AudioComponentFindNext(NULL, &desc);
> +  if (comp == NULL)

{ and } and below.

@@ +905,5 @@
> +    }
> +
> +    AudioStreamBasicDescription src_desc = stm->input_desc;
> +    /* AudioUnit touching input hardware is very sensitive to sample rate
> +     * thus we choose to set the device sampling rate and to resample later */

Sensitive in what sense ?

@@ +1133,5 @@
>  audiounit_stream_start(cubeb_stream * stm)
>  {
>    OSStatus r;
> +  if (stm->input_unit != NULL) {
> +    r = AudioOutputUnitStart(stm->input_unit);

It's crazy that you use AudioOutputUnitStart to start an input unit.

@@ +1152,5 @@
> +  if (stm->output_unit != NULL) {
> +    r = AudioOutputUnitStop(stm->output_unit);
> +    assert(r == 0);
> +  }
> +  if (stm->input_unit != NULL && stm->input_unit != stm->output_unit) {

When is `stm->input_unit == stm->output_unit` ?

Comment 7

[Alex Chronopoulos [:achronop]]

I post a separated answer to this one since I believe it reveals missing impact

> Sorry this was unclear. I meant: do the input and output callback always
> come like this:
> 
> input; output; input; output; input; output
> 
> Or can something like this happen:
> 
> > input; output; output; input; input; output;
> 
> is there a guarantee about this ?

No, I cannot see any guarantee about the synchronization of the threads, even if it happens all the time in my tests. I guess the wise thing to do is not to be based on that assumption. This makes the buffering more challenging. I see in other code samples that they make use of a circular buffer and that might be the correct direction. But creating a circular buffer in C just for audiounit wouldn't be overkill? Or if I have to isn't better to implement a general one available to all backends? I see some interesting implementation like https://github.com/michaeltyson/TPCircularBuffer but they are for OSX and I am not sure if it is good to drug them inside cubeb. 

How do you handle this in wasapi? In general, am I allowed to change the file from .c to .cpp?

Comment 8

[Alex Chronopoulos [:achronop]]

(In reply to Paul Adenot (:padenot) from comment #5)

> I think it's better to always allocate and always free, we should document
> it somewhere. This is not very high traffic methods anyways.
I added a comment in cubeb.h:224.

> If an object needs locking, you can use the following pattern:
> 
> object * get_object(cubeb_stream * stm, lock * aLock) {
>   aLock->assert_current_thread_owns();
>   return stm->object;
> }
> 
> and always use this getter in place of accessing the attribute directly.

I guess, I need to implement the assert_current_thread_owns() in a similar way PRLock does it here https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/pthreads/ptsynch.c#135 , right?

Comment 9

[Alex Chronopoulos [:achronop]]

(In reply to Paul Adenot (:padenot) from comment #6)

> @@ +266,5 @@
> > +  outframes = cubeb_resampler_fill(stream->resampler,
> > +                                   input_buffer,
> > +                                   &input_frames,
> > +                                   output_buffer,
> > +                                   output_frames);
> 
> Maybe we should clear out the input_buffer here, to be able to assert that
> we have fresh data.

Input buffered cleared after using it.

> @@ +722,5 @@
> > +  AudioComponent comp;
> > +  UInt32 enable;
> > +  AudioDeviceID devid;
> > +
> > +  desc.componentType = kAudioUnitType_Output;
> 
> Is that always _Output ? Seems strange.

It is strange indeed but it is the Apple way. In an unfortunate bit of naming to capture from input device you have to use an AudioUnit of type Output.

> @@ +905,5 @@
> > +    }
> > +
> > +    AudioStreamBasicDescription src_desc = stm->input_desc;
> > +    /* AudioUnit touching input hardware is very sensitive to sample rate
> > +     * thus we choose to set the device sampling rate and to resample later */
> 

Wrong phrasing. Comment updated.

> 
> @@ +1133,5 @@
> >  audiounit_stream_start(cubeb_stream * stm)
> >  {
> >    OSStatus r;
> > +  if (stm->input_unit != NULL) {
> > +    r = AudioOutputUnitStart(stm->input_unit);
> 
> It's crazy that you use AudioOutputUnitStart to start an input unit.

And since AU is of type Output the corresponding method must be used.

> @@ +1152,5 @@
> > +  if (stm->output_unit != NULL) {
> > +    r = AudioOutputUnitStop(stm->output_unit);
> > +    assert(r == 0);
> > +  }
> > +  if (stm->input_unit != NULL && stm->input_unit != stm->output_unit) {
> 
> When is `stm->input_unit == stm->output_unit` ?

Probably left there from some earlier experiments. Removed.

Comment 10

[Alex Chronopoulos [:achronop]]

Attachment: audiounit-full-duplex-updated2.patch

Patch updated with the second set of comments. I am asking one more thing so feel free to cancel the review.

Comment 11

[Paul Adenot (:padenot)]

(In reply to Alex Chronopoulos [:achronop] from comment #8)
> (In reply to Paul Adenot (:padenot) from comment #5)
> 
> > I think it's better to always allocate and always free, we should document
> > it somewhere. This is not very high traffic methods anyways.
> I added a comment in cubeb.h:224.
> 
> > If an object needs locking, you can use the following pattern:
> > 
> > object * get_object(cubeb_stream * stm, lock * aLock) {
> >   aLock->assert_current_thread_owns();
> >   return stm->object;
> > }
> > 
> > and always use this getter in place of accessing the attribute directly.
> 
> I guess, I need to implement the assert_current_thread_owns() in a similar
> way PRLock does it here
> https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/pthreads/
> ptsynch.c#135 , right?

There are more lightweight ways to implement it these days, try something like this:

> #include <pthread.h>
> #include <assert.h>
> #include <errno.h>
> 
> int main()
> {
>   pthread_mutexattr_t attr;
>   pthread_mutex_t mut;
> 
>   pthread_mutexattr_init(&attr);
>   pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK);
>   pthread_mutex_init(&mut, &attr);
> 
>   pthread_mutex_lock(&mut);
> 
> #define ASSERT_LOCKED(mutex) \
>   do {                   \
>   int rv;                \
>   rv = pthread_mutex_lock(&mutex); \
>   assert(rv == EDEADLK); \
>   } while (0);
> 
>   ASSERT_LOCKED(mut);
> 
>   pthread_mutex_unlock(&mut);
> 
>   ASSERT_LOCKED(mut);
> 
>   return 0;
> }

Comment 12

[Alex Chronopoulos [:achronop]]

Attachment: audiounit-full-duplex-updated3.patch

Major add is a simplified ring array responsible to keep pointers of the input buffers and deliver them when needed in the right order.

Also added assert for the lock mutex. I did not add getter methods for every data member. If you think it is not adequate in the way that it is now I will update.

The patch is rebased to the latest cubeb.

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8725700 [details] [diff] [review]
audiounit-full-duplex-updated3.patch

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

We need tests for this new ring buffer.

::: src/cubeb_audiounit.c
@@ +67,5 @@
> +} while (0);
> +
> +/* Ring array of pointers is used to hold input data buffers. In case that
> + * input and output callbacks do not arrive in a repeated order the ring buffer
> + * stores the buffers and provides them in correct order */

Full stop and the end of comment, and doxygen-style comments (/** */).

@@ +75,5 @@
> +  void* pointer_array[RING_ARRAY_CAPACITY];
> +  int tail;
> +  int count;
> +  int capacity;
> +  pthread_mutex_t mutex;

Document all those.

@@ +88,5 @@
> +  memset(ra->pointer_array, 0, sizeof(ra->pointer_array));
> +
> +  pthread_mutexattr_t attr;
> +  pthread_mutexattr_init(&attr);
> +  pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK);

Maybe we want this debug-only ?

@@ +93,5 @@
> +  int ret = pthread_mutex_init(&ra->mutex, &attr);
> +  pthread_mutexattr_destroy(&attr);
> +  assert(0 == ret);
> +
> +  return 0;

return ret; ?

@@ +96,5 @@
> +
> +  return 0;
> +}
> +
> +/* Set the allocated space to store the data.

/** + param documentation.

@@ +111,5 @@
> +
> +void
> +ring_array_destroy(ring_array * ra)
> +{
> +  pthread_mutex_destroy(&ra->mutex);

Assert that the array is empty, or do the appropriate cleaning measures.

@@ +120,5 @@
> +ring_array_store_buffer(ring_array * ra)
> +{
> +  pthread_mutex_lock(&ra->mutex);
> +  ASSERT_LOCKED(ra->mutex)
> +  assert(ra->count == 0 || (ra->tail + ra->count)%ra->capacity != ra->tail);

What are we doing when/if it gets full ? I think we should do something. I'm sure there are some drivers that can do crazy stuff. We don't have to do the impossible, but not crashing and outputing silence/dropping buffer would be good.

@@ +121,5 @@
> +{
> +  pthread_mutex_lock(&ra->mutex);
> +  ASSERT_LOCKED(ra->mutex)
> +  assert(ra->count == 0 || (ra->tail + ra->count)%ra->capacity != ra->tail);
> +  void* ret = ra->pointer_array[(ra->tail + ra->count)%ra->capacity];

Space around *.

@@ +131,5 @@
> +  return ret;
> +}
> +
> +/* Get the next available buffer filled buffer. */
> +void*

Space around *.

@@ +142,5 @@
> +    return NULL;
> +  }
> +  void* ret = ra->pointer_array[ra->tail];
> +
> +  ra->tail = (ra->tail + 1)%ra->capacity;

Space around operators.

@@ +368,5 @@
> +    if (fetch_input_buffer_list == NULL) {
> +      printf("We have got more output requests than input. "
> +             "This is either a hole or we are after a stream stop and input thread stopped before output\n");
> +      /* Provide silent input. Other than that we could provide silent output and exit without
> +       * calling user callback. I do not prefer it because the user loose the control of

Avoid first person comments.

Comment 14

[Alex Chronopoulos [:achronop]]

Tow quick clarifications. Will follow more updates.

(In reply to Paul Adenot (:padenot) from comment #13)
> Comment on attachment 8725700 [details] [diff] [review]
> audiounit-full-duplex-updated3.patch
> 
> Review of attachment 8725700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We need tests for this new ring buffer.

For now the ring buffer is only visibly to this file. Do you suggest to move it to a separate file and create tests for it? 

> @@ +120,5 @@
> > +ring_array_store_buffer(ring_array * ra)
> > +{
> > +  pthread_mutex_lock(&ra->mutex);
> > +  ASSERT_LOCKED(ra->mutex)
> > +  assert(ra->count == 0 || (ra->tail + ra->count)%ra->capacity != ra->tail);
> 
> What are we doing when/if it gets full ? I think we should do something. I'm
> sure there are some drivers that can do crazy stuff. We don't have to do the
> impossible, but not crashing and outputing silence/dropping buffer would be
> good.

If assert is disabled we override the buffer which means some distortion in the output. In general it is a little monolithic implementation. The idea is to choose big enough capacity to avoid that. If that's not possible drop is also an alternative.

Comment 15

[Alex Chronopoulos [:achronop]]

(In reply to Paul Adenot (:padenot) from comment #13)
> Comment on attachment 8725700 [details] [diff] [review]
> audiounit-full-duplex-updated3.patch
> 
> Review of attachment 8725700 [details] [diff] [review]:
> -----------------------------------------------------------------


> @@ +111,5 @@
> > +
> > +void
> > +ring_array_destroy(ring_array * ra)
> > +{
> > +  pthread_mutex_destroy(&ra->mutex);
> 
> Assert that the array is empty, or do the appropriate cleaning measures.

This does not have to be empty since the array does not allocate any space for the data. Actually if input thread happen to be terminated after output thread there might be some extra, not needed, buffers inside. 

All other comments updated.

Comment 16

[Alex Chronopoulos [:achronop]]

Attachment: audiounit-full-duplex-updated4.patch

New patch updated with review comments. The same patches is also pushed in my github branch:

https://github.com/achronop/cubeb/tree/audiounit-full-duplex

All updates are contained in the following commits:

* 76c1aa4 - Use mutex erro check on debug 
* afd50c7 - Remove extra debug messages
* ed61bb0 - Drop buffers if array is full
* 00b4d5c - Add documentation comment to ring array
* c9e4d6a - Style changes updates

thanks for your time

Comment 17

[Alex Chronopoulos [:achronop]]

Added one more commit: 
* 27a8209 - Clear warning in test_resampler.

Comment 18

[Paul Adenot (:padenot)]

(In reply to Alex Chronopoulos [:achronop] from comment #14)
> Tow quick clarifications. Will follow more updates.
> 
> (In reply to Paul Adenot (:padenot) from comment #13)
> > Comment on attachment 8725700 [details] [diff] [review]
> > audiounit-full-duplex-updated3.patch
> > 
> > Review of attachment 8725700 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We need tests for this new ring buffer.
> 
> For now the ring buffer is only visibly to this file. Do you suggest to move
> it to a separate file and create tests for it? 

Yes, that's a good way. Maybe you can just put it in cubeb_utils.h, although it's c++. Then again, I prefer C++ because it gives us RAII, which is nice for mutex and stuff like this.


> > @@ +120,5 @@
> > > +ring_array_store_buffer(ring_array * ra)
> > > +{
> > > +  pthread_mutex_lock(&ra->mutex);
> > > +  ASSERT_LOCKED(ra->mutex)
> > > +  assert(ra->count == 0 || (ra->tail + ra->count)%ra->capacity != ra->tail);
> > 
> > What are we doing when/if it gets full ? I think we should do something. I'm
> > sure there are some drivers that can do crazy stuff. We don't have to do the
> > impossible, but not crashing and outputing silence/dropping buffer would be
> > good.
> 
> If assert is disabled we override the buffer which means some distortion in
> the output. In general it is a little monolithic implementation. The idea is
> to choose big enough capacity to avoid that. If that's not possible drop is
> also an alternative.

Yeah doing either of those is good.

Other comments on github.

Comment 19

[Alex Chronopoulos [:achronop]]

Updates according to the latest comments. Commits can be found in the branch

https://github.com/achronop/cubeb/tree/audiounit-full-duplex

* 1a2dcd9 - Add test for ring array.
* d174f67 - Split ring array to a separate file
* 72274c4 - Extra dependency for test_resampler linking.
* 6aaa57d - Clear test_resampler link error
* ba16722 - Change mutex attr to default.
* 27a8209 - Clear warning in test_resampler. *previous last*

Feel free to comments on github. Thanks!

Comment 20

[Alex Chronopoulos [:achronop]]

One more commit very tiny
* e1e8ba9 Add some spaces for pretty print

Comment 21

[Paul Adenot (:padenot)]

I think we can close that, right ? This has landed when updating cubeb in bug 1251502.

Comment 22

[Paul Adenot (:padenot)]

(clearing NI)

Comment 23

[Paul Adenot (:padenot)]

Comment on attachment 8726225 [details] [diff] [review]
audiounit-full-duplex-updated4.patch

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

This has landed ages ago, just canceling the request.