948269 - Move MediaDecoderStateMachine::AudioLoop and related code to its own file

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

I've got a set of patches that move AudioLoop to a new class (AudioSink) in a new file, and split it up to be easier to read.  This should make additional refactoring work (such as bug 750596) easier to complete as the dependencies between MediaDecoderStateMachine and AudioLoop will be much clearer.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: Remove more coupling between AudioSink and StateMachine.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: p1: Move MediaDecoderStateMachine::AudioLoop and related code to AudioSink.cpp.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: p2: Add setter for mPlayStartTime.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: p3: Remove equivalent logic checks and other minor cleanups.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: p4: Migrate AudioLoop and related code into AudioSink class.

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: p5: AudioSink code cleanup.

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: p6: Split AudioLoop up into logical components.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: p7: Add OnAudioEndTimeUpdate and OnAudioSinkComplete to MediaDecoderStateMachine and call them from AudioSink. Split sleep out of Drain() into standalone SleepUntil().

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: p8: Introduce GetEndTime to AudioSink rather than fetching mAudioEndTime from StateMachine. Misc. code cleanups.

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: p9: Remove more coupling between AudioSink and StateMachine.

Comment 11

[Matthew Gregan [:kinetik]]

The rolled up patch is smaller, but this should be easier to review, because I've split the patches up into pure code moves with absolutely minimal changes, and logic changes.  

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5dd61bd442fa

Comment 12

[Matthew Gregan [:kinetik]]

Comment on attachment 8346326 [details] [diff] [review]
p1: Move MediaDecoderStateMachine::AudioLoop and related code to AudioSink.cpp.

Simple code move, no logic changes.

Comment 13

[Matthew Gregan [:kinetik]]

Comment on attachment 8346327 [details] [diff] [review]
p2: Add setter for mPlayStartTime.

Add a setter for mPlayStartTime which is hooked in a later patch to start/stop AudioSink playback (replacing calls to IsPlaying()).

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 8346328 [details] [diff] [review]
p3: Remove equivalent logic checks and other minor cleanups.

Simplify some tests:
- SEEKING and SHUTDOWN are equivalent to mStopThread.
- BUFFERING is equivalent to IsPlaying.

Deindents wait loop and deletes redundant seeking variable (apologies for mixing code/whitespace change here).

Change call to AtEndOfStream to IsFinished to make the test slightly clearer.

Remove redundant offset/frames variables from PlayFromAudioQueue.

Remove call to GetAudioClock() made after StopAudioThread(), since mAudioThread will be null by then and GetAudioClock() will return a value that's never used.

Comment 15

[Matthew Gregan [:kinetik]]

Comment on attachment 8346329 [details] [diff] [review]
p4: Migrate AudioLoop and related code into AudioSink class.

Move code in AudioSink.cpp into AudioSink class, and add the minimal amount of functionality to the class to build/run.

Eventually we'll want to remove the "friend class AudioSink", but that requires reducing the coupling between classes further.

Comment 16

[Matthew Gregan [:kinetik]]

Comment on attachment 8346330 [details] [diff] [review]
p5: AudioSink code cleanup.

Move StartAudioStreamPlaybackIfNeeded and WriteSilence into AudioSink class so they can use member variables.  Move some constants closer to the single place they're used.  PlaySilence/PlayFromAudioQueue can fetch the channel count from mInfo directly.

Comment 17

[Matthew Gregan [:kinetik]]

Comment on attachment 8346331 [details] [diff] [review]
p6: Split AudioLoop up into logical components.

Split AudioLoop up into more manageable functions.  Code moved without logic changes.

Comment 18

[Matthew Gregan [:kinetik]]

Comment on attachment 8346332 [details] [diff] [review]
p7: Add OnAudioEndTimeUpdate and OnAudioSinkComplete to MediaDecoderStateMachine and call them from AudioSink.  Split sleep out of Drain() into standalone SleepUntil().

Remove some inter-class coupling where AudioSink is reaching in to StateMachine's member variables directly by introducing OnAudioEndTimeUpdate and OnAudioSinkComplete.

Split Drain out so that the sleep loop is in SleepUntil.

Comment 19

[Matthew Gregan [:kinetik]]

Comment on attachment 8346333 [details] [diff] [review]
p8: Introduce GetEndTime to AudioSink rather than fetching mAudioEndTime from StateMachine.  Misc. code cleanups.

