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

RESOLVED FIXED in mozilla28

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sotaro, Assigned: padenot)

Tracking

26 Branch
mozilla28
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
blocking-b2g: --- → 1.3?
(Assignee)

Comment 1

5 years ago
Makes sense, I'll write a patch for that.
Assignee: nobody → paul
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 2

5 years ago
Created attachment 8335738 [details] [diff] [review]
Get more accurate latency numbers when using OpenSL. r=

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+
(Reporter)

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
I also notice that current video playback does not use stream_get_latency() for a/v sync.

Comment 6

5 years ago
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.)
(Reporter)

Comment 9

5 years ago
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
(Reporter)

Comment 10

5 years ago
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.
(Reporter)

Comment 11

5 years ago
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.
(Reporter)

Comment 12

5 years ago
(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
(Assignee)

Comment 13

5 years ago
Created attachment 8336533 [details] [diff] [review]
Get more accurate latency numbers when using OpenSL. r=

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)
(Assignee)

Updated

5 years ago
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? → -
(Reporter)

Comment 15

5 years ago
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) {
(Reporter)

Comment 16

5 years ago
Created attachment 8337924 [details]
hamachi libmedia.so's symbol

symbol got from libmedia.so on hamachi.
(Reporter)

Comment 17

5 years ago
(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.
(Reporter)

Updated

5 years ago
Blocks: 942988
(Reporter)

Updated

5 years ago
No longer blocks: 942988
(Reporter)

Comment 18

5 years ago
> I am going to create a bug for it.

Created Bug 942988.
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Comment 20

5 years ago
Created attachment 8340006 [details] [diff] [review]
Get more accurate latency numbers when using OpenSL. r=

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)
(Assignee)

Updated

5 years ago
Attachment #8336533 - Attachment is obsolete: true
Attachment #8336533 - Flags: review?(sotaro.ikeda.g)
(Reporter)

Comment 21

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Comment 24

5 years ago
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.

Updated

5 years ago
Flags: needinfo?(paul)

Updated

5 years ago
Blocks: 942741
(Assignee)

Comment 25

5 years ago
(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)

Comment 26

5 years ago
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.