833724 - [Audio] If a size of short tone is small then buffer size of audio backend then it doesn't be played.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

In sa_stream_write() of sydney_audio_gonk.cpp, we called sa_stream_resume() to start playing on the condition as below.

/* When it's not playing, it's a non-blocking call that will return a short
   write when the buffer is full.  Use a short write to indicate a good
   time to start the AudioTrack playing.
*/

If the size of short tone for playing is small then backend audio buffer, there will be no chance to start playing.

Comment 1

[viral [:viralwang]]

Attachment: patch v1

We use variable "isPaused" to check if we need to call sa_stream_resume() to start playing.
We set isPaused to 1 as initial and also make sure we will start playing if isPaused is set in function sa_stream_write()

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 711219 [details] [diff] [review]
patch v1

This change causes sa_stream_resume to be called after the very first write call.  That effectively disables the buffer filling behaviour that triggers sa_stream_resume on a short write (once the buffer has filled).  This was added in bug 695986 comment 5 to avoid stuttering when playback first starts.

In the other sydneyaudio backends, we added code to sa_stream_drain to check if any audio data is buffered but playback has not started, then force any buffered writes and start playback if necessary.  This works if the short sounds are played via the media decoders, since they're guaranteed to call sa_stream_drain.  Unfortunately, it doesn't help for short sounds played via mozWriteAudio because there's no clear point at which to drain.

Perhaps a better solution would be to call drain before destroying the stream inside nsHTMLAudioElement.

Comment 3

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #2)
> Perhaps a better solution would be to call drain before destroying the
> stream inside nsHTMLAudioElement.

Note that it's essential that drain is not called on the main thread, as it's a blocking call.

Comment 4

[Marco Chen [:mchen]]