Make audioDuration a member variable (mWritten) so it can be used elsewhere.  Add GetEndTime() to calculate audio end time rather than reaching in to StateMachine's copy.  Factor AudioQueue empty-but-expecting test out.  Move logging for silent frames into PlaySilence.  Some misc. code cleanups.

Comment 20

[Matthew Gregan [:kinetik]]

Comment on attachment 8346334 [details] [diff] [review]
p9: Remove more coupling between AudioSink and StateMachine.

Remove the "easy" remaining coupling between AudioSink and StateMachine.  Make SleepUntil a specific function that waits until end-of-audio, and convert it to use the audio clock rather than round tripping through the StateMachine's media clock.

Comment 21

[Matthew Gregan [:kinetik]]

And, of course, it doesn't build with --disable-unified-compilation.

Comment 22

[Matthew Gregan [:kinetik]]

Attachment: p10: Fix build with --disable-unified-compilation.

Comment 23

[Matthew Gregan [:kinetik]]

Comment on attachment 8346355 [details] [diff] [review]
p10: Fix build with --disable-unified-compilation.

Simple fix.

https://tbpl.mozilla.org/?tree=Try&rev=524e4519f576

There's some new orange (test_audio_wakelock.html) in the first push I'll chase up and put up extra patches for.

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8346327 [details] [diff] [review]
p2: Add setter for mPlayStartTime.

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

I'm fine with you addressing review comments in a single patch at the end, it's a pain to rebase changes through a patch queue.

