Closed
Bug 973408
Opened 11 years ago
Closed 11 years ago
Decode media data in runnables dispatched to the decode shared thread pool
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(9 files, 11 obsolete files)
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 1•11 years ago
|
||
Make MediaDecoderStateMachine::DecodeLoop()'s local variables into class members. This means we can make DecodeLoop re-entrant later.
Attachment #8384319 -
Flags: review?(kinetik)
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.
Attachment #8384321 -
Flags: review?(kinetik)
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.
Attachment #8384325 -
Flags: review?(kinetik)
Attachment #8384325 -
Flags: feedback?(sotaro.ikeda.g)
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.
Attachment #8384326 -
Flags: review?(kinetik)
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.
Attachment #8384327 -
Flags: review?(kinetik)
Assignee | ||
Comment 6•11 years ago
|
||
Perform audio and video decode in the pop listeners of their corresponding MediaQueue, if decoding is required.
Attachment #8384328 -
Flags: review?(kinetik)
Assignee | ||
Comment 7•11 years ago
|
||
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.
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)
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.
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.
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.
Attachment #8384319 -
Flags: review?(kinetik) → review+
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.
Attachment #8384321 -
Flags: review?(kinetik) → review+
Updated•11 years ago
|
Attachment #8384325 -
Flags: review?(kinetik) → review+
Updated•11 years ago
|
Attachment #8384326 -
Flags: review?(kinetik) → review+
Comment 13•11 years ago
|
||
Comment on attachment 8384325 [details] [diff] [review] Patch 2v1: Add NotifyWaitingForResourcesStatusChanged() Looks good.
Attachment #8384325 -
Flags: feedback?(sotaro.ikeda.g) → feedback+
Updated•11 years ago
|
Attachment #8384327 -
Flags: review?(kinetik) → review+
Updated•11 years ago
|
Attachment #8384328 -
Flags: review?(kinetik) → review+
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.
Attachment #8384329 -
Flags: review?(kinetik) → review+
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.
Attachment #8386371 -
Flags: review?(kinetik)
Assignee | ||
Comment 16•11 years ago
|
||
Once more because I'm paranoid: https://tbpl.mozilla.org/?tree=Try&rev=e883e84ca381
Updated•11 years ago
|
Attachment #8386371 -
Flags: review?(kinetik) → review+
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
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().
Updated•11 years ago
|
Attachment #8384329 -
Flags: feedback?(sotaro.ikeda.g)
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.
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?
Attachment #8386609 -
Flags: feedback?(sotaro.ikeda.g)
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.
Comment 22•11 years ago
|
||
Got logcat in the STR in Comment 18, after applying attachment 8386609 [details] [diff] [review].
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.
Comment 24•11 years ago
|
||
If UpdateIdleState() and triggering DecodeAudio() is unified, the problem might be fixed.
Updated•11 years ago
|
Attachment #8386609 -
Flags: feedback?(sotaro.ikeda.g)
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.
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. ;)
Attachment #8387239 -
Flags: feedback?(sotaro.ikeda.g)
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.
Attachment #8387239 -
Flags: feedback?(sotaro.ikeda.g) → feedback+
Assignee | ||
Comment 28•11 years ago
|
||
Patch 0, updated and rebased. r=kinetik
Attachment #8384319 -
Attachment is obsolete: true
Attachment #8388385 -
Flags: review+
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #8384321 -
Attachment is obsolete: true
Attachment #8386371 -
Attachment is obsolete: true
Attachment #8388388 -
Flags: review+
Assignee | ||
Comment 32•11 years ago
|
||
Attachment #8384325 -
Attachment is obsolete: true
Attachment #8384326 -
Attachment is obsolete: true
Attachment #8388391 -
Flags: review+
Assignee | ||
Comment 33•11 years ago
|
||
Attachment #8384327 -
Attachment is obsolete: true
Attachment #8388392 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8384328 -
Attachment is obsolete: true
Assignee | ||
Comment 35•11 years ago
|
||
Rebased, updated, split SetIdle(bool), to SetReaderIdle() and SetReaderActive() as per review comment.
Attachment #8384329 -
Attachment is obsolete: true
Attachment #8388394 -
Flags: review+
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.
Attachment #8386609 -
Attachment is obsolete: true
Attachment #8386812 -
Attachment is obsolete: true
Attachment #8387239 -
Attachment is obsolete: true
Attachment #8388396 -
Flags: review?(kinetik)
Updated•11 years ago
|
Attachment #8388396 -
Flags: review?(kinetik) → review+
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
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•