(In reply to Matthew Gregan [:kinetik] from comment #2)
> Comment on attachment 711219 [details] [diff] [review]
> patch v1
> 
> This change causes sa_stream_resume to be called after the very first write
> call.  That effectively disables the buffer filling behaviour that triggers
> sa_stream_resume on a short write (once the buffer has filled).  This was
> added in bug 695986 comment 5 to avoid stuttering when playback first starts.
> 

1. The frame size on unagi is 870 bytes. So in the current code if first write size is bigger then 870 (ex: 871) then sa_stream_resume will be called.
2. If the write size is small then 870 bytes then even we call audiotrack::start() the buffer doesn't be played. Because it doesn't fill the first frame buffer full.

So based on point 1 & 2, the patch here will not get worse then current design.
  a. if first write is bigger then 870, start() will be called on current code and patch.
  b. if first write is equal to 870, start() will be called by patch but current code. (Issue)
  c. if first write is small to 870, start() will be called by patch but current code.
     Even though, according to first frame doesn't be filled full so there is no real play occurred.

--------------------------------------

So I seems not find any case caused the worst behavior then current code.

Comment 5

[Marco Chen [:mchen]]

(In reply to Matthew Gregan [:kinetik] from comment #2)
> In the other sydneyaudio backends, we added code to sa_stream_drain to check
> if any audio data is buffered but playback has not started, then force any
> buffered writes and start playback if necessary.  This works if the short
> sounds are played via the media decoders, since they're guaranteed to call
> sa_stream_drain.  Unfortunately, it doesn't help for short sounds played via
> mozWriteAudio because there's no clear point at which to drain.

Since we can't guarantee what calling sequence with buffer size will be used through mozWriteAudio(). Do we need 
  1. To provide a API for user to get frame size of audio backend. So comment in mozWriteAudio() that the sound will be played only the first frame be filled full.
  2. if your write size is small then frame size then mozWriteAudio will append silence to fill the frame full on each call.

Comment 6

[Matthew Gregan [:kinetik]]

(In reply to Marco Chen [:mchen] from comment #4)
> 1. The frame size on unagi is 870 bytes. So in the current code if first
> write size is bigger then 870 (ex: 871) then sa_stream_resume will be called.

"frame size" is, unfortunately, an overloaded term in media.  In the Gecko media code, we use it to refer to the bundle of data that includes exactly one sample for each channel, e.g. the frame size for a 16-bit stereo format would be sizeof(int16_t) * 2 channels = 4 bytes (= 1 frame).

I assume you're using the term to refer to another buffer size, but I don't understand exactly which one you mean.

It turns out that the (r != towrite) test is never hit for regular media playback, because AudioLoop calls Pause() and Resume() before writing audio data, so AudioTrack::write blocks rather than returning a short write.  As a test I quickly removed the Pause/Resume and (with a 48kHz stereo stream) found it buffers 2612 frames (10448 bytes, ~54.5ms) before calling sa_stream_resume.  That happens on the third sa_stream_write of 4096 bytes.

Since we've haven't been hitting this code, I agree that the proposed change does not make the current situation worse.

Having said that, it will be better if we can find a solution that allows us to buffer as much as possible before starting playback, because it significantly reduces the chances of audio underruns when starting playback.

(In reply to Marco Chen [:mchen] from comment #5)
> Since we can't guarantee what calling sequence with buffer size will be used
> through mozWriteAudio(). Do we need 
>   1. To provide a API for user to get frame size of audio backend. So
> comment in mozWriteAudio() that the sound will be played only the first
> frame be filled full.
>   2. if your write size is small then frame size then mozWriteAudio will
> append silence to fill the frame full on each call.

We've known the Mozilla Audio Data API was incomplete and broken for a long time, but we've resisted extending it because the longer term plan is to replace it with the Web Audio API, which won't suffer from the same problems (and will be maintained).

Option 2 would result in gaps if the caller called mozWriteAudio many times with small buffers, since it'd insert silence between each write.  If we knew when the last mozWriteAudio call was made, we could write silence after that.

If we need a fix for this before that happens, maybe we can go with option 1.  Part of the API already exists, and is used, on AudioStream:  GetMinWriteSize.  That's used in AudioLoop to solve the same problem we're discussing here.  It's not currently exposed via the Audio Data API, though.

Most of these problems go away once we switch to using libcubeb instead of libsydneyaudio, so we might be better to focus on that if this doesn't require an urgent fix.

Comment 7

[Marco Chen [:mchen]]

Hi Matthew,

Thanks for your explanation here.
First, the frame I mean is about the circular buffer in audio backend.
Audio backend will maintain a buffer size X and split it to N frames . So each frame size will be X / N. Then as I knew that only a frame with full data will be played. This is the reason of the statement as below.

c. if first write is small to 870, start() will be called by patch but current code.
   Even though, according to first frame doesn't be filled full so there is no real play occurred.

---------------------------------------------

From my point of view, at least I want to solve the issue as below.

b. if first write is equal to 870, start() will be called by patch but current code. (Issue)

Because this will impact the performance of key tone from dialer. Currently keypad.js can't get frame/segment size so it directly write 1200 samples via mozWriteAudio(). This caused a blocking call about 80ms. So I suggest to solve the issue on 
  Trigging AudioTrack::start() when write size is bigger or equal to frame/segment size. And use IsPause flag to prevent re-entry.

Then at least I can suggest partner to modify keypad.js from writing 1200 samples to 435 (depending on their frame/segment size) each time.

Is this fine for current stage?
Thanks.

Comment 8

[viral [:viralwang]]

Attachment: patch v2

Here's updated patch to prevent we may lost the start function if the size of key tone is 870.
It can make sure we can play the key tone with size 870 in non-blocking way.

Comment 9

[viral [:viralwang]]

Attachment: patch v3

Comment 10

[viral [:viralwang]]

Comment on attachment 711735 [details] [diff] [review]
patch v3

updated the correct patch.

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 711735 [details] [diff] [review]
patch v3

Sorry to clobber your patch, but while investigating the bug so I understood, I ended up fixing a number of issues with the existing code.

Comment 12

[Matthew Gregan [:kinetik]]

Attachment: patch v4

I dug into this so I could understand the problem and ended up fixing it along the way.  Sorry to steal the bug!

There were a number of issues:

- Clock calculations from clock_gettime were overflowing (need to cast tv_sec to int64_t before converting to milliseconds).  I ended up removing the clock code entirely, because I found a better solution.
- get_write_size was completely broken.  Originally fixed by fixing the clock calculations, but I discovered it's possible to calculate it more accurately from the buffer size and playback position.
- bufferSize was initialized incorrectly (calculated buffer 1s, but AudioTrack initialized with frameCount (minimum buffer size)).

With this patch, mozWriteAudio no longer blocks, because get_write_size (AudioStream::Available) returns the correct value, and mozWriteAudio will now work in a non-blocking manner by performing short writes.

The issue where a write of exactly the same size as the AudioTrack's buffer (e.g. AudioTrack buffer size = 870, write 870) not starting is fixed by calling sa_stream_resume unconditionally.  This change removes the ability to "prefill" the entire buffer with multiple writes, but as I mentioned above this has effectively been disabled accidentally in the media decoder AudioLoop by calling Pause/Resume immediately after creating the stream, so we were never benefiting from that behaviour.

Most of these fixes need to be ported to the Android backend, too.  I'll file a new bug for that.

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: patch v4.1

Fix a typo in the debug-only logging.

Comment 14

[Marco Chen [:mchen]]

Comment on attachment 713241 [details] [diff] [review]
patch v4.1

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

I think this is a correct approach to get available buffer size and thanks for Matthew's help.
The patch is gave reviewed after clarifying questions.

::: media/libsydneyaudio/src/sydney_audio_gonk.cpp
@@ +169,5 @@
>      new AudioTrack(s->streamType,
>                     s->rate,
>                     AudioSystem::PCM_16_BIT,
>                     chanConfig,
> +                   s->bufferSize / s->channels / sizeof(int16_t),

Does this formula equal to frameCount?

@@ +244,5 @@
>      p += r;
>      wrote += r;
> +    s->amountWritten += r;
> +
> +    sa_stream_resume(s);

Do we need to check s->Pause for avoiding to call resume in multiple time?

Comment 15

[Matthew Gregan [:kinetik]]

Thanks for the review!

(In reply to Marco Chen [:mchen] from comment #14)
> Does this formula equal to frameCount?

Yes.  The reason I wrote it that way, rather than using frameCount directly, is that it is crucial that the AudioTrack's buffer size matches s->bufferSize.  This way, if the buffer size calculation is changed later, the correct value will be computed and passed in to the AudioTrack constructor.

> @@ +244,5 @@
> >      p += r;
> >      wrote += r;
> > +    s->amountWritten += r;
> > +
> > +    sa_stream_resume(s);
> 
> Do we need to check s->Pause for avoiding to call resume in multiple time?

I don't think it's required, but it does makes sense to check s->isPaused before calling sa_stream_resume since it's likely to be much cheaper.  I'll add that when I check in.

Comment 16

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #15)
> I don't think it's required, but it does makes sense to check s->isPaused
> before calling sa_stream_resume since it's likely to be much cheaper.  I'll
> add that when I check in.

I decided not to make this change because I already pushed the Android version of this patch without it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8475b801a4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a8475b801a4

Comment 18

[Marco Chen [:mchen]]

Issue: 
  Sydney backend didn't calculate the available audio buffer size correctly (available is always larger then real buffer size) and this caused mozAudioWrite() be a blocking function. So key tone on dialer app will be blocked on this function about 40ms (reference on unagi) (bug 832842). So it impacts the UI performance when user presses keys quickly.

Test:
  Test with patch on bug 832842 and mozAudioWrite() will become a non-blocking function and write enough audio data to fill backend buffer full.

Risk:
  Once backend buffer is larger then size writing by mozAudioWrite() then that audio will not be played because mozAudioXXX lacks the API to append silent data and flush buffer in the right timing.

  But 1. currently key tone frames number are 1200 
      2. and unagi backend buffer will be 870
  so there is no impact yet.

Media team plans to use cubeb backend and web audio to replace sydney backend & mozAudioWrite on V2.0.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking instead of blocking (same as with bug 832842) so please go ahead with an uplift nomination.