695986 - Android video playback suffers from bug 669556

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

I claimed that the testcase in bug 693905 worked once the  patches I mentioned had been applied, but that was incorrect.  There's an additional fix required that I had experimented with while debugging and was unknowingly still testing against due to doing partial source rebuilds.

The necessary fix is to force an initial position update via AudioParent::Notify when the first write occurs, otherwise the position estimator doesn't start returning valid times until the timer fires, which happens one second after initializing the AudioParent.

Comment 1

[Matthew Gregan [:kinetik]]

Hrm, there's another problem where it'll stutter when it begins playing, or within the first few seconds, and then never recover.  If you seek to a new position, it plays fine.  That sound suspiciously like bug 669556, which will still exist here due to reintroducing the Wait().  Maybe I'll need to fix bug 695612 before this is going to work properly.

Comment 2

[Matthew Gregan [:kinetik]]

I've verifying via logging that the remaining problem is simply bug 669556.  The AudioParent timer changes merely alter the behaviour (but not in a consistently good way).  It still makes sense to only run the timer when the stream is active, though.  I'll use this bug to track that fix and some other general improvements.

Fixing this correctly involves fixing bug 695612.

Comment 3

[Matthew Gregan [:kinetik]]

Changing summary.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: content/media patch v0

Fix bug 669556 on Android by removing the broken code.  We can do this now that Fennec is single process in the NativeUI world order, since the bug affecting the remoted audio code (bug 695612) no longer matters.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: sydneyaudio patch v0

Only start the AudioTrack playing once it signals its buffers are full.  Remove the unnecessary fake-blocking-write sleep in sa_stream_write.  Allocate a small Java byte array during init to be used for write calls--this avoids churning the GC by allocating on every write.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 576023 [details] [diff] [review]
sydneyaudio patch v0

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

::: media/libsydneyaudio/src/sydney_audio_android.c
@@ +277,4 @@
>    (*jenv)->PopLocalFrame(jenv, NULL);
>  
> +  ALOG("%p - New stream %u %u bsz=%u min=%u obsz=%u", s,  s->rate, s->channels,
> +       s->bufferSize, minsz, s->output_buf_size);

nit: remove extra space after the 's,'.  Also, the second line should be aligned under the %

Comment 7

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/22f2cc4d1a04
http://hg.mozilla.org/integration/mozilla-inbound/rev/f4381030a5ab

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/22f2cc4d1a04
https://hg.mozilla.org/mozilla-central/rev/f4381030a5ab