842195 - [A/V] Calcuation of buffer count that is used by an AudioTrack

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect, P2)

Description

[leo.bugzilla.gecko]

In function sa_stream_open() in /gecko/media/libsydneyaudio/src/Sydney_audio_gonk.cpp file, gecko just uses buffer count as many as minimum buffer count of AudioTrack.
Sometimes, it leads a buffer starving in AudioFlinger(AudioMixer).

In the AudioFlinger thread, everytime, it checks number of buffer count before mixing.
When a starving occur, AudioFlinger stopped because of this check.
(LOG_ASSERT(minFrames <= cblk->frameCount) routine in prepareTracks_l() in AudioFlinger.cpp.)

After I changed a routine of buffer count calculation, it works fine.
This is almost same calcuation routine as MediaPlayerService.cpp in Android.

############## before ##############
int frameCount;
if (AudioTrack::getMinFrameCount(&frameCount, s->streamType, s->rate) != NO_ERROR) {
  return SA_ERROR_INVALID;
}
############## after ##############
int afSampleRate;
int afFrameCount;

if (AudioSystem::getOutputFrameCount(&afFrameCount, s->streamType) != NO_ERROR) {
  return SA_ERROR_NO_INIT;
}
if (AudioSystem::getOutputSamplingRate(&afSampleRate, s->streamType) != NO_ERROR) {
  return SA_ERROR_NO_INIT;
}
int frameCount = (s->rate*afFrameCount*4)/afSampleRate;
###################################

The buffer count should be recalculated.
It can be reproduced by attached video clip.

Comment 1

[leo.bugzilla.gecko]

Attachment: video clip to reproduce this bug

Comment 2

[leo.bugzilla.gecko]

Attachment: video clip to reproduce this bug

Comment 3

[leo.bugzilla.gecko]

Attachment: video clip to reproduce this bug

Comment 4

[leo.bugzilla.gecko]

Attachment: video clip to reproduce this bug

Comment 5

[leo.bugzilla.gecko]

Becuase the video clip is more than 4MB,
I have uploaded video clip after I devided it into several archive.

Please use 7zip, if you have any problem to extract the video file.

Comment 6

[Marco Chen [:mchen]]

