612479 - Sydney audio Android Issues

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

The current implementation has some buffering problems.

1) Sydney assumes that Available() returns the totally available to write without blocking

2) If you call write(), the expectation is that it blocks until everything is written.

3) We do not start playing automagically after we get enough data written.  We must force a resume as soon as we have written (if we haven't been explictly paused).

There are some Android SDK deficiencies:

1) There is no way to know how much you can write without blocking

2) You have to set default buffer sizes at init time, and can not change them until you tear down the audio track.

Comment 1

[Doug Turner (:dougt)]

if this looks look, i can also update the patch file.

Comment 2

[Doug Turner (:dougt)]

Attachment: better drain impl.

"better" is probably the wrong word, but at least this will conform most of the time.

Comment 3

[Matthew Gregan [:kinetik]]

+    if (wroteSoFar != retval) {

Just checking, but the intention of this test is to only execute the following
code on the second and subsequent writes in the loop?

+
+      if (!s->isPaused)
+        (*jenv)->CallVoidMethod(jenv, s->output_unit, at.play);

I'm confused, how can isPaused be false if we're not playing?  If we call play,
don't we need to update isPaused?

+      struct timespec ts = {1, 0}; /* 1s */
+      nanosleep(&ts, NULL);
+    }

The audio buffer is only 1s long, so this seems like a guaranteed underrun.  What's this needed for?

Also, the entire loop can end up busy-waiting if write is returning 0.  Do we
need to sleep for a short period before trying to write more data if it
returned 0?

-  // XXX AudioTrack doesn't block on writes but it might not write everything.
-  //     Needs investigation.
-  *size = (s->rate * s->channels) / 2;
+  size = 4096;

It might be better to estimate avail based on the buffer size we configure the AudioTrack with?

+     position = frames * (PCM_16_BIT == 4 bytes) * channels

2 bytes

+  /* There is no way with the Android SDK to determine exactly how
+     long to playback.  We know that we only can buffer a max of 1s of
+     data, so lets sleep for the long.
+  */
+
+  struct timespec ts = {1, 0}; /* 1s */
+  nanosleep(&ts, NULL);

This could be based on the same logic as avail (max - avail = time to sleep).

Comment 5

[Doug Turner (:dougt)]

Attachment: now with estimation

Comment 6

[Matthew Gregan [:kinetik]]

+  size = s->bufferSize - (s->timePlaying * s->channels * s->rate * 1024) - s->amountWritten;

Shouldn't this be:

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

+  size_t size;
+  sa_stream_get_write_size(s, &size);
+
+  float x = (s->bufferSize - size) * 1000 / s->channels / s->rate / sizeof(int16_t) * NANOSECONDS_IN_MILLISECOND;
+  ALOG("%x - Drain - sleep for %f ns", s, x);
+
+  struct timespec ts = {1, x};

Rename size to available.  x should be a long (or whatever timespec.tv_nsec is).  ts should be initialized with {0, x}.  Move the define to the top of the file.

I'll still confused about the write loop, I'll try to catch up with you on IRC about that.

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 491028 [details] [diff] [review]
patch v.4

+#define NANOSECONDS_IN_MILLISECOND 1000000.

Remove the trailing '.'

+  do {
+
+    retval = (*jenv)->CallIntMethod(jenv,

Remove empty line.

+      if (!s->isPaused)
+	sa_stream_resume(s);

Add a comment about this, since it was non-obvious to me until I talked to you on IRC.

+  } while ( wroteSoFar < nbytes );

Remove extra whitespace.

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

Sorry, I know I just told you to use this, but now that I look at it I'm worried about underflow, and the order of operations is incorrect in what I said to use:

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

This is slightly better, but it might be even better to break it down and add underflow checks.  What do you think?

Everything else looks good, so r+ with the bits above.  I haven't tested this as I don't have an Android device yet.

Comment 10

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/6dd7e2f97639