Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Description • 11 years ago

The work in this bug is based on top of patches in bug 968016.

We should change our media decoding to instead call MediaDecoderReader::Decode{AudioData,VideoFrame}() from inside a runnable dispatched to the decode MediaTaskQueue. This is instead of every playing/decoding MediaDecoderReader having one dedicated thread on which it blocks calling the Decode* functions inside DecodeLoop().

This means we won't have to block one thread per running MediaDecoderReader. Dispatching runnables to call MediaDecoderReader::Decode*() to a thread pool means that we can have more decoders running concurrently, since each media element takes its turn decoding a single or small number of samples, then the next elements gets a turn. This improves our scalability, and means we can reduce the number of threads we use for decoding.

(We're still doing sync I/O in most (all?) of our MediaDecoderReader, so we're not yet at the point where we can safely have one decode thread per CPU core).

This also means that the MediaDecoderReaders can treat the MediaDecoderReader::Decode*() calls as a "need more input" signal, and return immediately and do their decoding asynchronously (like the MP4Reader /almost/ does today) if they wish. This will mean we can support asynchronous decoding, and we won't need to have audio and video decoding blocking and being performed on the same thread. So we won't need to buffer as much decoded audio for async decoders.

The runnable to call MediaDecoderReader::Decode*() is dispatched whenever the state machine pops a sample off the MediaQueue's to be rendered, provided the MediaQueue isn't "full". The MediaDecoderReader::Decode*() function (or some other async process) still needs to push MediaData samples onto the MediaQueues.

Thus we can support our legacy synchronous decoders alongside a new async decoding model in the same design.

I have working and (greenish looking) patches here:
https://tbpl.mozilla.org/?tree=Try&rev=bb31503af1ac

Before I can request review I need to ensure I'm calling MediaOmxReader::OnDecodeThread{Start,Finish}(), and be sure that failure on "B2G ICS Emulator Opt mtest-3" isn't a new orange.

