Marco Chen [:mchen] Reporter
	Description • 11 years ago

Refer to Bug 924383.

The audio playback is a key application on mobile platform so how to guarantee it's performance is important to satisfy user experience. 

Current Situation:

Currently the nice & priority of threads in the background will be set to lower then foreground app. And background app playing audio will be set between foreground & background app.

The problem is

  When foreground is busy, the music player in the background is easy to be noticed about underrun issue.

The rough idea

  ProcessPriorityManager will keep threads related to media playback to equal or higher then foreground app no matter they are in the background or foreground.

	Marco Chen [:mchen] Reporter
	Comment 1 • 11 years ago

The first implementation plan is

1. When creating threads related to media playback, it should register it's thread ID into a white list in content process.

2. Before changing the priority of a process and it's threads, parent process will ask content to provide white list then keep priority in the white list to equal or larger then foreground app.

	Star Cheng [:scheng]
	Comment 2 • 11 years ago

Hi, Kyle

To improve the user experience for audio playback, we want to maintain a white list to guarantee audio thread can get higher priority. Could you give some suggestion to the idea or any concern about that?

	Thinker Li [:sinker]
	Comment 3 • 11 years ago

Do you ever try to increase the size of the output buffer?  I think most problems of this type can be improve by adjusting buffering factors and algorithm.  I means if the root cause is about latency of decoding, not the through-put of the CPU.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 4 • 11 years ago

Yes, you should consider what Thinker said in comment 3.  If throughput really is the problem, you can manipulate the thread priority via nsISupportsPriority (which nsIThread will QueryInterface to).  Without reading the pthreads documentation I'm not sure how thread priorities interact with process priorities.  You should be careful with thread priorities too, e.g. http://www.codinghorror.com/blog/2006/08/thread-priorities-are-evil.html

	Marco Chen [:mchen] Reporter
	Comment 5 • 11 years ago

Hi Thinker and Kyle,

Thanks for your consultant in advance.

To increase Audio Latency (mapped to deep buffer) can't solve this kind of issue.

Consider that

  1. Playing a music app in the background.
  2. Sync accounts from facebook in the foreground.

In this case the foreground process for syncing accounts will take almost cpu resources and long time, so the deep buffer can just solve the pulse loading from foreground or B2G processes but not this case.

	Marco Chen [:mchen] Reporter
	Comment 6 • 11 years ago

Hi Thinker and Kyle,

May I know any other concern/suggestion from you?
We also discussed with Chris Pearce during media work week and he thought this makes sense but need to discuss with B2G experts too.

Or we can try to implement it then see the result.

Thanks.

	Thinker Li [:sinker]
	Comment 7 • 11 years ago

Hi Macro, I suggest to change the priority value manually for experiments before starting implementation.  I am not sure if the shell of gonk is enough for changing priority.  But, you still could attach the process with gdb to change priority manually.

	Marco Chen [:mchen] Reporter
	Comment 8 • 11 years ago

Refer to the STR from Bug 871431 and I roll back the nice value of BACKGROUND_PERCEIVABLE to more lower (11) for easy to reproduce.
  STR:
    1. Play an OGG audio in the background.
    2. Lunch a browser to surf complex web site (ex: tw.yahoo.com).
    3. Listen the quality of music in the background.

  Actually result: During loading the page, the audio output of music will be intermittent.

Experiment:
  1. Attach the b2g process by GDB and call setpriority() to adjust media threads.
  2. To adjust nice value of threads below to 1 (same as foreground)
                                  PRIO  NICE RTPRI  SCHED  
app_1082  1099  1082  81968  33308 21    1     0     0     c00dc6c0 401356ec S Media State
app_1082  1300  1082  81968  33308 21    1     0     0     c00dc6c0 401356ec S Media Decode
app_1082  1305  1082  81968  33308 21    1     0     0     c00dc6c0 401356ec S Media Audio
app_1082  1306  1082  81968  33308 21    1     0     0     c00dc6c0 401356ec S AudioTrackThrea

  Then the issue is gone, music can be played well without any interrupt.

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

For HW decoding (mp3)

  The actually codec threads are running on the media server which is another process outside the B2G control. Refer to their threads's nice values. All the them are lower or equal to 1.

