Closed Bug 973408 Opened 6 years ago Closed 6 years ago

Decode media data in runnables dispatched to the decode shared thread pool

Categories

(Core :: Audio/Video, defect)

29 Branch
x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(9 files, 11 obsolete files)

19.39 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
6.15 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
17.99 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
9.14 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
2.97 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
23.49 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
28.69 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
26.65 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
13.67 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
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.
Blocks: 973705
Depends on: 973235
Blocks: 752796
Make MediaDecoderStateMachine::DecodeLoop()'s local variables into class members. This means we can make DecodeLoop re-entrant later.
Attachment #8384319 - Flags: review?(kinetik)
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.
Attachment #8384321 - Flags: review?(kinetik)
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.
Attachment #8384325 - Flags: review?(kinetik)
Attachment #8384325 - Flags: feedback?(sotaro.ikeda.g)
Add pop listeners to MediaQueue. We can then use this to run the decode when data is popped in a later patch.
Attachment #8384326 - Flags: review?(kinetik)
Split DecodeLoop into its audio and video decoding sub-operations. This means we can decode audio and video independently in a later patch.
Attachment #8384327 - Flags: review?(kinetik)
Perform audio and video decode in the pop listeners of their corresponding MediaQueue, if decoding is required.
Attachment #8384328 - Flags: review?(kinetik)
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.
Attachment #8384329 - Attachment description: Patch 6v1: Inform readers when they're "idle", instead of having OnStart → Patch 6v1: Inform readers when they're "idle", instead of having OnDecodeThreadStart
Attachment #8384329 - Flags: review?(kinetik)
Attachment #8384329 - Flags: feedback?(sotaro.ikeda.g)
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.
(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.
Depends on: 861136
Blocks: 979104
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.
Attachment #8384319 - Flags: review?(kinetik) → review+
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.
Attachment #8384321 - Flags: review?(kinetik) → review+
Attachment #8384325 - Flags: review?(kinetik) → review+
Attachment #8384326 - Flags: review?(kinetik) → review+
Comment on attachment 8384325 [details] [diff] [review]
Patch 2v1: Add NotifyWaitingForResourcesStatusChanged()

Looks good.
Attachment #8384325 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Attachment #8384327 - Flags: review?(kinetik) → review+
Attachment #8384328 - Flags: review?(kinetik) → review+
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.
Attachment #8384329 - Flags: review?(kinetik) → review+
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.
Attachment #8386371 - Flags: review?(kinetik)
Attachment #8386371 - Flags: review?(kinetik) → review+
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
(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().
Attachment #8384329 - Flags: feedback?(sotaro.ikeda.g)
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.
Attached patch Patch: Hopefully fix idle on B2G (obsolete) — Splinter Review
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?
Attachment #8386609 - Flags: feedback?(sotaro.ikeda.g)
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.
Attached file logcat with manually added logs (obsolete) —
Got logcat in the STR in Comment 18, after applying attachment 8386609 [details] [diff] [review].
(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.
If UpdateIdleState() and triggering DecodeAudio() is unified, the problem might be fixed.
Attachment #8386609 - Flags: feedback?(sotaro.ikeda.g)
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.
Attached patch Patch v2: Hopefully fix B2G idle (obsolete) — Splinter Review
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. ;)
Attachment #8387239 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 8387239 [details] [diff] [review]
Patch v2: Hopefully fix B2G idle

Good! OmxDecoder::Play() and OmxDecoder::Pause() are called as expected.
Attachment #8387239 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Patch 0, updated and rebased. r=kinetik
Attachment #8384319 - Attachment is obsolete: true
Attachment #8388385 - Flags: review+
Patch 1 (formerly 0.5).
Attachment #8388386 - Flags: review+
Attachment #8384321 - Attachment is obsolete: true
Attachment #8386371 - Attachment is obsolete: true
Attachment #8388388 - Flags: review+
Attachment #8384325 - Attachment is obsolete: true
Attachment #8384326 - Attachment is obsolete: true
Attachment #8388391 - Flags: review+
Attachment #8384328 - Attachment is obsolete: true
Rebased, updated, split SetIdle(bool), to SetReaderIdle() and SetReaderActive() as per review comment.
Attachment #8384329 - Attachment is obsolete: true
Attachment #8388394 - Flags: review+
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.
Attachment #8386609 - Attachment is obsolete: true
Attachment #8386812 - Attachment is obsolete: true
Attachment #8387239 - Attachment is obsolete: true
Attachment #8388396 - Flags: review?(kinetik)
Attachment #8388396 - Flags: review?(kinetik) → review+
Depends on: 982032
Depends on: 1005622
You need to log in before you can comment on or make changes to this bug.