Closed Bug 940707 Opened 6 years ago Closed 6 years ago

opensl_get_min_latency() does not care about latency after mixing in AudioFlinger

Categories

(Core :: Audio/Video, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g -

People

(Reporter: sotaro, Assigned: padenot)

References

Details

Attachments

(2 files, 2 obsolete files)

cubeb_opensl.c is used on b2g. When I read opensl_get_min_latency(), it seems to care about latency until audio track mixed by AudioFlinger, but not care about the latency after mixed until output to audio hw.

http://mxr.mozilla.org/mozilla-central/source/media/libcubeb/src/cubeb_opensl.c#227


AudioSystem::getOutputLatency() seems to provide this info.
http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libmedia/AudioSystem.cpp#290
blocking-b2g: --- → 1.3?
Makes sense, I'll write a patch for that.
Assignee: nobody → paul
Summary: opensl_get_min_latency() does not care about latency after mixing at SurfaceFlinger on gonk → opensl_get_min_latency() does not care about latency after mixing in AudioFlinger
This should be better.
Attachment #8335738 - Flags: review?(sotaro.ikeda.g)
Attachment #8335738 - Flags: review?(kinetik)
Comment on attachment 8335738 [details] [diff] [review]
Get more accurate latency numbers when using OpenSL. r=

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

::: media/libcubeb/src/cubeb_opensl.c
@@ +534,5 @@
> +    return CUBEB_ERROR;
> +  }
> +
> +  rv = get_output_latency(&mixer_latency, stm->stream_type);
> +

No newline.
Attachment #8335738 - Flags: review?(kinetik) → review+
Comment on attachment 8335738 [details] [diff] [review]
Get more accurate latency numbers when using OpenSL. r=

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

I tested the patch on hamachi(ics gonk) and nexus7(flo, JB 4.3 gonk) and found some problems.

::: media/libcubeb/src/cubeb_opensl.c
@@ +513,5 @@
> +  uint32_t samplerate;
> +
> +  /* The latency returned by AudioFlinger is in ms, so we have to get
> +   * AudioFlinger's samplerate to convert it to frames. */
> +  rv = opensl_get_preferred_sample_rate(stm->context, &samplerate);

This function failed on gonk ICS b2g.(hamachi device)
In the function, AudioSystem::getOutputLatency() is called. It is not defined on android ICS.

@@ +517,5 @@
> +  rv = opensl_get_preferred_sample_rate(stm->context, &samplerate);
> +  if (!rv) {
> +    return CUBEB_ERROR;
> +  }
> +

The function returns CUBEB_OK(=0) when success.

@@ +527,5 @@
> +  /* Get the latency, in ms, from AudioFlinger */
> +  /* status_t AudioSystem::getOutputLatency(uint32_t* latency,
> +   *                                        audio_stream_type_t streamType) */
> +  get_output_latency =
> +    dlsym(libmedia, "_ZN7android11AudioSystem16getOutputLatencyEPji");

In android JB4.3, the function is changed to '_ZN7android11AudioSystem16getOutputLatencyEPj19audio_stream_type_t'.

