840311 - Gonk backend for sydneyaudio uses wrong buffer/latency size

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

The Android backend that the Gonk backend was based on queries the minimum buffer size and ensures the calculated buffer size is never less than that.  The Gonk backend queries the minimum buffer size, then uses that and ignores the calculated buffer size.

See http://mxr.mozilla.org/mozilla-central/source/media/libsydneyaudio/src/sydney_audio_gonk.cpp#163

frameCount is passed to the AudioTrack constructor, but the original intent is to pass s->bufferSize (converted back to frames).

The result of this is that we always construct an AudioTrack with a ~55ms buffer, rather than the intended ~1000ms buffer.  This is likely to result in more frequent wakeups to refill buffers, and more underruns during media playback.  It also means the calculations in get_write_size and drain are incorrect as they use s->bufferSize.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Comment 2

[Matthew Gregan [:kinetik]]

The problem with fixing this is that the Gaia dialer wants to play short (~150ms) sounds, but the mozWriteAudio API provides no way to detect how much the caller must write before playback begins.

If we increase the AudioTrack buffer size, either the mozWriteAudio needs to be extended to expose buffer sizing, or (as a hack) the dialer needs to write "enough" silence after the dialtone to force playback to begin.

Comment 3

[Marco Chen [:mchen]]

Hi Matthew,

Could we set different lentency based on stream types?
For example, if stream type is SYSTEM then we set it as minFrames. If MUSIC then we set it to more bigger size. Since audio data api will be deprecated and only dialer used it, maybe we could do this hack to cover this issue.

Comment 4

[Matthew Gregan [:kinetik]]

Yeah, that could work.

Comment 5

[Marco Chen [:mchen]]

Attachment: 4times_minFrames_for_music_stream_only

Hi Matthew,

As we discussed, I set minFrames to 4 times for music stream only.
(the code only show 2 times because the calculation of AudioTrack::getMinFrameCount already do twice of minFrames.)

Thanks.

Comment 6

[Matthew Gregan [:kinetik]]

s->bufferSize needs to reflect the buffer size passed to AudioTrack, otherwise calculations using s->bufferSize (e.g. get_write_size) will produce incorrect results.

So I'd make this something like:

   s->bufferSize = frameCount * s->channels * sizeof(int16_t);
   if (s->streamType == AudioSystem::MUSIC) {
     s->bufferSize *= 2;
   }

But rather than special casing MUSIC (which doesn't help general media elements that default to SYSTEM), can we special case RINGER to use the minimum buffer size and use a larger (2x or whatever's appropriate) one for every other type instead?

Comment 7

[Marco Chen [:mchen]]

Attachment: Patch v2

Hi Matthew,

Thanks for your comment here.

1. I checked that dialer app use normal channel for keypad tone so it is SYSTEM stream type after mapping to Android Audio Stream Type.

2. Following your suggestion, set minframes to 4 times but keep twice when stream type is SYSTEM.

Comment 8

[Matthew Gregan [:kinetik]]

Ah, I was confused, I thought the dialer was using "ringer", but it looks like that changed since I last looked.

The problem then is that the default for media elements is also "system", and we really want to apply this hack to the dialer and nothing else.

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 724301 [details] [diff] [review]
Patch v2

Based on my previous comment, I don't think we should do it this way.

Having thought about it some more, the goal here is to increase the buffer size for general audio playback without breaking the dialer (which uses mozWriteAudio), so another approach is to add a B2G specific change to nsHTMLAudioElement that enables the small buffer hack/workaround for streams created via mozSetup, and then increase the default buffer size.

Comment 10

[Marco Chen [:mchen]]

Hi Matthew,

Thanks for the review and suggestion.

So the goal here is
  1. enlarge the buffer size.
  2. Hack the mozWriteAudio path for padding the slience data to fill buffer full "on each call".
  
  My opinion:
  1. Based on different stream type to give difference min buffer size for reason of lantency.

On the other way, may I take this bug to fix it?
Thanks.

Comment 11

[Matthew Gregan [:kinetik]]

Just to clarify, is a fix for this bug targeted for B2G 1.x? If not, we can just switch to cubeb and close this out.  Otherwise, please do take the bug, thank you!

Comment 12

[Marco Chen [:mchen]]

The only reason now is Bug 842195 which is posted by leo.
And leo is on V1.1.

But currently it is not with leo+, I will ask there first.

Comment 13

[Marco Chen [:mchen]]

Attachment: Patch v3

Hi Matthew,

1. I hack mozWriteAudio API to fill the buffer full with silence data if writing size is small then avaliable size. (with MOZ_B2G)

2. In sydney_audio_gonk, I set frameCount to 1s in AudioTrack with music stream type and 4 times of minframes with others stream type (for short latency).

Thanks for your feedback.

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 726571 [details] [diff] [review]
Patch v3

Thanks, if this works well enough, let's go with it.  I'm a bit uneasy about adding these hacks, but we have a plan to get away from them after 1.x.

+#ifdef MOZ_B2G
+  uint32_t availableLen = mAudioStream->Available();
+  // Don't write more than can be written without blocking.
+  uint32_t writeLen = std::min(availableLen, dataLength / mChannels);
+#else
   // Don't write more than can be written without blocking.
   uint32_t writeLen = std::min(mAudioStream->Available(), dataLength / mChannels);
+#endif

For this part, just use the B2G bit on all platforms, i.e.:

  uint32_t availableLen = mAudioStream->Available();
  // Don't write more than can be written without blocking.
  uint32_t writeLen = std::min(availableLen, dataLength / mChannels);

...with no ifdef.

Comment 15

[Marco Chen [:mchen]]

Issue: sydney_audio_gonk set FrameCount as twice of min frames when creating AudioTrack. But there is a bug as below in AudioFlinger. (Please refer to Bug 842195)

  mixing thread will hold 1 more frame then twice of min frames, when source is needed to re-sample.

Solution: 
  1. Like what Android do, we set FrameCount to 4 times of min frames. And specially set frameCount to 1 seconds for matching the buffer from decoding thread.
  2. In mozAudioWrite(), we append slience data to fill the backend buffer full or once backend buffer size is more then size wrote by mozWriteAudio(). That will not be played. This is a lack on this seriels of APIs (Note that this API will be removed from master and instead by WebAudio and now only dialer - keypad.js uses it.)

Test:
  Tested it with music/FM/video and ring tone.

Risk: 
  It needs Bug 833724 & Bug 832842 too.

Comment 16

[Lucas Adamski [:ladamski]]

(In reply to Marco Chen [:mchen] from comment #15)
> Risk: 
>   It needs Bug 833724 & Bug 832842 too.

Does it need those bug or does it block them?  Seems like 832842 at least is blocked by this bug.

Comment 17

[Marco Chen [:mchen]]

1. Bug 842195 depends on this bug.

2. Bug 832842 depends on Bug 833724 & this bug.

Comment 18

[Marco Chen [:mchen]]

Attachment: Patch Checkin-Version for b2g18 branch

After discussing with Matthew, the check-in patch doesn't contain the part of hacking mozWriteAudio() because the frameCounts set by sydney backend is not larger then key tone frames.

Comment 19

[Marco Chen [:mchen]]

Hi Sheriff,

Thanks for your help to checkin first.

And according to sydney backend already be removed from m-c tree, I can't give the patch into try server or in inbound tree. So I tested by patch on b2g18 and b2g18-v1.0.1. Please help to check-in into b2g18 series branch only.

I don't meet this situation before, if I miss any rule please let me know.
Thanks.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7d3760018845