+++ 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

Set to koi?, because it blocks Bug #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()

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

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

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+.

milan, can you set this bug to koi+. This fix is necessary to fix Bug 933376(koi+ bug).

Pushed to mozilla-inbound, with comment (uint32_t -> uint64_t) addressed. https://hg.mozilla.org/integration/mozilla-inbound/rev/81c66b34eb05

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d9f7d349d9ce Does this need to be upstreamed somewhere?

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.