Closed Bug 693131 Opened 10 years ago Closed 10 years ago

Problems playing 11kHz WAV audio and short files on Android


(Core :: Audio/Video, defect)

Not set





(Reporter: cjones, Assigned: kinetik)




(3 files, 2 obsolete files)

Attached file test
 (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
W/System.err( 2826): 	at<init>(
W/System.err( 2826): 	at<init>(
W/System.err( 2826): 	at Method)
W/System.err( 2826): 	at 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.
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.
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."));
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;


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

should fix this.
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);
Attached patch WIP (obsolete) — Splinter Review
This makes the error go away, but I still don't get any audio :/.
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.
Matthew/Mike, is that a reasonable workaround?  Matthew, do you have a patch I can try?
I'll post a patch soon, I'm trying to work out exactly what the minimum-write requirements might be.
Assignee: nobody → kinetik
Attached patch patch v0 (obsolete) — Splinter Review
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.
Attachment #566096 - Attachment is obsolete: true
Attachment #566958 - Flags: review?(doug.turner)
Duplicate of this bug: 694770
Attached patch patch v1Splinter Review
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)
Attachment #566958 - Attachment is obsolete: true
Attachment #566958 - Flags: review?(doug.turner)
Attachment #567973 - Flags: review?(doug.turner)
Summary: Problems playing WAV audio → Problems playing 11kHz WAV audio and short files on Android
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.
Attachment #567973 - Flags: review?(doug.turner) → review+
> 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. */
Blocks: 643454
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.