(In reply to leo.bugzilla.gecko from comment #0)
> In function sa_stream_open() in
> /gecko/media/libsydneyaudio/src/Sydney_audio_gonk.cpp file, gecko just uses
> buffer count as many as minimum buffer count of AudioTrack.
> Sometimes, it leads a buffer starving in AudioFlinger(AudioMixer).
> 
> In the AudioFlinger thread, everytime, it checks number of buffer count
> before mixing.
> When a starving occur, AudioFlinger stopped because of this check.
> (LOG_ASSERT(minFrames <= cblk->frameCount) routine in prepareTracks_l() in
> AudioFlinger.cpp.)
> 
> After I changed a routine of buffer count calculation, it works fine.
> This is almost same calcuation routine as MediaPlayerService.cpp in Android.
> 
> ############## before ##############
> int frameCount;
> if (AudioTrack::getMinFrameCount(&frameCount, s->streamType, s->rate) !=
> NO_ERROR) {
>   return SA_ERROR_INVALID;
> }

Please refer to the link as below.
https://www.codeaurora.org/gitweb/quic/la/?p=platform/frameworks/base.git;a=blob;f=media/libmedia/AudioTrack.cpp;h=140bd3547ec6e7f45b9cece8212089231e5834fd;hb=refs/heads/b2g/ics_strawberry_v1#l53

AudioTrack::getMinFrameCount(...) already did what you suggested here. The only difference is the value of minBufCount. Proposal set it as a fixed number 4 but calculate it from latency is 2. I think the starvation is not caused by the calculation of frame count but the decoding speed.

So I guess it seems that the proposal here tried to enlarge the frame counts so sydney can write more data in once and let decoding thread have more time to decode. Will try to verify it.

Comment 7

[leo.bugzilla.gecko]

Could you please update progress?

Comment 8

[Marco Chen [:mchen]]

(In reply to leo.bugzilla.gecko from comment #7)
> Could you please update progress?

I think this is a bug on Android Audio Flinger not Gecko. In audio flinger's mixing thread it will hold a two times of mFrameCount (the maximum value) for mixing purpose. And the formula listed as below will cause two times of minFrames is bigger then cblk->frameCount (in default it is two times of mFrameCount too.)

minFrames = (mFrameCount * t->sampleRate()) / mSampleRate + 1 + 1;

So the assertion will be happened but actually it is harmless in release build.
And that is why the four times of mFrameCount can avoid this assert.

Comment 9

[leo.bugzilla.gecko]

(In reply to Marco Chen [:mchen] from comment #8)
> (In reply to leo.bugzilla.gecko from comment #7)
> > Could you please update progress?
> 
> I think this is a bug on Android Audio Flinger not Gecko. In audio flinger's
> mixing thread it will hold a two times of mFrameCount (the maximum value)
> for mixing purpose. And the formula listed as below will cause two times of
> minFrames is bigger then cblk->frameCount (in default it is two times of
> mFrameCount too.)
> 
> minFrames = (mFrameCount * t->sampleRate()) / mSampleRate + 1 + 1;
> 
> So the assertion will be happened but actually it is harmless in release
> build.
> And that is why the four times of mFrameCount can avoid this assert.

Thanks for your reply.
A two time of mFrameCount is right in theory. But, For reading and writing shared memory, latency is happened between gecko and audioflinger. 

This below code is the android original source. Android changed to 4 times values for preventing underrun. this value is optimized by the process of fine tuning. 
-------------------------------------------------------------
TODO: Find real cause of Audio/Video delay in PV framework and remove this workaround
/* static */ int MediaPlayerService::AudioOutput::mMinBufferCount = 4;

status_t MediaPlayerService::AudioOutput::open(
        uint32_t sampleRate, int channelCount, int format, int bufferCount,
        AudioCallback cb, void *cookie)
{
    mCallback = cb;
    mCallbackCookie = cookie;
    // Check argument "bufferCount" against the mininum buffer count
    if (bufferCount < mMinBufferCount) {
        bufferCount = mMinBufferCount;
    }
    if (mTrack) close();
    int afSampleRate;
    int afFrameCount;
    int frameCount;

    if (AudioSystem::getOutputFrameCount(&afFrameCount, mStreamType) != NO_ERROR) {
        return NO_INIT;
    }
    if (AudioSystem::getOutputSamplingRate(&afSampleRate, mStreamType) != NO_ERROR) {
        return NO_INIT;
    }
    frameCount = (sampleRate*afFrameCount*bufferCount)/afSampleRate;
------------------------------------------------------
LOG_ASSERT(minFrames <= cblk->frameCount);
This condition`s mismatch can hears some strange sounds according to environment.

Comment 10

[Marco Chen [:mchen]]

Thanks for the information here.

I think it is worth do adjusting the AudioTrack's minFrames by the stream types.
Ex: system type will be 2 times for short lentency but music type will be 4 times of minFrameSize.

It is easy to fix on sydney but the cubed will replace it.
So I am looking into cubeb_opensl.c for how to adjust min frame size via openSL ES API.

Comment 11

[Matthew Gregan [:kinetik]]

I ran into this a little while ago.  See bug 840311.  It's a bit painful to fix because apps using the Audio Data API on B2G now rely on the small audio buffer.

Switching from sydneyaudio to cubeb makes this easier, and switching the B2G apps from the Audio Data API to the Web Audio API will make this problem disappear.

cubeb should be honouring the latency argument to cubeb_stream_init and using a buffer size computed from that rather than the minimum.  See http://hg.mozilla.org/mozilla-central/file/324985f4c4ea/media/libcubeb/src/cubeb_opensl.c#l223

Comment 12

[leo.bugzilla.gecko]

Do you have plan to change syndney audio to cubeb?
If so, please let me know the schedule.

And is it right that cubeb is using wilhelm in android?

Comment 13

[Matthew Gregan [:kinetik]]

Yes, that's the plan.  The switch for B2G is tracked in bug 848581.  It'll land on trunk as soon as the prerequisite fixes have landed.

If wilhelm is OpenSL, then yes, for B2G and Android 2.3 upwards.  Android 2.2 will be using the AudioTrack backend implemented in bug 698328.

Comment 14

[Marco Chen [:mchen]]

Hi leo,

Will you put this bug into leo+?
It yes, we can just evealute whether we enable cubeb + OpenSL backend or fix it on the Sydney + AudioTrack backend.

Thanks.

Comment 15

[leo.bugzilla.gecko]

I cannot set this but into leo+.
Did I miss something?

Comment 16

[Marco Chen [:mchen]]

(In reply to leo.bugzilla.gecko from comment #15)
> I cannot set this but into leo+.
> Did I miss something?

I don't get your point? Currently we need the progress of leo? -> leo+ then we can land the fixing into appropirate branch. Or even we fixed but can't land.

So please eavluate the servirity of this issue then ask for leo? .

Comment 17

[leo.bugzilla.gecko]

When I try to change this bug into leo+.
Bugzilla shows message like below.

Bugzilla has suffered an internal error:
The function Bugzilla::Flag->set_flag requires a id/type_id argument, and that argument was not set.

Comment 18

[Marco Chen [:mchen]]

Hi leo,

You can set to leo? only and which will be discussed by Mozilla to +.

Comment 19

[leo.bugzilla.gecko]

Could you please share the release schedule of this cobeb?
I just want to know which version I can get patch for sydney -> cobeb.

Comment 20

[Marco Chen [:mchen]]

Hi leo,

The cubeb is decoded to enable on V2.0 not V1.X so Bug 840311 is fired to solve this on sydney backend.

Comment 21

[leo.bugzilla.gecko]

Is it reviewed and included in v1.1??

Comment 22

[Marco Chen [:mchen]]

(In reply to leo.bugzilla.gecko from comment #21)
> Is it reviewed and included in v1.1??

Yes, it is reviewd already. And it nees leo+ approval to land on appropirate branch.

Comment 23

[leo.bugzilla.gecko]

Could you please share the schedule to release the patch related to this issue?

Comment 24

[Marco Chen [:mchen]]

Hi leo,

Bug 840311 is already landed into b2g18 branch already.