693131 - Problems playing 11kHz WAV audio and short files on Android

Status: RESOLVED FIXED

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 565777 [details]
test

STR
 (1) Open the attached testcase
 (2) Press the "Beep" button

You should hear the DTMF tone for "0", and do, on my Ubuntu desktop.  In firefox-android, I don't hear anything.

In a bit more detail: I tried playing this audio in both a b2g build (which is still very close to firefox-android under the covers) on an unbranded Galaxy S II, and nothing happened.  This is equivalent to trying to play audio from the chrome process.  I got this error in logcat

W/System.err( 2826): java.lang.IllegalArgumentException: Invalid audio buffer size.
W/System.err( 2826): 	at android.media.AudioTrack.audioBuffSizeCheck(AudioTrack.java:436)
W/System.err( 2826): 	at android.media.AudioTrack.<init>(AudioTrack.java:314)
W/System.err( 2826): 	at android.media.AudioTrack.<init>(AudioTrack.java:265)
W/System.err( 2826): 	at dalvik.system.NativeStart.run(Native Method)
W/System.err( 2826): 	at dalvik.system.NativeStart.run(Native Method)

Not very helpful.  I tried the same test page in a vanilla firefox-android nightly on another phone of my, Galaxy S II Epic 4G, and got the same result: nothing played.  In this setup, the audio would have been played from a content process.  There were no errors in the logcat this time.

In even more detail: the audio format is 16-bit single-channel 11KHz PCM (WAV), ~240ms.  It's embedded in the testcase as a base64-encoded data URI.  I'll attach the raw .wav file it was encoded from in a second.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 565778 [details]
raw audio file used in test above

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Another datapoint: on my Galaxy S II Epic 4G, I tried to play the raw audio file in firefox-android, but it downloaded and played the file in my music player instead.  The file played just fine.  So android and my HW are able to play the file.

I tried modifying the testcase above to load the raw .wav file (<audio src="0.wav">) instead of using a base64 URI, but I got the same result: no audio played.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In my b2g build, the check that's failing is

    private void audioBuffSizeCheck(int audioBufferSize) {
        // NB: this section is only valid with PCM data.
        //     To update when supporting compressed formats
        int frameSizeInBytes = mChannelCount
                * (mAudioFormat == AudioFormat.ENCODING_PCM_8BIT ? 1 : 2);
        if ((audioBufferSize % frameSizeInBytes != 0) || (audioBufferSize < 1)) {
            throw (new IllegalArgumentException("Invalid audio buffer size."));
        }

Comment 4

[Matthew Gregan [:kinetik]]

AudioTrack's buffer size parameters is supposed to be in bytes, but the code in sydney_audio_android.c#sa_stream_create_pcm is calculating the buffer size in samples.  Changing

  s->bufferSize = rate * channels;

to

  s->bufferSize = rate * channels * sizeof(short);

should fix this.

Comment 5

[Matthew Gregan [:kinetik]]

sa_stream_get_write_size is also mixing units:

  *size = s->bufferSize - ((s->timePlaying * s->channels * s->rate /
                            MILLISECONDS_PER_SECOND) - s->amountWritten);

should be 

  *size = s->bufferSize - ((s->timePlaying * s->channels * s->rate * sizeof(short) /
                            MILLISECONDS_PER_SECOND) - s->amountWritten);

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 566096 [details] [diff] [review]
WIP

This makes the error go away, but I still don't get any audio :/.

Comment 7

[Matthew Gregan [:kinetik]]

The logic in sa_stream_write to start the stream playing after the first write won't run if the first write is small enough to complete in a single loop iteration.  With that fixed the audio still isn't audible.  This can be worked around by writing additional data in sa_stream_drain (e.g. s->bufferSize) before draining, suggesting a minimum write size before data becomes audible--but I don't see such a requirement in the AudioTrack documentation.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Matthew/Mike, is that a reasonable workaround?  Matthew, do you have a patch I can try?

Comment 9

[Matthew Gregan [:kinetik]]

I'll post a patch soon, I'm trying to work out exactly what the minimum-write requirements might be.

Comment 10

[Matthew Gregan [:kinetik]]

Created attachment 566958 [details] [diff] [review]
patch v0

This is the best I can come up with for now.  The original testcase works on every button press for me on a Nexus S.  Includes the fixes described in this bug so far, plus a workaround in sa_stream_drain that fills the audio buffers with silence to force a flush of any already buffered data.  To avoid drain being delayed by the additional silence, compute the drain wait based on the amount of data written before the silence.  Also includes a fix to sa_stream_destroy where the previous code was leaving the stream active, resulting in obtainBuffer timeouts appearing in logcat.

Comment 12

[Matthew Gregan [:kinetik]]

Created attachment 567973 [details] [diff] [review]
patch v1

Minor changes:
- use sizeof(int16_t) rather sizeof(short) to match existing code
- use SetByteArrayRegion rather than GetByteArrayElements/ReleaseByteArrayElements (reported to be fasted--I couldn't measure a difference, but the code is simpler)

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 567973 [details] [diff] [review]
patch v1

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

R+ if you address the leak (either fix it or comment here), and add a comment to the extra sa_stream_write()

::: media/libsydneyaudio/src/sydney_audio_android.c
@@ +311,2 @@
>  
>    jbyteArray bytearray = (*jenv)->NewByteArray(jenv, nbytes);

why don't we free this any longer?

@@ +463,5 @@
>    size_t available;
>    sa_stream_get_write_size(s, &available);
>  
> +  void *p = calloc(1, available);
> +  sa_stream_write(s, p, available);

i don't understand this bit completely.  maybe a comment would help.

Comment 14

[Matthew Gregan [:kinetik]]

> why don't we free this any longer?

The object allocated by NewByteArray is (and was) freed by the GC.  The removed Release was paired with the removed Get.

> i don't understand this bit completely.  maybe a comment would help.

Good point, I'll add this comment when I check in:

/* This is somewhat of a hack (see bug 693131).  The AudioTrack documentation
   doesn't make it clear how much data must be written before a chunk of data is
   played, and experimentation with short streams required filling the entire
   allocated buffer.  To guarantee that short streams (and the end of longer
   streams) are audible, fill the remaining space in the AudioTrack with silence
   before sleeping.  Note that the sleep duration is calculated from the
   duration of audio written before filling the buffer with silence. */

Comment 15

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/92900f4e19f1

Comment 16

[Marco Bonardo [::mak]]

https://hg.mozilla.org/mozilla-central/rev/92900f4e19f1