media     907   899   26480  5320  -2    -19   1     2     c00788c0 400f1594 S AudioOut_1
media     908   899   26480  5320  20    0     0     0     c03d4c10 400f17b0 D Binder Thread #
media     1223  899   26480  5320  20    0     0     0     c03d4b1c 400f17b0 S Binder Thread #
media     1405  899   26480  5320  21    1     0     0     c00dc6c0 400f26ec S mediaserver
media     1409  899   26480  5320  18    -2    0     0     c00dc6c0 400f26ec S OMXCallbackDisp

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

Actually this mechanism was already exist in FxOS (media server), so I proposed to do it in the B2G as well.

	Marco Chen [:mchen] Reporter
	Comment 9 • 11 years ago

Hi Thinker,

Could this experiment prove the benefit of this bug?
Thanks.

Hi Chris,

Bruce Sun discussed with you about this.
And may I know your suggestion from media point of view? Thanks.

	Thinker Li [:sinker]
	Comment 10 • 11 years ago

It is good to me.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 11 • 11 years ago

This seems reasonable to me.

Sotaro: do you think the approach suggested in comment 0 is reasonable?

Marco: Another thing you could also try increasing AMPLE_AUDIO_USECS in MediaDecoderStateMachine to a large value (say 2x, 5x, 10x, 20x, ... larger). This number controls how much decoded audio we buffer in memory. Unfortunately if we buffer too much we'll fill up memory. We'd need about 192,000 bytes per second of decoded audio PCM samples buffered on ARM CPUs (48000Hz, 2 channel, int16 samples). So we'd need heuristics to ensure we only increased the number of decoded samples buffered when the media was playing in the background.

	Marco Chen [:mchen] Reporter
	Comment 12 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #11)
> Marco: Another thing you could also try increasing AMPLE_AUDIO_USECS in
> MediaDecoderStateMachine to a large value (say 2x, 5x, 10x, 20x, ...
> larger). This number controls how much decoded audio we buffer in memory.

Hi Chris,

Thanks for the advice.

And your suggest is the same concept with comment 3 from Thinker. (increase buffer for decoded PCM data)
But this kind of solution can solve the pulse loading but can't solve the constant heavy loading from fg process.

	Sotaro Ikeda [:sotaro]
	Comment 13 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #11)
> This seems reasonable to me.
> 
> Sotaro: do you think the approach suggested in comment 0 is reasonable?

It is reasonable. Only my concern to the approach is running sw video codec in background app. IIRC, current b2g do not have this use case. For a video tag, it is banned in b2g for the time being. But I am not sure about WebRTC or in future.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 14 • 11 years ago

Yeah, we should and try it and see what happens.  It will degrade foreground app performance, of course.

	Dietrich Ayala (:dietrich)
	Comment 15 • 11 years ago

Joe, considering the demo scripts this should be a 1.3t blocker.

Hi Star - have you begun working on this? Any progress?

	Marvin Khoo [:Marvin_Khoo]
	Comment 16 • 11 years ago

Not sure if this affected. On Tarako, we should have music playback in background and not being killed as acceptance criteria. furthermore, the quality / performance of the playback is yet to be check, the playback shall be smooth enough and not choppy. let's review this.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 17 • 11 years ago

I tested it today in the room and the audio performance is pretty choppy when the gallery is started or a phone call is received.

	Star Cheng [:scheng]
	Comment 18 • 11 years ago

