Closed Bug 1104643 Opened 5 years ago Closed 5 years ago

Refine the audio clock on B2G platform.

Categories

(Core :: Audio/Video, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

Details

(Whiteboard: [backout-asap])

Attachments

(1 file, 3 obsolete files)

When I was debugging for bug 1102047 (play a 360p video clip), I print some logs in Advanceframe() runs on StateMachine thread.
I found that the clock time returned by GetAudioClock() doesn't increase by every Advanceframe() iteration. That makes the StateMachine thread do nothing waste cpu power because the clock time doesn't go forward.

The audio clock increase here:
AudioTrackShared.cpp ServerProxy::releaseBuffer
which controlled by another process 
|media     197   1     29112  7304  ffffffff b6f3f6e4 S /system/bin/mediaserver|
And the ServerProxy::releaseBuffer usually increase the audio clock by 500~900 audio frames (11ms~20ms under 44100Hz)

Based on the observation, that means the audio clock won't be updated during the period (11~20ms). We can do something to improve the playback performance.

For example:
1. Use system clock instead audio clock?
2. Hybrid the system clock and audio clock?
3. Add a notification when the audio clock be updated? (Now we pull the audio clock)
Now I'm considering to modify the AudioStream::GetPositionInFramesUnlocked and AudioStream::DataCallback
because the cubeb_stream_get_position returns a cached audio clock.
Assignee: nobody → bechen
Maybe we can try to interpolate the audio clock in the b2g process using a system clock? I know some audio stacks do this for this exact reason (pulse in particular).

We prefer to use the audio clock to prevent drifts, but interpolating would not cause drifts.
Do you mean to change gonk or Android code? Since we can't interpolate if we don't know when ServerProxy::releaseBuffer happens.
Change gecko. We could experiment with synthesizing a clock from the callbacks buffer size, and interpolating with a system clock.

Pseudo-code:

> cubeb_stream {
>  .... (all the other fields)
>  /* number of frames since the start of the stream */
>  long clock_time;
>  /* udpated at each callback */
>  timestamp last_clock_update_timestamp;
> }
> 
> /* this is the refill callback called by opensl */
> void refill_callback(void * audio_buffer, long frames, void * stm)
> {
>   stm->clock_time += frames;
>   stm->last_clock_update_timestamp = timestamp();
> }
> 
> long opensl_get_current_position(cubeb_stream * stm)
> {
>   uint32_t now = timestamp();
>   return convert_to_frames(now - stm->last_clock_update_timestamp) + stm->clock_time + latency;
> }
Does that assume when data callback is received, all frames passed to previous data callbacks are already consumed/played? Since opensl can accetp multiple buffer queues, I am not sure if this assumption still holds.
Attached patch bug-1104643.v01.patch (obsolete) — Splinter Review
The idea of the patch is that: if we get the same value from mAudioStream->GetPositionInFramesUnlocked(), then I use system_clock to compensate the audio_clock.
Attachment #8558361 - Flags: feedback?(jwwang)
Comment on attachment 8558361 [details] [diff] [review]
bug-1104643.v01.patch

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

This could result in audio time fluctuation if we over-compensate frames.

::: dom/media/AudioStream.cpp
@@ +1216,5 @@
>  }
>  
> +int64_t AudioClock::ComputeCompensativePosition(TimeDuration& aDuration)
> +{
> +  int64_t deltaFrames = FrameHistory::UsToFrames<int64_t>(

You have to consider track rate change. Not all deltaFrames are played in the same rate.
Attachment #8558361 - Flags: feedback?(jwwang) → feedback-
Comment on attachment 8558361 [details] [diff] [review]
bug-1104643.v01.patch

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

::: dom/media/AudioStream.cpp
@@ +1217,5 @@
>  
> +int64_t AudioClock::ComputeCompensativePosition(TimeDuration& aDuration)
> +{
> +  int64_t deltaFrames = FrameHistory::UsToFrames<int64_t>(
> +                          (int64_t)aDuration.ToMicroseconds(), mOutRate);

Do you mean the |mOutRate| might be changed during the (N)th compensation and (N+1)th compensation?
If it is, maybe I should pass the |TimeDuration aDuration| into |FrameHistory::ComputeCompensativePosition| so that I can use the rate in FrameHistory::Chunk.
Attached patch bug-1104643.v02.patch (obsolete) — Splinter Review
Move the compensation logic to cubeb_opensl.c::opensl_stream_get_position.
Attachment #8558361 - Attachment is obsolete: true
Attachment #8559045 - Flags: feedback?(jwwang)
You probably want to get Matthew Gregan and/or Paul Adenot's feedback on this patch.
Comment on attachment 8559045 [details] [diff] [review]
bug-1104643.v02.patch

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

::: media/libcubeb/src/cubeb_opensl.c
@@ +687,5 @@
>    return CUBEB_OK;
>  }
>  
> +static int64_t lastPosition = -1;
> +static int64_t lastPositionTimeStamp = 0;

It won't work in multi-threaded environment.
Attachment #8559045 - Flags: feedback?(jwwang) → feedback-
Attached patch bug-1104643.v03.patch (obsolete) — Splinter Review
Attachment #8559045 - Attachment is obsolete: true
Attachment #8564884 - Flags: feedback?(padenot)
Attachment #8564884 - Flags: feedback?(jwwang)
Benjamin, do we have numbers about the resolution of clock_gettime(CLOCK_MONOTONIC, ...); on Android? My concern is that is the clock is not regular enough, we'll end up not improving, here. I know there are other ways to get a clock on Android, but maybe it's not needed.

I think just running a modified gecko with a tight loop that calls `clock_gettime(CLOCK_MONOTONIC, ...);`, checking the delta values would be enough.
Flags: needinfo?(bechen)
Thanks for your reminder, I'll restart the bug in a few days, maybe next week.
Flags: needinfo?(bechen)
(In reply to Paul Adenot (:padenot) from comment #13)
> Benjamin, do we have numbers about the resolution of
> clock_gettime(CLOCK_MONOTONIC, ...); on Android? My concern is that is the
> clock is not regular enough, we'll end up not improving, here. I know there
> are other ways to get a clock on Android, but maybe it's not needed.
> 
> I think just running a modified gecko with a tight loop that calls
> `clock_gettime(CLOCK_MONOTONIC, ...);`, checking the delta values would be
> enough.

I just modify the SandboxFilter.cpp to allow the clock_getres syscall on my flame device.
And the resolution of CLOCK_MONOTONIC is 1ns, CLOCK_MONOTONIC_COARSE is 10000000ns (10ms).
So the resolution of CLOCK_MONOTONIC is quite precise on Android and B2G platform.
Attachment #8564884 - Flags: review?(padenot)
Attachment #8564884 - Flags: feedback?(padenot)
Attachment #8564884 - Flags: feedback?(jwwang)
Attachment #8564884 - Flags: feedback-
Attachment #8564884 - Flags: feedback-
Comment on attachment 8564884 [details] [diff] [review]
bug-1104643.v03.patch

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

That should be fine (but see comments), thanks. Still, it would be nice to know the actual precision of the clock.

::: media/libcubeb/src/cubeb_opensl.c
@@ +70,5 @@
>    cubeb_resampler * resampler;
>    unsigned int inputrate;
>    unsigned int outputrate;
>    unsigned int latency;
> +#if defined(__ANDROID__)

Those #ifdefs are useless, this file is only compiled on android and b2g anyways.

@@ +512,5 @@
>    stm->inputrate = stream_params.rate;
>    stm->latency = latency;
>    stm->stream_type = stream_params.stream_type;
>    stm->framesize = stream_params.channels * sizeof(int16_t);
> +#if defined(__ANDROID__)

Same things.

@@ +708,5 @@
>    res = (*stm->play)->GetPosition(stm->play, &msec);
>    if (res != SL_RESULT_SUCCESS)
>      return CUBEB_ERROR;
>  
> +#if defined(__ANDROID__)

Same.
Attachment #8564884 - Flags: review?(padenot) → review+
(In reply to Paul Adenot (:padenot) from comment #16)
> Comment on attachment 8564884 [details] [diff] [review]
> bug-1104643.v03.patch
> 
> Review of attachment 8564884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That should be fine (but see comments), thanks. Still, it would be nice to
> know the actual precision of the clock.
> 

Here is the result of my flame device to run a while-loop 10 times.

                                            diff
I/Cubeb_OpenSL(20878): i 0 tr 25605677
I/Cubeb_OpenSL(20878): i 1 tr 25750104      144427
I/Cubeb_OpenSL(20878): i 2 tr 25936250      186146
I/Cubeb_OpenSL(20878): i 3 tr 26098958      162708
I/Cubeb_OpenSL(20878): i 4 tr 26223177      124219
I/Cubeb_OpenSL(20878): i 5 tr 26330260      107083
I/Cubeb_OpenSL(20878): i 6 tr 26435260      105000
I/Cubeb_OpenSL(20878): i 7 tr 26537812      102552
I/Cubeb_OpenSL(20878): i 8 tr 26641093      103281
I/Cubeb_OpenSL(20878): i 9 tr 26744062      102969

So the diff is about 0.102ms ~ 0.186ms
This patch breaks the assertion MOZ_ASSERT(position >= mLastGoodPosition, "cubeb position shouldn't go backward"); in AudioStream::GetPositionInFramesUnlocked().

For example:
At the first iteration: 
|msec| is 0 from GetPosition(stm->play, &msec), and the |maximum_position| is 10.

At the second iteration:
|msec| is still 0, so I can interpolate the time by system time to 7 (0~10).

At the third iteration:
|msec| is 5 from GetPosition(stm->play, &msec), and the |maximum_position| is 15.

5 < 7 hit the assertion.
Address comment 16, comment 18.
Attachment #8564884 - Attachment is obsolete: true
Attachment #8601418 - Flags: review?(padenot)
Comment on attachment 8601418 [details] [diff] [review]
bug-1104643.v04.patch

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

::: media/libcubeb/src/cubeb_opensl.c
@@ +739,5 @@
>        unadjusted_position : maximum_position;
>    } else {
>      *position = 0;
>    }
> +  stm->lastCompensativePosition = *position;

Weird name, but it's apparently correct english :-)
Attachment #8601418 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/7a7035e9ebfc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
We might need this landing backed out since it is causing bug 1135963.
QA Whiteboard: [QAnalyst-Triage+]
Whiteboard: [backout-asap]
Comment on attachment 8601418 [details] [diff] [review]
bug-1104643.v04.patch

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

::: media/libcubeb/src/cubeb_opensl.c
@@ +711,5 @@
>      return CUBEB_ERROR;
>  
> +  struct timespec t;
> +  clock_gettime(CLOCK_MONOTONIC, &t);
> +  if(stm->lastPosition == msec || msec < stm->lastCompensativePosition) {

Now the code flow always go into this if condition because the |lastCompensativePosition| is not correct.

@@ +739,5 @@
>        unadjusted_position : maximum_position;
>    } else {
>      *position = 0;
>    }
> +  stm->lastCompensativePosition = *position;

This line is wrong | stm->lastCompensativePosition = *position;|,
it should be |stm->lastCompensativePosition = (msec - mixer_latency + compensation_msec)|.

But even I fix this line, we may still have the a/v sync problem in bug 1165963.
Now the problem is that if we over compensate the audio_clock by system_clock (see example in comment 18),
the code flow I wrote will keeping over compensate the audio_clock until reach |maximum_position|. Cause the A/V sync issue.
You need to log in before you can comment on or make changes to this bug.