Closed Bug 944132 Opened 7 years ago Closed 7 years ago

AudioClock::GetPosition() is not precise in gonk

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: sotaro, Assigned: padenot)

References

Details

(Keywords: regression, Whiteboard: burirun3 )

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #933376 +++

During playback of the following youtube video, audio and video became out of sync. By investigating the problem, it became clear that AudioClock::GetPosition() became out of sync from actual value. It is cause by lose precision by multiple audio time unit transforms.

http://www.youtube.com/watch?v=fOQqDMOLooc
No longer depends on: 934106, 935118
Set to koi?, because it blocks Bug #933376.
blocking-b2g: --- → koi?
Blocks: 933376
No longer depends on: 933376
From AudioClock::GetPosition() to AudioTrack::getPosition(), functions are called like following.

AudioClock::GetPosition()
->BufferedAudioStream::GetPositionInFramesInternal()
->BufferedAudioStream::GetPositionInFramesUnlocked()
->opensl_stream_get_position()
->android_audioPlayer_getPosition()
->AudioTrack::getPosition()
Assignee: nobody → sotaro.ikeda.g
Unit transforms are done 3 time until AudioClock::GetPosition().

AudioClock::GetPosition() // transform unit from number of frames to ms 
->BufferedAudioStream::GetPositionInFramesInternal()
->BufferedAudioStream::GetPositionInFramesUnlocked()
->opensl_stream_get_position() // transform unit from ms to number of frames
->android_audioPlayer_getPosition() // transform unit from number of frames to ms
->AudioTrack::getPosition()  // get total number of frames played since playback start.
playback time is changed like the following. They are actual value got from the logcat.

AudioClock::GetPosition()            // 1276119808 us(1276120 ms)
->BufferedAudioStream::GetPositionInFramesInternal()
->BufferedAudioStream::GetPositionInFramesUnlocked()
->opensl_stream_get_position()      // 56276880 frames
->android_audioPlayer_getPosition() // 1279020 ms
->AudioTrack::getPosition()         // 56404800 frames
Assignee: sotaro.ikeda.g → nobody
Blocks: 944167
No longer blocks: 944167
Is this really a regression, or did that flag come in from cloning?
I forgot to remove 'regression' from cloning. But it is actually a regression from b2gv1.1 to v1.2. It changed the gonk api from AudioTrack to OpenSLES. By the change, audio time resolution was regressed.
Do we have a chance to go back to the old API?  Is that the solution?
In b2g v1.1, audio out put is controlled by "libsydneyaudio + AudioTrack". In b2g v1.2, audio out put is controlled by "libcubeb + OpenSLES". libsydneyaudio was already replaced by libcubeb and removed from gecko. We can revert OpenSLES to AudioTrack. But OpenSLES was chosen because of the following reasons.

 - AudioTrack API is an unstable API, some apis are slightly different depends on each android versions.
 - OpenSLES is public api in Android. android's Firefox browser uses it.
   Use Same API to android android's Firefox browser and on other platforms that support OpenSLES.

There is a basic implementation of "libcubeb + AudioTrack". But it seems not tested and might need to fix build failure in some android versions. So, I am not sure changed to "libcubeb + AudioTrack" could be a viable solution now.
Consider the following:

Stream at 44100Hz, stereo, 2 bytes per sample (= 4 byte per frame)
The number of bytes per second is 44100 * 4 = 176400.
Say we are 10ms in in the media.

Old code:
*position = (stm->bytespersec / (1000 * stm->framesize)) * msec
(176400) / (1000 * 4) * 10 = 440 frames

Later, we go and divide that by the samplerate to get ms again:
440 / 44100 * 1000 = 9.977324263038549ms

The right way to do this is:

*position = (stm->bytespersec / stm->framesize) * msec / 1000;
(176400 / 4) * 10 / 1000 = 441 frames
441 / 44100 * 1000 = 10ms
Attachment #8340031 - Flags: review?(sotaro.ikeda.g)
Assignee: nobody → paul
Thanks! I am going to check the patch.
Comment on attachment 8340031 [details] [diff] [review]
Make sure we don't loose precision when computing clock when using opensl.

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

The patch has bit overflow problem. Other parts works good.

::: media/libcubeb/src/cubeb_opensl.c
@@ +531,5 @@
>      return CUBEB_ERROR;
> +
> +  samplerate = stm->bytespersec / stm->framesize;
> +
> +  *position = samplerate * msec / 1000;

This part causes bit overflow. On hamachi, I confirmed that "44100 * 97414 / 1000" becomes 990 and video update was stopped after that. By changing samplerate to 64bit, the problems was fixed.
By locally fixing the overflow problem, I confirmed the video in Comment 0 can be playbacked with a/v sync when the video is stored on sdcard. The following is the audio time info at almost end of the video(around 25 minites). The difference of the frame number was less than 1ms.

AudioClock::GetPosition()            // 1225659008 us(1225659 ms)
->BufferedAudioStream::GetPositionInFramesInternal()
->BufferedAudioStream::GetPositionInFramesUnlocked()
->opensl_stream_get_position()      // 54051561 frames
->android_audioPlayer_getPosition() // 1225659 ms
->AudioTrack::getPosition()         // 54051600 frames
Comment on attachment 8340031 [details] [diff] [review]
Make sure we don't loose precision when computing clock when using opensl.

Overflow problem is just a nit to fix. Set review+.
Attachment #8340031 - Flags: review?(sotaro.ikeda.g) → review+
milan, can you set this bug to koi+. This fix is necessary to fix Bug 933376(koi+ bug).
Flags: needinfo?(milan)
blocking-b2g: koi? → koi+
Flags: needinfo?(milan)
Pushed to mozilla-inbound, with comment (uint32_t -> uint64_t) addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/81c66b34eb05
https://hg.mozilla.org/mozilla-central/rev/81c66b34eb05
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Ryan, I'm taking care of this, this library (libcubeb) is written by Mozilla, but upstream is https://github.com/kinetiknz/cubeb. I was just waiting for other patches to get merged so I can upstream everything at once.
You need to log in before you can comment on or make changes to this bug.