(In reply to Dietrich Ayala (:dietrich) from comment #15)
> Joe, considering the demo scripts this should be a 1.3t blocker.
> 
> Hi Star - have you begun working on this? Any progress?

There is not prototype to demo yet. According to offline discussion with Marco & Alan, the todolist as below followings:

ToDoList:
1.make sure is there any available sys node /proc/pid/xxxx/mediaTag (True/False)
2.confirm the timing of invloking ipdl of funtion

	Star Cheng [:scheng]
	Comment 19 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #17)
> I tested it today in the room and the audio performance is pretty choppy
> when the gallery is started or a phone call is received.

I update the SW. (The SW info as below followings:)

Environmental Variables:
Device: Tarako
BuildID: 20140214123908
Version: 28.0

And I tested 2cases, the scenarios are:

[case1]
Step 1: Open music app and press home key to make music app playing in background.
Step 2: Open Gallery (some pictures in the SD card)

[case2]
Step 1: Open music app and press home key to make music app playing in background.
Step 2: Open Settings app.

There was not choppy to the music audio stream. Music playing smoothly.

	Dietrich Ayala (:dietrich)
	Comment 20 • 11 years ago

I'm also seeing pretty good performance in the latest builds. However, I have heard that some apps like Email are exhibiting choppy audio.

We should test under heavy workload for Email and Messages app, if those apps are important for Tarako market.

Otherwise should also test while running the popular social apps.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 21 • 11 years ago

Yes, the performance issues were greatly improved by the GC suppression patch.  My comment was from Monday ... I should have come back and updated the bug.

	Star Cheng [:scheng]
	Comment 22 • 11 years ago

At first, we wanted to provide a workaround solution in 1.3t branch. The concept is to increase MusicApp in background process priority(ie. decrease nice value from 18 to 7 or lower). So I modified the below code to set this mozapptype as critical type:

config.url.startsWith(window.location.protocol +'//music.gaiamobile.org') in the function (@gaia/apps/system/js/browser_frame.js ::setMozAppType)

Per my investigation, the nice value of music app in background is 7 in the latest tarako builds. Despite I use the workaround solution, the nice value is 7 (music app in background). So I don't consider that this workaround solution can help any more. I suggestion that don't set 1.3t+. 

And could anyone know where is the solution? Why the latest build (maybe not? maybe land to center branch for a long time) can make the nice value of music app in bg is 7 instead of 18. Because I am going on implement for this bug, I want to reference it.

	Gabriele Svelto [:gsvelto]
	Comment 23 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> Yes, you should consider what Thinker said in comment 3.  If throughput
> really is the problem, you can manipulate the thread priority via
> nsISupportsPriority (which nsIThread will QueryInterface to).  Without
> reading the pthreads documentation I'm not sure how thread priorities
> interact with process priorities.

We should be careful with that as we're already globally adjusting thread priorities relatively to the process' priority, see bug 861434, bug 879529 and the code here:

http://hg.mozilla.org/mozilla-central/file/6687d299c464/hal/gonk/GonkHal.cpp#l1335

Besides Linux POSIX thread implementation does not allow changing a thread priority with the normal interfaces; if you want to adjust a thread's nice value you have to jump through a bunch of hoops, check some of the horror in bug 841651.

(In reply to Star Cheng [:scheng] from comment #22)
> And could anyone know where is the solution? Why the latest build (maybe
> not? maybe land to center branch for a long time) can make the nice value of
> music app in bg is 7 instead of 18. Because I am going on implement for this
> bug, I want to reference it.

It should be 7 as that corresponds to the BACKGROUND_PERCEIVABLE priority level which should always be set for applications playing audio. If that's not the case then we must have introduced a regression in the process priority manager or something else feeding it the information it uses to adjust priorities.

	Star Cheng [:scheng]
	Comment 24 • 11 years ago

(In reply to Dietrich Ayala (:dietrich) from comment #20)
> I'm also seeing pretty good performance in the latest builds. However, I

According to the comment 21, the GC suppression patch improves the performance. Before patching the GC suppression, it's very choppy to play music in bg under opening any app in foreground (I didn't observe this phenomenon in hamachi device in the latest build). And this phenomenon obviously occurs in Tarako. It should be Tarako-specific issue. 

> have heard that some apps like Email are exhibiting choppy audio.
> 
> We should test under heavy workload for Email and Messages app, if those
> apps are important for Tarako market.
> 
> Otherwise should also test while running the popular social apps.

And this is implemented for general case/general device to improve user experience not for 1.3t only, and it can't be guaranteed to solve any very heavy workload case that someone can create.

I tested it(play music in bg under using Message app) with latest build. There is not choppy phenomenon except attach some big size pictures. If these cases are concerned, maybe to file another bug for 1.3t.

	Ben Kelly [:bkelly, not reviewing]
	Comment 25 • 11 years ago

I believe my patches in bug 980027 will resolve this issue.  They permit the libmedia threads to stay at their elevated priority.

	Sotaro Ikeda [:sotaro]
	Comment 26 • 11 years ago

(In reply to Ben Kelly [:bkelly] from comment #25)
> I believe my patches in bug 980027 will resolve this issue.  They permit the
> libmedia threads to stay at their elevated priority.

I do not thinks so. This bug is basically handling the problem of audio related threads' priorities in an application process. To fix the problem, need to raise the audio decoding thread to some level in application process.

I am attending in media playback/recording work week in Taipei. And talked a little bit about real-time thread's priority. Almost responses are basically not good about using RT priority.

	Ben Kelly [:bkelly, not reviewing]
	Comment 27 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #26)
> I do not thinks so. This bug is basically handling the problem of audio
> related threads' priorities in an application process. To fix the problem,
> need to raise the audio decoding thread to some level in application process.
> 
> I am attending in media playback/recording work week in Taipei. And talked a
> little bit about real-time thread's priority. Almost responses are basically
> not good about using RT priority.

Have you looked at what the patches do now?

1) RT priority is pref'd off, but available as an option for future tuning.
2) It prevents gecko's process priority manager from overriding the higher priority set by libmedia on its decoding threads.  So they properly stay at ANDROID_PRIORITY_AUDIO.
3) They create a framework which lets gecko threads do hal::SetCurrentThreadPriority(THREAD_PRIORITY_<CAPABILITY>).  Right now I just have THREAD_PRIORITY_COMPOSITOR, but this is intended to support things like THREAD_PRIORITY_WEBAUDIO.

