Return early when the length of incoming data is zero

[achronop]

On debug builds zero data length was crashing in PodMove/Zero/Copy() methods used by auto_array.

[achronop]

This is the fix for the following crash in firefox:

* thread #49, name = 'NativeA~allback', stop reason = signal SIGABRT
    frame #0: 0x00007fff643452c2 libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fff643452c2 <+10>: jae    0x7fff643452cc            ; <+20>
    0x7fff643452c4 <+12>: movq   %rax, %rdi
    0x7fff643452c7 <+15>: jmp    0x7fff6433f453            ; cerror_nocancel
    0x7fff643452cc <+20>: retq
Target 0: (plugin-container) stopped.
(lldb) bt
* thread #49, name = 'NativeA~allback', stop reason = signal SIGABRT
  * frame #0: 0x00007fff643452c2 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff64400bf1 libsystem_pthread.dylib`pthread_kill + 284
    frame #2: 0x00007fff642af6a6 libsystem_c.dylib`abort + 127
    frame #3: 0x00007fff6427820d libsystem_c.dylib`__assert_rtn + 324
    frame #4: 0x000000011f1c7e90 XUL`void PodCopy<float>(destination=0x0000000000000000, source=0x000000016ea21000, count=0) at cubeb_utils.h:31:3
    frame #5: 0x000000011f1c7d27 XUL`auto_array<float>::push(this=0x0000000159b3ffd8, elements=0x000000016ea21000, length=0) at cubeb_utils.h:198:5
    frame #6: 0x000000011f1d114d XUL`passthrough_resampler<float>::fill(this=0x0000000159b3ffb0, input_buffer=0x000000016ea21000, input_frames_count=0x000070001042a7d0, output_buffer=0x000000016caed010, output_frames=512) at cubeb_resampler.cpp:83:29
    frame #7: 0x000000011f1d2c60 XUL`::cubeb_resampler_fill(resampler=0x0000000159b3ffb0, input_buffer=0x000000016ea21000, input_frames_count=0x000070001042a7d0, output_buffer=0x000000016caed010, output_frames_needed=512) at cubeb_resampler.cpp:374:21
    frame #8: 0x00000001256cbebe XUL`cubeb_coreaudio::backend::resampler::Resampler::fill::hf2676688889433b5(self=0x000000016d7c8d08, input_buffer=0x000000016ea21000, input_frame_count=0x000070001042a7d0, output_buffer=0x000000016caed010, output_frames_needed=512) at resampler.rs:52:13
    frame #9: 0x00000001256722eb XUL`cubeb_coreaudio::backend::audiounit_output_callback::_$u7b$$u7b$closure$u7d$$u7d$::hd373eb27e0f9fe4a((null)=0x000070001042c8b0, stm=0x000000016d7c8c00, output_frames=512, buffers=&mut [coreaudio_sys::AudioBuffer] @ 0x000070001042b4e8) at mod.rs:666:25
    frame #10: 0x00000001256705ee XUL`cubeb_coreaudio::backend::audiounit_output_callback::hb2a0aaee49ab8bc9(user_ptr=0x000000016d7c8c00, flags=0x000070001042cc90, tstamp=0x000070001042cc18, bus=0, output_frames=512, out_buffer_list=0x000000016d7a1f80) at mod.rs:727:34
    frame #11: 0x000000016f084a1f CoreAudio`AUConverterBase::RenderBus(unsigned int&, AudioTimeStamp const&, unsigned int, unsigned int) + 729
    frame #12: 0x000000016f1a0587 CoreAudio`AUBase::DoRenderBus(unsigned int&, AudioTimeStamp const&, unsigned int, AUOutputElement*, unsigned int, AudioBufferList&) + 169
    frame #13: 0x000000016f19fce7 CoreAudio`AUBase::DoRender(unsigned int&, AudioTimeStamp const&, unsigned int, unsigned int, AudioBufferList&) + 601
    frame #14: 0x000000016f086ba6 CoreAudio`AUHAL::AUIOProc(unsigned int, AudioTimeStamp const*, AudioBufferList const*, AudioTimeStamp const*, AudioBufferList*, AudioTimeStamp const*, void*) + 1704
    frame #15: 0x00007fff37d0b2d2 CoreAudio`HALC_ProxyIOContext::IOWorkLoop() + 4908
    frame #16: 0x00007fff37d09df4 CoreAudio`HALC_ProxyIOContext::IOThreadEntry(void*) + 122
    frame #17: 0x00007fff37d09956 CoreAudio`HALB_IOThread::Entry(void*) + 72
    frame #18: 0x00007fff643fe2eb libsystem_pthread.dylib`_pthread_body + 126
    frame #19: 0x00007fff64401249 libsystem_pthread.dylib`_pthread_start + 66
    frame #20: 0x00007fff643fd40d libsystem_pthread.dylib`thread_start + 13