::: content/media/MediaDecoderStateMachine.h
@@ +621,5 @@
>    // or via an event. Access protected by decoder monitor.
>    TimeStamp mTimeout;
>  
> +  // Set the time that playback started from the system clock.
> +  void SetPlayStartTime(const TimeStamp& aTimeStamp) {

Add to comment: // Can only be called on the state machine thread.

And assert we're on the state machine thread.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8346328 [details] [diff] [review]
p3: Remove equivalent logic checks and other minor cleanups.

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

::: content/media/AudioSink.cpp
@@ +119,2 @@
>                (mReader->AudioQueue().GetSize() == 0 &&
> +               !mReader->AudioQueue().IsFinished()))) {

AudioQueue::AtEndOfStream() returns true when GetSize() == 0 && mEndOfStream, which is what you're testing here, so we can just test mReader->AudioQueue().AtEndOfStream() here, right?

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8346329 [details] [diff] [review]
p4: Migrate AudioLoop and related code into AudioSink class.

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

::: content/media/AudioSink.cpp
@@ +376,5 @@
> +    if (setVolume) {
> +      mAudioStream->SetVolume(volume);
> +    }
> +
> +    if (setPlaybackRate && mAudioStream->SetPlaybackRate(playbackRate) != NS_OK) {

I normally use NS_FAILED() instead of !=NS_OK, but up to you.

::: content/media/MediaDecoderStateMachine.cpp
@@ +1114,5 @@
>  void MediaDecoderStateMachine::SetVolume(double volume)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>    mVolume = volume;

Kinda sad that we have volume stored in at least 3 places, but oh well.

::: content/media/MediaDecoderStateMachine.h
@@ +635,5 @@
>  
>    // Media Fragment end time in microseconds. Access controlled by decoder monitor.
>    int64_t mFragmentEndTime;
>  
> +  nsRefPtr<AudioSink> mAudioSink;

I think you need to retain a comment about needing to hold the decoder monitor before calling into this (except when calling Shutdown()), as the locking protocols are very important to the safety of the shutdown process of the state machine.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8346331 [details] [diff] [review]
p6: Split AudioLoop up into logical components.

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

::: content/media/AudioSink.cpp
@@ +131,4 @@
>    }
>  
>    while (1) {
> +    if (!ContinuePlayback()) {

This function does two things; wait until we've got audio to push, and check whether we need to terminate the thread. I think we should call this WaitForAudioToPlay() or WaitUntilPlayNeeded() or somesuch, so that it's clear that this Waits()s rather than continues to playback audio.

::: content/media/AudioSink.h
@@ +48,5 @@
> +  bool InitializeAudioStream();
> +  void Drain();
> +  void Cleanup();
> +
> +  bool ContinuePlayback();

Document return value, and on InitializeAudioStream().

bool is IMO a horrible return value, as unless the function is called IsSomeCondition(), the meaning of the return value isn't clear. I much prefer returning NS_ERROR_FAILURE when we fail to do something due to having been shutdown during our operation.

@@ +50,5 @@
> +  void Cleanup();
> +
> +  bool ContinuePlayback();
> +
> +    // Write aFrames of audio frames of silence to the audio hardware. Returns

Indentation off here.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8346328 [details] [diff] [review]
p3: Remove equivalent logic checks and other minor cleanups.

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

::: content/media/AudioSink.cpp
@@ +119,2 @@
>                (mReader->AudioQueue().GetSize() == 0 &&
> +               !mReader->AudioQueue().IsFinished()))) {

I'm wrong, AtEndOfStream is:

  bool AtEndOfStream() {
    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
    return GetSize() == 0 && mEndOfStream;
  }

So they're not equivalent. Ignore this comment.

Comment 29

[Matthew Gregan [:kinetik]]

Attachment: p11: Address review comments and fix remaining orange.

Comment 30

[Matthew Gregan [:kinetik]]

Comment on attachment 8350347 [details] [diff] [review]
p11: Address review comments and fix remaining orange.

This is pretty green:
https://tbpl.mozilla.org/?tree=Try&rev=85ac7e184d6b

The crashtest orange is due to an assertion of OnDecodeThread || OnStateMachineThread I added to SetPlayStartTime, but I've dropped that in this final patch.  It still asserts that the lock is held, so it should be quite safe.

The other orange was caused by the conversion of Drain/Wait to use the AudioClock.  I realized that the Wait part only exists to make Drain interruptible, so I altered AudioStream to allow Drain to be interrupted directly, then wired that up and ditched all of the sleep-until-drained code.  Net win!

Comment 31

[Matthew Gregan [:kinetik]]

Note that my plan is to fold this into a single patch for landing, I don't think it makes sense to reveal most of this as individual patches in the tree history--it's just split up to make reviewing easier.

Comment 32

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2c930a960c

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/719a42eaed4c for making test_audio_wakelock.html permanently time out on Android 4.0:

https://tbpl.mozilla.org/php/getParsedLog.php?id=32253285&tree=Mozilla-Inbound

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Same on B2G ICS emulator: https://tbpl.mozilla.org/php/getParsedLog.php?id=32256350&tree=Mozilla-Inbound

Comment 35

[Matthew Gregan [:kinetik]]

Attachment: p12: Make AudioStream init non-fatal again.

I've rebased the rest of the patches but won't repost them since they're ready to land.

This patch should fix the failures on B2G and Android.  This patch set made AudioStream::Init fatal (AudioLoop would bail), which was new behaviour and it turns out it breaks a handful of tests, such as test_audio_wakelock, which uses a file that fails to play on some Android configs due to an unsupported sample rate (11027 Hz).  This change restores the existing non-fatal AudioStream::Init behaviour.  Also fixes a compile error caused by -Werror, and removes the frameOffset parameter from PlaySilence/PlayFromAudioQueue since it's no longer needed with the removal of AudioAvailableEventManager (and the rest of the Audio Data API).

It'd be nice to deal with the failing AudioStream::Init in a better way, but that can be done in a new bug when someone has the time for it.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=40b543ac51f6

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8401090 [details] [diff] [review]
p12: Make AudioStream init non-fatal again.

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

Yeah, we should get around to handling AudioStream Init failure sometime...

Comment 37

[Matthew Gregan [:kinetik]]

I think the Init thing is covered by bug 542635, so no new bug filed.

Landed all twelve patches as a rollup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac8fe9a90c5

Comment 38

[Matthew Gregan [:kinetik]]

Attachment: bug948269_followup_v0.patch

This new assert was triggering during test_playback; it can happen if StartAudioStreamIfPlaybackNeeded hasn't triggered before we hit Drain().

Comment 39

[Matthew Gregan [:kinetik]]

Follow up: https://hg.mozilla.org/integration/mozilla-inbound/rev/e00d10064639

Comment 40

[Ed Morley [:emorley]]

Unfortunately this broke B2G's gaia-integration tests:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=gaia-integration&tochange=2ac8fe9a90c5&fromchange=6a9a14232208

eg:
{
19:03:05     INFO -  TEST-START | Contacts > Activities webcontacts/tel activity Error message when no contacts
19:03:17     INFO -  TEST-PASS | Contacts > Activities webcontacts/tel activity Error message when no contacts
19:03:17     INFO -  TEST-END | Contacts > Activities webcontacts/tel activity Error message when no contacts
19:03:17     INFO -  TEST-PENDING | Contacts > Activities webcontacts/tel activity Error message selected contact has no number
19:03:17     INFO -  TEST-END | Contacts > Activities webcontacts/tel activity Error message selected contact has no number
19:03:17     INFO -  TEST-PENDING | Contacts > Search Search Mode Can enter and exit search mode
19:03:17     INFO -  TEST-END | Contacts > Search Search Mode Can enter and exit search mode
19:03:18     INFO -  TEST-START | Contacts > Form Click phone number Add a simple contact
19:03:30     INFO -  TEST-PASS | Contacts > Form Click phone number Add a simple contact
19:03:30     INFO -  TEST-END | Contacts > Form Click phone number Add a simple contact
19:03:31     INFO -  TEST-PENDING | Contacts > Form Click phone number Can create custom label
19:03:31     INFO -  TEST-END | Contacts > Form Click phone number Can create custom label
19:03:31     INFO -  TEST-START | Contacts > Form Facebook contacts Add phone number from Dialer to existing Facebook contact
19:09:01     INFO - Automation Error: mozprocess timed out after 330 seconds running ['make', 'test-integration', 'NPM_REGISTRY=http://npm-mirror.pub.build.mozilla.org', 'REPORTER=mocha-tbpl-reporter', 'TEST_MANIFEST=./shared/test/integration/tbpl-manifest.json']
19:09:01    ERROR - timed out after 330 seconds of no output
19:09:01    ERROR - Return code: -9
19:09:01     INFO - TinderboxPrint: gaia-integration-tests: <em class="testfail">T-FAIL</em>
19:09:01    ERROR - Tests exited with return code -9: harness failures
19:09:01    ERROR - # TBPL FAILURE #
19:09:01     INFO - Running post-action listener: _resource_record_post_action
19:09:01     INFO - Running post-run listener: _resource_record_post_run
19:09:02     INFO - Total resource usage - Wall time: 737s; CPU: 49.0%; Read bytes: 153587712; Write bytes: 1677234176; Read time: 36488; Write time: 8016620
19:09:02     INFO - install - Wall time: 30s; CPU: 100.0%; Read bytes: 0; Write bytes: 93507584; Read time: 0; Write time: 521620
19:09:02     INFO - run-tests - Wall time: 707s; CPU: 47.0%; Read bytes: 152834048; Write bytes: 1567064064; Read time: 36004; Write time: 7398944
19:09:02     INFO - Copying logs to upload dir...
19:09:02     INFO - mkdir: /builds/slave/test/build/upload/logs
program finished with exit code 2
elapsedTime=813.111983
========= Finished '/tools/buildbot/bin/python scripts/scripts/gaia_integration.py ...' failed (results: 2, elapsed: 13 mins, 32 secs) (at 2014-04-03 19:09:02.457781) =========
}

Backout:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b327711444ed
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd79d6f4a7e

Comment 41

[Ed Morley [:emorley]]

This failure was in the try run:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37193751&tree=Try

However since gaia-integration tests don't label tests timeouts properly, it was masked by the already filed intermittent failure bug, bug 953212. Sigh (at the test harness). I've filed bug 992220 to remove this unhelpfulness.

Comment 42

[Phil Ringnalda (:philor)]

Also, asserted fairly often about https://tbpl.mozilla.org/php/getParsedLog.php?id=37283740&tree=Mozilla-Inbound

Comment 43

[Matthew Gregan [:kinetik]]

Rebased and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9aa95a326850

Comment 44

[Matthew Gregan [:kinetik]]

This should fix at least one of the test failures: https://tbpl.mozilla.org/?tree=Try&rev=7ba644967aaa

Comment 45

[Matthew Gregan [:kinetik]]

Attachment: bug948269_rollup_v1.patch

Rolls up the previous patches and rebased to trunk on top of async reader changes plus the patches in bug 1020538.  Media mochitests are now looking better: https://tbpl.mozilla.org/?tree=Try&rev=7a9e163502ad

Comment 46

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b559ec64da05

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b559ec64da05