On my Windows 8 laptop an opt Firefox build with my patches I can concurrently play 50 videos in the same page (with the videos stored on disk so the network isn't an issue) and they all play back more-or-less together and roughly in sync, with a few dropped frames. IE plays all videos a little more choppily than we do. Chrome manages to play the audio tracks and that's it.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 1 • 11 years ago

Make MediaDecoderStateMachine::DecodeLoop()'s local variables into class members. This means we can make DecodeLoop re-entrant later.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 2 • 11 years ago

Split up the cases in DecodeThreadRun() into their own decode tasks. This is so that we can dispatch the specific task we need to action a particular decoder state.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 3 • 11 years ago

Change how MediaDecoderReaders notify the MediaDecoderStateMachine that they have acquired resources required to decode. We used to notify decoder monitor. Now with this patch we call a function which dispatches a decode metadata task.

Note that we used to wait in DecodeThreadRun() while we were waiting for resources, but I removed that in the previous patch. Now we exit the decode metadata task instead. So now when the MediaDecoderReader re-acquires the resources it needs it cause another decode metadata task to run to complete the task.

This all means we don't need to block a thread while waiting to acquire decoder resources.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 4 • 11 years ago

Add pop listeners to MediaQueue. We can then use this to run the decode when data is popped in a later patch.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 5 • 11 years ago

Split DecodeLoop into its audio and video decoding sub-operations. This means we can decode audio and video independently in a later patch.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 6 • 11 years ago

Perform audio and video decode in the pop listeners of their corresponding MediaQueue, if decoding is required.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 8 • 11 years ago

Comment on attachment 8384329 [details] [diff] [review]
Patch 6v1: Inform readers when they're "idle", instead of having OnDecodeThreadStart

(Hit enter by mistake while uploading this...)

It doesn't make sense any more to have MediaDecoderReader::OnDecodeThread{Start, Finish}(). We don't need it on Windows anymore, as mozilla::SharedThreadPool takes care of initializing MSCOM for MediaDecoder and WebAudio clients. The only remaining user is the B2G backend, and that uses it as a way to signal that the decoders won't be used for a while, and can be "paused" and put into a low power mode.

So this patch adds the concept of the Reader being "idle". The state machine sets the Reader as idle when we don't need to decode (the MediaQueues are full) and the HTMLMediaElement is paused. The OMX and B2G RTSP Readers use this to pause/play the OMX decoder, putting it in low power mode.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 9 • 11 years ago

This is pretty green, except for known orange:

https://tbpl.mozilla.org/?tree=Try&rev=ff110eb9436a
https://tbpl.mozilla.org/?tree=Try&rev=1fc17ca86fb0
https://tbpl.mozilla.org/?tree=Try&rev=40544caa2f8d

Also, content/media/test/test_mediarecorder_getencodeddata.html is failing on MacOSX 10.6 Debug almost perma. Randy is looking at this.

Also in the above try pushes I re-enabled the Windows mochitests that were disabled. I think we can try re-enabling these on m-c after my patches here land, as I've fixed a number of oranges while getting these patches green. The tests disabled on Windows pass consistently now. I've got just about got the high try over the past few weeks testing these patches, so I'm pretty confident in re-enabling these tests.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 10 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #9)
> Also, content/media/test/test_mediarecorder_getencodeddata.html is failing
> on MacOSX 10.6 Debug almost perma. Randy is looking at this.

And this failure I don't understand, as AFAICT we don't even instantiate MediaDecoder on this test.

	Matthew Gregan [:kinetik]
	Comment 11 • 11 years ago

Comment on attachment 8384319 [details] [diff] [review]
Patch 0v1: Make MediaDecoderStateMachine::DecodeLoop()'s state class members

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +206,4 @@
>    mBufferingWait = mRealTime ? 0 : BUFFERING_WAIT_S;
>    mLowDataThresholdUsecs = mRealTime ? 0 : LOW_DATA_THRESHOLD_USECS;
>  
> +  mVideoPrerollFrames = mRealTime ? 0 : GetAmpleVideoFrames() / 2;

I think with the rest of these changes we can get rid of GetAmpleVideoFrames() completely.

	Matthew Gregan [:kinetik]
	Comment 12 • 11 years ago

Comment on attachment 8384321 [details] [diff] [review]
Patch 1v1: Split DecodeThreadRun() into separate decode tasks

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1647,5 @@
>      res = mReader->ReadMetadata(&info, &tags);
>    }
> +  if (NS_SUCCEEDED(res) &&
> +      (mState == DECODER_STATE_DECODING_METADATA) &&
> +      (mReader->IsWaitingMediaResources())) {

Drop the extraneous parens around the last two tests.

	Sotaro Ikeda [:sotaro]
	Comment 13 • 11 years ago

Comment on attachment 8384325 [details] [diff] [review]
Patch 2v1: Add NotifyWaitingForResourcesStatusChanged()

Looks good.

	Matthew Gregan [:kinetik]
	Comment 14 • 11 years ago

Comment on attachment 8384329 [details] [diff] [review]
Patch 6v1: Inform readers when they're "idle", instead of having OnDecodeThreadStart

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

::: content/media/MediaDecoderReader.h
@@ +83,5 @@
> +  // during initialization). When the media element is paused and we don't
> +  // need to decode any more data, the state machine sets the reader idle.
> +  // The reader can use this notification to enter a low power state when
> +  // the decoder isn't needed, if desired. This is most useful on mobile.
> +  virtual void SetIdle(bool aIsIdle) { }

Prefer SetIdle/SetActive pair rather than having to remember/guess what the bool param means.

::: content/media/MediaDecoderStateMachine.cpp
@@ +559,5 @@
>    if (mState != DECODER_STATE_DECODING && mState != DECODER_STATE_BUFFERING) {
>      mDispatchedVideoDecodeTask = false;
>      return;
>    }
> +  EnsureNotIdle();

EnsureActive()?

::: content/media/omx/RtspOmxReader.cpp
@@ +307,5 @@
>      if (controller) {
> +      if (aIsIdle) {
> +        controller->Pause();
> +      } else {
> +        controller->Play();  

Trailing space.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 15 • 11 years ago

As suggested, we can remove MediaDecoderStateMachine::GetAmpleVideoFrames(). This also means we can remove MediaOMXStateMachine. We now rely on the pref media.video-queue.default-size to set the media queue ample frame size.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 16 • 11 years ago

Once more because I'm paranoid:
https://tbpl.mozilla.org/?tree=Try&rev=e883e84ca381

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 17 • 11 years ago

Compile errors on B2G in the last try push, fixed and re-pushed to try (B2G only) here:
https://tbpl.mozilla.org/?tree=Try&rev=2e35bd2a18f9

	Sotaro Ikeda [:sotaro]
	Comment 18 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #7)
> Created attachment 8384329 [details] [diff] [review]
> Patch 6v1: Inform readers when they're "idle", instead of having
> OnDecodeThreadStart

I confirmed how the patch works on master hamachi. I tested the following STR. But OmxDecoder::Pause() was not called even in the following STR.
- [1] Start Music App
- [2] Start mp3 audio playback.
- [3] Pause audioplayback

By the change UpdateIdleState() becomes harder to track. Then UpdateIdleState() call seems missing somewhere. It seems difficult to logically judge the correctness of UpdateIdleState() calls. In future, it might be better to change the code as easier to understand the correctness of calling UpdateIdleState().

	Sotaro Ikeda [:sotaro]
	Comment 19 • 11 years ago

In the STR in Comment 18, there is a case that OmxDecoder::Pause() is called. But almost all cases, I did not see OmxDecoder::Pause() called.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 20 • 11 years ago

Thanks for testing the patches Sotaro. That's very valuable feedback. I can't reproduce the problem myself, but I think I understand the problem. Can you please apply the following patch and see if the problem still occurs?

	Sotaro Ikeda [:sotaro]
	Comment 21 • 11 years ago

I checked attachment 8386609 [details] [diff] [review] on hamachi. It did not work. The problem was at NeedToDecodeAudio().

In the last DecodeAudio() call, NeedToDecodeAudio() return false. But after that NeedToDecodeAudio() returns true. But DecodeAudio() is not triggered, therefore NeedToDecodeAudio() remain true forever.

	Sotaro Ikeda [:sotaro]
	Comment 22 • 11 years ago

Got logcat in the STR in Comment 18, after applying attachment 8386609 [details] [diff] [review].

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 23 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> I checked attachment 8386609 [details] [diff] [review] on hamachi. It did
> not work. The problem was at NeedToDecodeAudio().
> 
> In the last DecodeAudio() call, NeedToDecodeAudio() return false. But after
> that NeedToDecodeAudio() returns true. But DecodeAudio() is not triggered,
> therefore NeedToDecodeAudio() remain true forever.

This must be because NeedToDecodeAudio() depends on GetDecodedAudioDuration(), which takes into account the amount of data that has been pushed into the audio stream for playback, but has not yet been played. After pausing there's a slight delay in between the pause happening, and playback actually stopping. During that time, we must be playing enough audio to push us under the the ample audio threshold, so we think we need to decode more audio. But we're not advancing the decode enough such that we pop a sample off the audio queue, and cause us to actually decode more data.

I will find a solution for this.

	Sotaro Ikeda [:sotaro]
	Comment 24 • 11 years ago

If UpdateIdleState() and triggering DecodeAudio() is unified, the problem might be fixed.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 25 • 11 years ago

UpdateIdleState() is already called from DecodeAudio() if NeedToDecodeAudio() returns false (same thing happens for video), so it should not need to be unified.

Unless the return value of NeedToDecodeAudio() is changing between the call to NeedToDecodeAudio() in DecodeAudio() and the call to NeedToDecodeAudio() inside UpdateIdleState() called by DecodeAudio()? It could happen I guess.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 26 • 11 years ago

Sotaro: can you test this patch please? I think it should fix the problem.

This changes UpdateIdleState() to dispatch tasks to decode more if needed. Basically unifying the logic as you suggested. ;)

	Sotaro Ikeda [:sotaro]
	Comment 27 • 11 years ago

Comment on attachment 8387239 [details] [diff] [review]
Patch v2: Hopefully fix B2G idle

Good! OmxDecoder::Play() and OmxDecoder::Pause() are called as expected.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 28 • 11 years ago

Patch 0, updated and rebased. r=kinetik

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 29 • 11 years ago

Patch 1 (formerly 0.5).

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 35 • 11 years ago

Rebased, updated, split SetIdle(bool), to SetReaderIdle() and SetReaderActive() as per review comment.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 36 • 11 years ago

Merge the logic that checks if we need to dispatch more decode tasks with the logic to set the reader idle, and make decisions as to whether we need to decode based on a consistent value of NeedToDecodeAudio().

The return value of NeedToDecodeAudio() can change between calls, so we need to cache its return value to ensure our logic behaves consistently.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 37 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=94f7a469d1be

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 38 • 11 years ago

Landed for Gecko 30.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5645f7981c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9dfc05b1be9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e78ec92e1c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcaba0ea4352
https://hg.mozilla.org/integration/mozilla-inbound/rev/8615140a33ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b88c88f821
https://hg.mozilla.org/integration/mozilla-inbound/rev/a029b4912051
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ddb43c62d33
https://hg.mozilla.org/integration/mozilla-inbound/rev/c234a1aeaeab

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/b5645f7981c3
https://hg.mozilla.org/mozilla-central/rev/d9dfc05b1be9
https://hg.mozilla.org/mozilla-central/rev/5e78ec92e1c1
https://hg.mozilla.org/mozilla-central/rev/dcaba0ea4352
https://hg.mozilla.org/mozilla-central/rev/8615140a33ca
https://hg.mozilla.org/mozilla-central/rev/67b88c88f821
https://hg.mozilla.org/mozilla-central/rev/a029b4912051
https://hg.mozilla.org/mozilla-central/rev/7ddb43c62d33
https://hg.mozilla.org/mozilla-central/rev/c234a1aeaeab