So to repeat, that bug is no longer enabling RT priority anywhere.

If you have a comment on the new patches, please make it on bug 980027 soon.  I would also really like to see what your alternative solution looks like.

	Sotaro Ikeda [:sotaro]
	Comment 28 • 11 years ago

Sorry, I was my misunderstanding. The comment was already obsolete.

	Sotaro Ikeda [:sotaro]
	Comment 29 • 11 years ago

It might be better to get a feed back from :padenot and :cpearce if get a feedback to around thread priority related media.

	Marco Chen [:mchen] Reporter
	Comment 30 • 11 years ago

(In reply to Ben Kelly [:bkelly] from comment #27)
> (In reply to Sotaro Ikeda [:sotaro] from comment #26)
> > I do not thinks so. This bug is basically handling the problem of audio
> > related threads' priorities in an application process. To fix the problem,
> > need to raise the audio decoding thread to some level in application process.
> > 
> > I am attending in media playback/recording work week in Taipei. And talked a
> > little bit about real-time thread's priority. Almost responses are basically
> > not good about using RT priority.
> 
> Have you looked at what the patches do now?
> 
> 1) RT priority is pref'd off, but available as an option for future tuning.
> 2) It prevents gecko's process priority manager from overriding the higher
> priority set by libmedia on its decoding threads.  So they properly stay at
> ANDROID_PRIORITY_AUDIO.

Hi Ben,

It seems that the patch there tried to keep priority of threads in libmedia.
And this is for OMX decoding only.

What we want to do here is to adjust related media threads running for SW decoding (ex: opus, vorbis).
Which didn't be counted into libmedia's magic.

Hi Sotaro,

One thing I want to clarify is that initial goal of this bug is to change the nice value as the same with foreground app not change scheduling policy. Even more we can considered to schedule_fifo which is the same one with threads in libmedia.

	Ben Kelly [:bkelly, not reviewing]
	Comment 31 • 11 years ago

(In reply to Marco Chen [:mchen] from comment #30)
> It seems that the patch there tried to keep priority of threads in libmedia.
> And this is for OMX decoding only.
> 
> What we want to do here is to adjust related media threads running for SW
> decoding (ex: opus, vorbis).
> Which didn't be counted into libmedia's magic.

Understood.  I think the groundwork from bug 980027 will provide a path to accomplish this.  We just first need to make the new hal::SetCurrentThreadPriority() work from child processes.  This is bug 982325.

Once that is fixed we can work update the other media thread priorities in this bug.

	Ivan Tsay (:ITsay)
	Comment 32 • 11 years ago

Move this bug to backlog with priority.