@@ +535,5 @@
> +  }
> +
> +  rv = get_output_latency(&mixer_latency, stm->stream_type);
> +
> +  if (!rv) {

success NO_ERROR(= 0). In success case, fall to error path.
I also notice that current video playback does not use stream_get_latency() for a/v sync.
cubeb_stream_get_position() is used to get current position from libcubeb. Do we need to subtract some latency value in order to do a/v sync? or this position value is already good for a/v sync?
It should already be factored into the result of cubeb_stream_get_position, if that's not true for a given backend then that's a bug that should be fixed in libcubeb.
Currently, this api is only being used for latency logging.  So this should *not* be a v1.3 blocker.  (If we decide to use this api for something more, then we would reconsider its blocker status.)
cubeb_stream_get_position() also does not care about the latency after mixed until output to audio hw. cubeb_stream_get_position() end up to AudioTrack::getPosition(). The getPosition() returns the total number of frames played since playback start. It care about the number of frame consumed at the Mixer. It does not care about the time from Mixer to audio out.

The following is the call stack of cubeb_stream_get_position().
(gdb) info stack
#0  android::AudioTrack::getPosition (this=0x43fd9580, position=0x471a5cfc) at frameworks/base/media/libmedia/AudioTrack.cpp:818
#1  0x462c1f32 in android::AudioTrackProxy::getPosition (pPlayItf=<value optimized out>, pPosMsec=0x471a5d14)
    at system/media/wilhelm/src/android/AudioTrackProxy.h:44
#2  android_audioPlayer_getPosition (pPlayItf=<value optimized out>, pPosMsec=0x471a5d14)
    at system/media/wilhelm/src/android/AudioPlayer_to_android.cpp:2006
#3  0x462d37d0 in IPlay_GetPosition (self=0x43cf5870, pMsec=0x471a5d2c) at system/media/wilhelm/src/itf/IPlay.c:210
#4  0x412c2b60 in opensl_stream_get_position (stm=0x442c18c0, position=0x471a5d48)
    at /home/sotaro/b2g_master_hamachi/B2G/gecko/media/libcubeb/src/cubeb_opensl.c:497
#5  0x412c2a4a in cubeb_stream_get_position (stream=0x442c18c0, position=0x471a5d48)
    at /home/sotaro/b2g_master_hamachi/B2G/gecko/media/libcubeb/src/cubeb.c:249
#6  0x40e6c926 in mozilla::BufferedAudioStream::GetPositionInFramesUnlocked (this=0x44d8b300)
    at /home/sotaro/b2g_master_hamachi/B2G/gecko/content/media/AudioStream.cpp:847
#7  0x40e6c93e in mozilla::BufferedAudioStream::GetPositionInFramesInternal (this=0x44d8b300)
    at /home/sotaro/b2g_master_hamachi/B2G/gecko/content/media/AudioStream.cpp:832
#8  0x40e64f12 in mozilla::AudioClock::GetPosition (this=0x44d8b318) at 

/home/sotaro/b2g_master_hamachi/B2G/gecko/content/media/AudioStream.cpp:1106
I am not sure if current BufferedAudioStream and MediaDecoderStateMachine can handle correctly the following situations.
- audio output via bluetooth
     In this situation, latency from Mixer to audio out becomes large.
- video is paused in the middle of the duration. And resume playback.

Event when AudioTrack::getPosition() start to increase, audio does not output until the duration of "from Mixer to audio out" go by. Current audio architecture seems not handle this correctly.
From Commet 9, current cubeb_stream_get_position() implementation seems correct. The problem in Comment 10 need to be handled in AudioClock and MediaDecoderStateMachine level.
(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> From Commet 9, current cubeb_stream_get_position() implementation seems
> correct. The problem in Comment 10 need to be handled in AudioClock and
> MediaDecoderStateMachine level.

Android stagefright handles it in AudioPlayer::getRealTimeUsLocked().
http://androidxref.com/4.4_r1/xref/frameworks/av/media/libstagefright/AudioPlayer.cpp#704
I could only test this on 4.3, and it works there. I think I have the right
symbols for earlier android versions, but I don't have the right libmedia.so to
check.
Attachment #8336533 - Flags: review?(sotaro.ikeda.g)
Attachment #8335738 - Attachment is obsolete: true
Attachment #8335738 - Flags: review?(sotaro.ikeda.g)
(In reply to Maire Reavy [:mreavy] from comment #8)
> Currently, this api is only being used for latency logging.  So this should
> *not* be a v1.3 blocker.  (If we decide to use this api for something more,
> then we would reconsider its blocker status.)

blocking- per this comment
blocking-b2g: 1.3? → -
Comment on attachment 8336533 [details] [diff] [review]
Get more accurate latency numbers when using OpenSL. r=

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

The patch worked on nexus7, but crashed on hamachi. at get_output_samplingrate((int *) rate, AUDIO_STREAM_TYPE_MUSIC);
The symbol of AudioSystem::getOutputSamplingRate() in android ICS hamachi is different. And "if (get_output_samplingrate) {" need to be "if (!get_output_samplingrate) {".

::: media/libcubeb/src/cubeb_opensl.c
@@ +303,5 @@
> +     * status_t AudioSystem::getOutputSamplingRate(int* samplingRate, int streamType)
> +     * if we cannot find getPrimaryOutputSamplingRate. */
> +    get_output_samplingrate =
> +      dlsym(libmedia, "_ZN7android11AudioSystem21getOutputSamplingRateEPj19audio_stream_type_t");
> +    if (get_output_samplingrate) {

This check seems opposite. shouldn't it like the following?
> if (!get_output_samplingrate) {
symbol got from libmedia.so on hamachi.
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> I am not sure if current BufferedAudioStream and MediaDecoderStateMachine
> can handle correctly the following situations.
> - audio output via bluetooth
>      In this situation, latency from Mixer to audio out becomes large.
> - video is paused in the middle of the duration. And resume playback.
> 
> Event when AudioTrack::getPosition() start to increase, audio does not
> output until the duration of "from Mixer to audio out" go by. Current audio
> architecture seems not handle this correctly.

I am going to create a bug for it.
Blocks: 942988
No longer blocks: 942988
> I am going to create a bug for it.

Created Bug 942988.
(In reply to Sotaro Ikeda [:sotaro] from comment #15)
> Comment on attachment 8336533 [details] [diff] [review]
> Get more accurate latency numbers when using OpenSL. r=
> 
> Review of attachment 8336533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch worked on nexus7, but crashed on hamachi. at
> get_output_samplingrate((int *) rate, AUDIO_STREAM_TYPE_MUSIC);
> The symbol of AudioSystem::getOutputSamplingRate() in android ICS hamachi is
> different. And "if (get_output_samplingrate) {" need to be "if
> (!get_output_samplingrate) {".
> 
> ::: media/libcubeb/src/cubeb_opensl.c
> @@ +303,5 @@
> > +     * status_t AudioSystem::getOutputSamplingRate(int* samplingRate, int streamType)
> > +     * if we cannot find getPrimaryOutputSamplingRate. */
> > +    get_output_samplingrate =
> > +      dlsym(libmedia, "_ZN7android11AudioSystem21getOutputSamplingRateEPj19audio_stream_type_t");
> > +    if (get_output_samplingrate) {
> 
> This check seems opposite. shouldn't it like the following?
> > if (!get_output_samplingrate) {

Sorry, I haven't uploaded the right version. I have an updated patch that works better, taking into account all the different versions I could find.
So, I grabbed an hamachi, and tested this patch, and it works.

I've also fixed symbols for the get_min_latency function.
Attachment #8340006 - Flags: review?(sotaro.ikeda.g)
Attachment #8336533 - Attachment is obsolete: true
Attachment #8336533 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8340006 [details] [diff] [review]
Get more accurate latency numbers when using OpenSL. r=

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

The patch works on hamachi and nexus-7(flo).
Attachment #8340006 - Flags: review?(sotaro.ikeda.g) → review+
https://hg.mozilla.org/mozilla-central/rev/7e7ba237d059
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment on attachment 8340006 [details] [diff] [review]
Get more accurate latency numbers when using OpenSL. r=

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

::: media/libcubeb/src/cubeb_opensl.c
@@ +577,5 @@
> +    return CUBEB_ERROR;
> +  }
> +
> +  *latency = NBUFS * stm->queuebuf_len / stm->framesize + // OpenSL latency
> +             mixer_latency * samplerate / 1000; // AudioFlinger latency

May I ask a question here? Does OpenSL Latency really need to be considered as a latency?

For mixer_latency, it is caused by Audio Flinger will push audio frame into HW one by one and align the sample rate so it need a short time to fulfill the HW buffer and latency from components.

For openSL_latency, we didn't fill it's buffer one by one and align the sample rate, but push frames as soon as possible until it is full.
Flags: needinfo?(paul)
Blocks: 942741
(In reply to Marco Chen [:mchen] [PTO from 2013/12/24 ~ 2014/01/02] from comment #24)
> Comment on attachment 8340006 [details] [diff] [review]
> Get more accurate latency numbers when using OpenSL. r=
> 
> Review of attachment 8340006 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libcubeb/src/cubeb_opensl.c
> @@ +577,5 @@
> > +    return CUBEB_ERROR;
> > +  }
> > +
> > +  *latency = NBUFS * stm->queuebuf_len / stm->framesize + // OpenSL latency
> > +             mixer_latency * samplerate / 1000; // AudioFlinger latency
> 
> May I ask a question here? Does OpenSL Latency really need to be considered
> as a latency?
> 
> For mixer_latency, it is caused by Audio Flinger will push audio frame into
> HW one by one and align the sample rate so it need a short time to fulfill
> the HW buffer and latency from components.
> 
> For openSL_latency, we didn't fill it's buffer one by one and align the
> sample rate, but push frames as soon as possible until it is full.

I'm not sure if we are using the words in the same way, so I may misunderstand what you mean. In Gecko (and a bunch of other software), a frame is the set of the sample from all the channels that are played at the same time. An interleaved audio buffer is a succession of frames.

By "align the samplerate", do you mean resampling from the sample rate of the audio buffer passed to OpenSL to the sample rate of the mixer?

OpenSL pass buffers to Audio Flinger one by one, and I agree that in theory, we should be able to ping pong two buffers between OpenSL and Audio Flinger, but from my testing, it glitches (maybe because of scheduling issues, maybe something else).

Anyways, I was plan to dig into this, but I lacked time before the break. If needed, maybe I can start again.
Flags: needinfo?(paul)
Hi,

I should re-phrase my description.
Now talk the latency combination one by one.

  1. mixer_latency: The latency here contained the HW buffering and components propagation (ex: D/A A/D converter).
    Image that when audio flinger push first frame into Audio driver at N ms then we expect to listen it at (N + mixer_lantency) ms. (I have no problem on this)

  2. openSL_latency: This latency will indicate that when AudioStream push first frame into Cubeb, how many time OpenSL ES can pass the first frame into Audio Flinger.

I would said that there is no delay on "2. openSL_Lantency", because there is no considerable delay in the following call flow.

  AudioStream::Start -> opensl_stream_start -> bufferqueue_callback -> SetPlayState

So I think we don't need to count in "OpenSL latency".
You need to log in before you can comment on or make changes to this bug.