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

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 14 obsolete attachments)

59.39 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
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.
Attachment #8346324 - Attachment is obsolete: true
Attachment #8346326 - Attachment description: Move MediaDecoderStateMachine::AudioLoop and related code to AudioSink.cpp. → p1: Move MediaDecoderStateMachine::AudioLoop and related code to AudioSink.cpp.
Posted patch p5: AudioSink code cleanup. (obsolete) — Splinter Review
Attachment #8346327 - Attachment description: Add setter for mPlayStartTime. → p2: Add setter for mPlayStartTime.
Attachment #8346328 - Attachment description: Remove equivalent logic checks and other minor cleanups. → p3: Remove equivalent logic checks and other minor cleanups.
Attachment #8346329 - Attachment description: Migrate AudioLoop and related code into AudioSink class. → p4: Migrate AudioLoop and related code into AudioSink class.
Attachment #8346330 - Attachment description: AudioSink code cleanup. → p5: AudioSink code cleanup.
Attachment #8346331 - Attachment description: Split AudioLoop up into logical components. → p6: Split AudioLoop up into logical components.
Attachment #8346332 - Attachment description: Add OnAudioEndTimeUpdate and OnAudioSinkComplete to MediaDecoderStateMachine and call them from AudioSink. Split sleep out of Drain() into standalone SleepUntil(). → p7: Add OnAudioEndTimeUpdate and OnAudioSinkComplete to MediaDecoderStateMachine and call them from AudioSink. Split sleep out of Drain() into standalone SleepUntil().
Attachment #8346333 - Attachment description: Introduce GetEndTime to AudioSink rather than fetching mAudioEndTime from StateMachine. Misc. code cleanups. → p8: Introduce GetEndTime to AudioSink rather than fetching mAudioEndTime from StateMachine. Misc. code cleanups.
Attachment #8346334 - Attachment description: Remove more coupling between AudioSink and StateMachine. → p9: Remove more coupling between AudioSink and StateMachine.
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 on attachment 8346326 [details] [diff] [review]
p1: Move MediaDecoderStateMachine::AudioLoop and related code to AudioSink.cpp.

Simple code move, no logic changes.
Attachment #8346326 - Flags: review?(cpearce)
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()).
Attachment #8346327 - Flags: review?(cpearce)
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.
Attachment #8346328 - Flags: review?(cpearce)
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.
Attachment #8346329 - Flags: review?(cpearce)
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.
Attachment #8346330 - Flags: review?(cpearce)
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.
Attachment #8346331 - Flags: review?(cpearce)
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.
Attachment #8346332 - Flags: review?(cpearce)
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.
Attachment #8346333 - Flags: review?(cpearce)
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.
Attachment #8346334 - Flags: review?(cpearce)
And, of course, it doesn't build with --disable-unified-compilation.
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.
Attachment #8346355 - Attachment description: Fix build with --disable-unified-compilation. → p10: Fix build with --disable-unified-compilation.
Attachment #8346355 - Flags: review?(cpearce)
Attachment #8346326 - Flags: review?(cpearce) → review+
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.
Attachment #8346327 - Flags: review?(cpearce) → review+
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?
Attachment #8346328 - Flags: review?(cpearce) → review+
Attachment #8346330 - Flags: review?(cpearce) → review+
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.
Attachment #8346329 - Flags: review?(cpearce) → review+
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.
Attachment #8346331 - Flags: review?(cpearce) → review+
Attachment #8346332 - Flags: review?(cpearce) → review+
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.
Attachment #8346333 - Flags: review?(cpearce) → review+
Attachment #8346334 - Flags: review?(cpearce) → review+
Attachment #8346355 - Flags: review?(cpearce) → review+
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!
Attachment #8350347 - Attachment description: Address review comments and fix remaining orange. → p11: Address review comments and fix remaining orange.
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.
Attachment #8350347 - Flags: review?(cpearce) → review+
Assignee: kinetik → nobody
Assignee: nobody → kinetik
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
Attachment #8401090 - Flags: review?(cpearce)
Attachment #8401090 - Attachment description: bug948269_p12_v0.patch → p12: Make AudioStream init non-fatal again.
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...
Attachment #8401090 - Flags: review?(cpearce) → review+
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
Posted patch bug948269_followup_v0.patch (obsolete) — Splinter Review
This new assert was triggering during test_playback; it can happen if StartAudioStreamIfPlaybackNeeded hasn't triggered before we hit Drain().
Attachment #8401673 - Flags: review?(cpearce)
Attachment #8401673 - Flags: review?(cpearce) → review+
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
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.
This should fix at least one of the test failures: https://tbpl.mozilla.org/?tree=Try&rev=7ba644967aaa
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
Attachment #8346326 - Attachment is obsolete: true
Attachment #8346327 - Attachment is obsolete: true
Attachment #8346328 - Attachment is obsolete: true
Attachment #8346329 - Attachment is obsolete: true
Attachment #8346330 - Attachment is obsolete: true
Attachment #8346331 - Attachment is obsolete: true
Attachment #8346332 - Attachment is obsolete: true
Attachment #8346333 - Attachment is obsolete: true
Attachment #8346334 - Attachment is obsolete: true
Attachment #8346355 - Attachment is obsolete: true
Attachment #8350347 - Attachment is obsolete: true
Attachment #8401090 - Attachment is obsolete: true
Attachment #8401673 - Attachment is obsolete: true
Attachment #8445622 - Flags: review+
Depends on: 1020538
https://hg.mozilla.org/mozilla-central/rev/b559ec64da05
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1032266
Depends on: 1037423
No longer depends on: 1032266
You need to log in before you can comment on or make changes to this bug.