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

RESOLVED FIXED

Status

Firefox OS
Gaia::Video
P2
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: leo.bugzilla.gecko, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 714991 [details]
video clip to reproduce this bug
(Reporter)

Comment 2

5 years ago
Created attachment 714992 [details]
video clip to reproduce this bug
(Reporter)

Comment 3

5 years ago
Created attachment 714993 [details]
video clip to reproduce this bug
(Reporter)

Comment 4

5 years ago
Created attachment 714994 [details]
video clip to reproduce this bug
(Reporter)

Comment 5

5 years ago
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

5 years ago
(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.
(Reporter)

Comment 7

5 years ago
Could you please update progress?

Comment 8

5 years ago
(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.
(Reporter)

Comment 9

5 years ago
(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

5 years ago
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.
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
(Reporter)

Comment 12

5 years ago
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?
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

5 years ago
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.
Flags: needinfo?(leo.bugzilla.gecko)
(Reporter)

Updated

5 years ago
Flags: needinfo?(leo.bugzilla.gecko)
(Reporter)

Comment 15

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

Comment 16

5 years ago
(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? .
(Reporter)

Comment 17

5 years ago
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

5 years ago
Hi leo,

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

Updated

5 years ago
Flags: needinfo?(leo.bugzilla.gecko)
(Reporter)

Comment 19

5 years ago
Could you please share the release schedule of this cobeb?
I just want to know which version I can get patch for sydney -> cobeb.
Flags: needinfo?(leo.bugzilla.gecko)
(Reporter)

Updated

5 years ago
Flags: needinfo?(leo.bugzilla.gecko)

Comment 20

5 years ago
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.
Depends on: 840311
(Reporter)

Comment 21

5 years ago
Is it reviewed and included in v1.1??
Flags: needinfo?(leo.bugzilla.gecko)

Comment 22

5 years ago
(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.
(Reporter)

Comment 23

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

Comment 24

5 years ago
Hi leo,

Bug 840311 is already landed into b2g18 branch already.
(Reporter)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.