(lldb) up 5
frame #5: 0x000000011f1c7d27 XUL`auto_array<float>::push(this=0x0000000159b3ffd8, elements=0x000000016ea21000, length=0) at cubeb_utils.h:198:5
   195 	    if (length_ + length > capacity_) {
   196 	      reserve(length_ + length);
   197 	    }
-> 198 	    PodCopy(data_ + length_, elements, length);
   199 	    length_ += length;
   200 	  }
   201
(lldb) p length_ + length
(unsigned long) $0 = 0
(lldb) p data_ + length_
(float *) $1 = 0x0000000000000000
(lldb) up
frame #6: 0x000000011f1d114d XUL`passthrough_resampler<float>::fill(this=0x0000000159b3ffb0, input_buffer=0x000000016ea21000, input_frames_count=0x000070001042a7d0, output_buffer=0x000000016caed010, output_frames=512) at cubeb_resampler.cpp:83:29
   80  	      // It can happen when system's performance is poor. Audible silence is
   81  	      // being pushed at the end of the short input buffer. An improvement for
   82  	      // the future is to resample to the output number of frames, when that happens.
-> 83  	      internal_input_buffer.push(static_cast<T*>(input_buffer),
   84  	                                 frames_to_samples(*input_frames_count));
   85  	      if (internal_input_buffer.length() < frames_to_samples(output_frames)) {
   86  	        // This is unxpected but it can happen when a glitch occurs. Fill the
(lldb)
frame #7: 0x000000011f1d2c60 XUL`::cubeb_resampler_fill(resampler=0x0000000159b3ffb0, input_buffer=0x000000016ea21000, input_frames_count=0x000070001042a7d0, output_buffer=0x000000016caed010, output_frames_needed=512) at cubeb_resampler.cpp:374:21
   371 	                     void * output_buffer,
   372 	                     long output_frames_needed)
   373 	{
-> 374 	  return resampler->fill(input_buffer, input_frames_count,
   375 	                         output_buffer, output_frames_needed);
   376 	}
   377
(lldb)

[ChunMinChang]

Is *input_frames_count 0 in this case (p *input_frames_count should be 0)?

https://github.com/kinetiknz/cubeb/blob/a971bf1a045b0e5dcaffd2a15c3255677f43cd2d/src/cubeb_resampler.h#L64

I guess the cause hitting that assertion is because the PodCopy is called but reserve is never called. So data_ is nullptr and data_ + length_ = nullptr + 0 makes assert(destination && ....) in PodCopy panics.

If input_frames_count is 0, it means there is nothing on the input side for resampling. I wonder if we still need to use resampler in this case.

If we should avoid using resampler when *input_frames_count == 0, we can

• Add assert(*input_frames_count > 0)` in resampler code
• Do some special handling when audiounit_output_callback give a 0 input buffer.

I am not sure if it's proper to allow for giving a 0-size input buffer when the resampling_in_buffer isn't initialized. Maybe others could give better opinions

[achronop]

The *input_frames_count is 0 and the root cause is exactly that, reserve is not being called and we panic as you described.

If we should avoid using resampler when *input_frames_count == 0, we can

My opinion is that we must continue using resampler when the *input_frames_count == 0. This is because the input data (to the resampler), is buffered in internal_input_buffer so we hope to have extra input frames buffered that will be passed to the output even if the provided input frame number is zero for this iteration.

Now if we want to avoid calling the internal_input_buffer.push(), which is the auto_array::push, with zero incoming frames is a valid option. My decision was to handle that case internally in auto_array because it makes more sense to me that the auto_array, as an independent data structure, handles that corner case internally. But it is not a strong opinion. If we want to assert in auto_array that the length is always greater than zero and move that check in the resampler I can hear it.

[ChunMinChang]

This is because the input data (to the resampler), is buffered in internal_input_buffer so we hope to have extra input frames buffered that will be passed to the output even if the provided input frame number is zero for this iteration.

Does the output callback come before the input callback so there is no data in the input buffer (e.g., the output callback is the first callback, before input callback, for a duplex stream)?

Should silent frames be appended in this case? I wonder if we would hear some noise without appending some silent frames. IIRC, https://github.com/ChunMinChang/cubeb-coreaudio-rs/pull/79 dealt with a similar issue: there is a glitch when output callback comes before input callback. @padenot: Do you remember what happened in that PR?

From the perspective of auto_array, returning early when length is natural; asserting length is ok. It depends on if we want to restrict the usage of push or not. I don't have a strong opinion either. It's ok for me to return early in this case, if it's a valid case for using the resampler.

[achronop]

Does the output callback come before the input callback so there is no data in the input buffer (e.g., the output callback is the first callback, before input callback, for a duplex stream)?

Yes, that's why reserve has never been called and the crash is happening.

Should silent frames be appended in this case?

Regarding passthrough resampler, I would expect that silence will be added right after, here

[ChunMinChang]

Regarding passthrough resampler, I would expect that silence will be added right after, here

Cool. I have nothing to worry if the silent frames would be appended automatically.