Closed Bug 612479 Opened 9 years ago Closed 9 years ago

Sydney audio Android Issues

Categories

(Core :: Audio/Video, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #490781 - Flags: review?(kinetik)
tracking-fennec: --- → ?
if this looks look, i can also update the patch file.
tracking-fennec: ? → 2.0b3+
Attached patch better drain impl. (obsolete) — Splinter Review
"better" is probably the wrong word, but at least this will conform most of the time.
Attachment #490781 - Attachment is obsolete: true
Attachment #490786 - Flags: review?(kinetik)
Attachment #490781 - Flags: review?(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).
Duplicate of this bug: 592030
Attached patch now with estimation (obsolete) — Splinter Review
Attachment #490786 - Attachment is obsolete: true
Attachment #491002 - Flags: review?(kinetik)
Attachment #490786 - Flags: review?(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.
Attached patch patch v.4 (obsolete) — Splinter Review
Attachment #491002 - Attachment is obsolete: true
Attachment #491027 - Flags: review?(kinetik)
Attachment #491002 - Flags: review?(kinetik)
Attached patch patch v.4Splinter Review
Attachment #491028 - Flags: review?(kinetik)
Attachment #491027 - Attachment is obsolete: true
Attachment #491027 - Flags: review?(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.
Attachment #491028 - Flags: review?(kinetik) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.