Closed
Bug 979104
Opened 10 years ago
Closed 10 years ago
Support async MediaDecoderReaders
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(4 files, 3 obsolete files)
141.37 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
48.81 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
3.39 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
3.94 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
Once bug 962385 lands, with a little work we can support running legacy synchronous MediaDecoderReaders and new asynchronous MediaDecoderReaders together. We should be able to support running both sync and async readers with the same code, we shouldn't need special code paths for each type. We need to: * Make FindStartTime() work when decoding is async. * Make DecodeToTarget() work when decoding is async. * Anything else required... I'm working on making MP4Reader async, will do the above and anything else required first.
Assignee | ||
Comment 1•10 years ago
|
||
Copy paste fail, I meant bug 973408 not bug 962385.
Assignee | ||
Comment 2•10 years ago
|
||
This is a work-in-progress patch that is green: https://tbpl.mozilla.org/?tree=Try&rev=6709f55e4438 It needs cleaning up, but the base structure is there. Feedback on structure/design welcome. Every MediaDecoderReader that wants to run async can change its interface to implement RequestAudio()/RequestVideo(), and callback with exactly one AudioData/VideoData when a sample is available. The default MediaDecoderReader implementation Adapts the async interface to the old synchronous one. So the existing Readers still work. This patch changes none of the existing MediaDecoderReaders to run asynchronously. Every MediaDecoderReader that wants to run async will need to be modified implement the new interface. I have a patch that successfully makes the WMFReader async, not uploading that just yet as I am focusing on getting the base landed first. I had to disable wegbl_conformance test tex-image-and-sub-image-2d-with-video.html on MacOSX 10.6. It's already disabled on WinXP. I think this test fails because we never reach readyState HAVE_FUTURE_DATA in this testcase on MacOSX. This may be because decoding is slightly slower on the single stream single media case with this patch. We should change our readyState logic to not depend on the number of frames decoded, but the amount of data downloaded. Then we'd fix a raft of similar random failures (mostly related to "canplaythrough" and "playing" events firing). We should do this in a separate bug.
Assignee | ||
Comment 3•10 years ago
|
||
* Add Request{Audio,Video}Data() functions to MediaDecoderReader. * Add callback interface that MediaDecoderReader uses to return samples requested by Request{Audio,Video}Data() functions. * StateMachine has a MediaDataDecodedListener that implements the MediaDecoderReader callback interface. The MediaDataDecodedListener receives samples, and delivers them in tasks on the decode task queue for enqueuing in the StateMachine. This ensures that it's safe to call the callbacks on any thread. * Provide MediaDecoderReader::Request{Audio,Video}Data() implementations that use the old synchronous API to fulfill the new API. * Make MediaDecoderReader refcounted, so that we can create RunnableMethods to it easily, for dispatching tasks to run its functions. * Add an explicit Shutdown() function to MediaDecoderReader. This is important for interrupting async operations. * Errors can now be reported using the MediaDecoderReader callback interface. * Seeking is implemented by calling MediaDecoderReader::Seek(), and remembering the seek target and decoding up until we've hit the target. * The skip-to-next-keyframe and ample-audio-threshold logic remains unchanged so far. We could probably tweak this once we have actual async Readers in the tree. * Still need to remove the FindStartTime() call in MediaSourceReader. Will do that in a follow up. Without this, async-only decoders won't work in MSE. Once we remove the FindStartTime() call, we can move FindStartTime() into OggReader, the only other user. * Add a "watchdog" to manifest.js that logs unfinished tests if a test takes longer than 10s to run. Useful for debugging timeouts! Sorry for the monster patch. Rebasing a patch queue is hell.
Attachment #8420713 -
Attachment is obsolete: true
Attachment #8422154 -
Flags: review?(kinetik)
Assignee | ||
Comment 4•10 years ago
|
||
I need to disable this test on MacOX10.6, as it's frequently failing with my other patch here applied due to the way we calculate whether we should fire the "playing" event when we're playing video being susceptible to the timing changes made in my patch here. Changing that is a non trivial amount of work, and we'll have to do that in a follow up bug. This test is already disabled on WinXP.
Attachment #8422156 -
Flags: review?(jgilbert)
Assignee | ||
Comment 5•10 years ago
|
||
Filed bug 1009999 to follow up on making HTMLMediaElement.readyState/playing event dispatch more reliable.
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8422154 [details] [diff] [review] Patch: Make MediaDecoderStateMachine handle async Readers Review of attachment 8422154 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaDecoderStateMachine.cpp @@ +58,5 @@ > } \ > PR_END_MACRO > +#define SAMPLE_LOG(msg, ...) \ > + PR_BEGIN_MACRO \ > + if (!PR_GetEnv("MEDIA_LOG_SAMPLES")) { \ I meant PR_GetEnv("MEDIA_LOG_SAMPLES") (no "!")...
Assignee | ||
Comment 7•10 years ago
|
||
Now that try is working again... https://tbpl.mozilla.org/?tree=Try&rev=1a96caa215ef
Comment 8•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #4) > Created attachment 8422156 [details] [diff] [review] > Patch disable faiing webgl video conformance test case > > I need to disable this test on MacOX10.6, as it's frequently failing with my > other patch here applied due to the way we calculate whether we should fire > the "playing" event when we're playing video being susceptible to the timing > changes made in my patch here. Changing that is a non trivial amount of > work, and we'll have to do that in a follow up bug. This test is already > disabled on WinXP. Is this a problem with the test or our implementation?
Flags: needinfo?(cpearce)
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #8) > (In reply to Chris Pearce (:cpearce) from comment #4) > > Created attachment 8422156 [details] [diff] [review] > > Patch disable faiing webgl video conformance test case > > > > I need to disable this test on MacOX10.6, as it's frequently failing with my > > other patch here applied due to the way we calculate whether we should fire > > the "playing" event when we're playing video being susceptible to the timing > > changes made in my patch here. Changing that is a non trivial amount of > > work, and we'll have to do that in a follow up bug. This test is already > > disabled on WinXP. > > Is this a problem with the test or our implementation? This is a problem with our implementation of how we determine when to fire the "playing" event at the video element. It will take a non-trivial amount of work to fix that. When we do we can re-enable this test on MacOSX (and WinXP).
Flags: needinfo?(cpearce)
Comment 10•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #9) > (In reply to Jeff Gilbert [:jgilbert] from comment #8) > > (In reply to Chris Pearce (:cpearce) from comment #4) > > > Created attachment 8422156 [details] [diff] [review] > > > Patch disable faiing webgl video conformance test case > > > > > > I need to disable this test on MacOX10.6, as it's frequently failing with my > > > other patch here applied due to the way we calculate whether we should fire > > > the "playing" event when we're playing video being susceptible to the timing > > > changes made in my patch here. Changing that is a non trivial amount of > > > work, and we'll have to do that in a follow up bug. This test is already > > > disabled on WinXP. > > > > Is this a problem with the test or our implementation? > > This is a problem with our implementation of how we determine when to fire > the "playing" event at the video element. It will take a non-trivial amount > of work to fix that. When we do we can re-enable this test on MacOSX (and > WinXP). Can you briefly explain why this is hard, since it doesn't seem hard? I ask because temporarily disabling a test pending future non-trivial work usually means we'll never get around to fixing it. I'm also confused why this is considered an acceptable regression, especially if it's going to negatively impact test coverage for a different module. With this patch, is there really no solid way to be notified when a video begins playing?
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #10) > (In reply to Chris Pearce (:cpearce) from comment #9) > > (In reply to Jeff Gilbert [:jgilbert] from comment #8) > > > (In reply to Chris Pearce (:cpearce) from comment #4) > > > > Created attachment 8422156 [details] [diff] [review] > > > > Patch disable faiing webgl video conformance test case > > > > > > > > I need to disable this test on MacOX10.6, as it's frequently failing with my > > > > other patch here applied due to the way we calculate whether we should fire > > > > the "playing" event when we're playing video being susceptible to the timing > > > > changes made in my patch here. Changing that is a non trivial amount of > > > > work, and we'll have to do that in a follow up bug. This test is already > > > > disabled on WinXP. > > > > > > Is this a problem with the test or our implementation? > > > > This is a problem with our implementation of how we determine when to fire > > the "playing" event at the video element. It will take a non-trivial amount > > of work to fix that. When we do we can re-enable this test on MacOSX (and > > WinXP). > > Can you briefly explain why this is hard, since it doesn't seem hard? If you don't think it's hard, you're welcome to write a patch. ;) It's hard because it requires us to rewrite the inner logic of our media stack. Every time we do a major change like that it takes us weeks to fix all the random orange that inevitably pop up. > I ask > because temporarily disabling a test pending future non-trivial work usually > means we'll never get around to fixing it. We have jwwang following up robustness and reliability issues like this. > I'm also confused why this is considered an acceptable regression, > especially if it's going to negatively impact test coverage for a different > module. Because this patch provides infrastructure for us to run our audio and video decoders on separate threads, and that will provide significant performance improvement once we have completed the process of making our decoders async. We need to make our decoders conform to the new interface before we'll see the big wins come from this, and we can't do that until this has landed. > With this patch, is there really no solid way to be notified when a video > begins playing? Currently this is not an event that denotes that we're playing, it denotes that we can *continue* playing, and that relies on the number of frames we've decoded and the network speed. Guesstimating the network rate and the decode rate is unreliable even without this patch; the test is already disabled on WinXP. This patch is a major step towards improving the robustnes, reliability and performance of our media stack, but we won't see the big gains until future work is also complete.
Comment 12•10 years ago
|
||
Alright, it sounds like the test is actually waiting on the wrong event, so we should fix that. Let's do that it bug 1012958.
Updated•10 years ago
|
Attachment #8422156 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Note to self: do not regress Bug 1008012 when landing this.
Comment 14•10 years ago
|
||
Comment on attachment 8422154 [details] [diff] [review] Patch: Make MediaDecoderStateMachine handle async Readers Review of attachment 8422154 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, a few minor issues commented on below. ::: content/media/MediaData.h @@ +53,5 @@ > > // Duration of sample, in microseconds. > const int64_t mDuration; > > + // True if this is the first sample after a gap or "discontinuity" in No need for scare quotes. ::: content/media/MediaDecoderReader.cpp @@ +260,5 @@ > + } > + ReleaseMediaResources(); > +} > + > +AudioDecodeBarrier::AudioDecodeBarrier() This isn't a barrier in the usual CS sense (by my interpretation, at least). What about a different name? Rendezvous? ::: content/media/MediaDecoderReader.h @@ +171,5 @@ > MediaInfo GetMediaInfo() { return mInfo; } > > protected: > > + // Decodes an unspecified amount of audio data, enqueuing the audio data Clarify that the comment for this and DecodeVideoFrame is intended to apply to the overrides, since it's obviously false for the implementation here and could confuse a newcomer. ::: content/media/MediaDecoderStateMachine.h @@ +7,2 @@ > > +Each video element for a media file has one thread called the "audio thread". s/video/media/ @@ +11,5 @@ > +hardware. This is done in a separate thread to ensure that the > +audio hardware gets a constant stream of data without > +interruption due to decoding or display. At some point > +AudioStream will be refactored to have a callback interface > +where it asks for data and an extra thread will no longer be s/an extra/this/ @@ +23,5 @@ > +samples into its queues for the consuming threads to pull from. > + > +The MediaDecoderReader can choose to decode asynchronously, or synchronously > +and return requested samples synchronously inside it's Request*Data() > +functions via callback. Asynchronous decoding is preferred. + "and should be used for any new readers"? @@ +29,4 @@ > Synchronisation of state between the thread is done via a monitor owned > by MediaDecoder. > > +The lifetime of the audio threads is controlled by the state machine when s/threads/thread/ @@ +69,5 @@ > Both queues are bounded by a maximum size. When this size is reached > +the decode tasks will no longer request video or audio depending on the > +queue that has reached the threshold. If both queues are full, no more > +decode tasks will be dispatched to the decode task queue, so other > +decoders will have an oppourtunity to run. opportunity @@ +322,2 @@ > MOZ_ASSERT(mReader); > if (mReader) { Seems weird to assert mReader and then handle it being null on the next line. @@ +373,5 @@ > > void AssertCurrentThreadInMonitor() const { mDecoder->GetReentrantMonitor().AssertCurrentThreadIn(); } > > + // Inserts MediaData* samples into their respective MediaQueues. > + // "Finishes" the queue if nullptr is passed in. Can't we just have the caller call Finish()? If not, remove the scare quotes and make it explicit that it simply calls Finish(). @@ +677,5 @@ > > + // These return true if the respective stream's decode has reached > + // the end of stream. > + bool IsAudioDecoding(); > + bool IsVideoDecoding(); Comment vs function naming/return value is confusing. @@ +792,5 @@ > // first acquire the decoder monitor and check that it is non-null. > RefPtr<AudioStream> mAudioStream; > > // The reader, don't call its methods with the decoder monitor held. > + // This is created in the play state machine's constructor. -play @@ +882,5 @@ > > + // This temporarily stores the previous frame we've seen while we're > + // decoding during a seek. This is so that if we hit end of stream while > + // we're seeking, we can still display the last frame. > + nsAutoPtr<VideoData> mFirstVideoFrameAfterSeek; I'm confused by previous/last/first. Can this be called mLastVideoFrameDecoded? ::: content/media/MediaQueue.h @@ +46,5 @@ > + if (aItem) { > + nsDeque::Push(aItem); > + } else { > + Finish(); > + } Can't we just have the caller call Finish()? If not, needs to be explicitly documented that it calls Finish() on null aItem. ::: content/media/VideoUtils.cpp @@ +194,5 @@ > > +TemporaryRef<SharedThreadPool> GetMediaDecodeThreadPool() > +{ > + return SharedThreadPool::Get(NS_LITERAL_CSTRING("Media Decode"), > + Preferences::GetUint("media.num-decode-threads", 25)); It'd be good if we could run mochitests with this set to 1 as well as with it at the default. ::: content/media/VideoUtils.h @@ +187,5 @@ > T& mVar; > const T mValue; > }; > > +TemporaryRef<SharedThreadPool> GetMediaDecodeThreadPool(); Needs a bit of documentation. ::: content/media/mediasource/MediaSourceDecoder.cpp @@ +94,5 @@ > > + void OnVideoDecoded(VideoData* aSample) > + { > + if (!aSample) { > + // End of stream. See if we can switch to another video decoder. Can we have another callback for end-of-stream? Relying on a null test and making a callback for handling a decoded sample do double duty feels odd. @@ +156,2 @@ > private: > + // These are written on the decode task queue threads, and read on the Incomplete comment. @@ +157,5 @@ > + // These are written on the decode task queue threads, and read on the > + int64_t mTimeThreshold; > + bool mDropVideoBeforeThreshold; > + > + bool SwitchVideoReaders() { Leave this called "Maybe..." since it's not guaranteed to switch. ::: content/media/omx/MediaOmxReader.cpp @@ +59,5 @@ > > mAudioChannel = dom::AudioChannelService::GetDefaultAudioChannel(); > } > > MediaOmxReader::~MediaOmxReader() Remove. ::: content/media/plugins/MediaPluginReader.cpp @@ +34,5 @@ > mAudioSeekTimeUs(-1) > { > } > > MediaPluginReader::~MediaPluginReader() Might as well remove this completely. Also, MediaDecoderReader's destructor still calls ResetDecode(), so this was never needed in the first place? Does MDR still need to call ResetDecode()? ::: content/media/test/manifest.js @@ +699,1 @@ > } Please remove the unnecessary whitespace and newlines. ::: content/media/test/test_bug493187.html @@ +52,5 @@ > + var v = e.target; > + info("[" + v._name + "] error"); > +} > + > + Here too. @@ +75,5 @@ > v.addEventListener("canplaythrough", canPlayThrough, false); > v.addEventListener("seeking", startSeeking, false); > + v.addEventListener("seeked", seeked, false); > + v.addEventListener("error", error, false); > + And here. ::: content/media/webaudio/MediaBufferDecoder.cpp @@ +269,5 @@ > + audioQueue.Push(audio.forget()); > + } else { > + // End of stream. > + break; > + } if (!audio) { // End of stream. break; } audioQueue.Push(audio.forget()); @@ +274,3 @@ > } > > + Whitespace?
Attachment #8422154 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 15•10 years ago
|
||
> > + // Inserts MediaData* samples into their respective MediaQueues. > > + // "Finishes" the queue if nullptr is passed in. > > Can't we just have the caller call Finish()? I was trying to avoid requiring the caller to remember to do this, since we've had bugs in the past when we forgot. The problem is largely removed by having an explicit EOS callback instead however. > ::: content/media/VideoUtils.cpp > @@ +194,5 @@ > > > > +TemporaryRef<SharedThreadPool> GetMediaDecodeThreadPool() > > +{ > > + return SharedThreadPool::Get(NS_LITERAL_CSTRING("Media Decode"), > > + Preferences::GetUint("media.num-decode-threads", 25)); > > It'd be good if we could run mochitests with this set to 1 as well as with > it at the default. I don't really want to double the runtime of our test suite... We could just re-run one test with the pref set, like test_playback or test_bug498187? > ::: content/media/omx/MediaOmxReader.cpp > @@ +59,5 @@ > > > > mAudioChannel = dom::AudioChannelService::GetDefaultAudioChannel(); > > } > > > > MediaOmxReader::~MediaOmxReader() > > Remove. We get compile errors if we do because the default dtor then needs to know the definitions of android::OmxDecoder and android::MediaExtractor. It's easier to just have the dtor in MediaOmxReader.cpp where those are defined than exporting and including all the things required to do this. https://tbpl.mozilla.org/?tree=Try&rev=b78f28d1705b
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #4) > Created attachment 8422156 [details] [diff] [review] > Patch disable faiing webgl video conformance test case > > I need to disable this test on MacOX10.6, as it's frequently failing with my > other patch here applied due to the way we calculate whether we should fire > the "playing" event when we're playing video being susceptible to the timing > changes made in my patch here. Changing that is a non trivial amount of > work, and we'll have to do that in a follow up bug. This test is already > disabled on WinXP. I realized the reason we're failing webgl conformance test tex-image-and-sub-image-2d-with-video.html on MacOSX 10.6 is because I wasn't doing skip-to-next-keyframe correctly in my adaption layer from the async to old sync decoders; MediaDecoderReader::RequestVideoData(). MediaDecoderReader::RequestVideoData() assumed that the Reader's DecodeVideoFrame() function didn't return until it had reached the next keyframe, but that's not how DecodeVideoFrame() works, we're supposed to keep calling it until it reports that it's reached the next keyframe. So we'd end up skipping all frames, and thus not filling up the video frame queue. Once I fixed that, webgl conformance test tex-image-and-sub-image-2d-with-video.html on MacOSX 10.6 seems green: https://tbpl.mozilla.org/?tree=Try&rev=64752a83946e
Assignee | ||
Comment 17•10 years ago
|
||
Patch 1 rebased on trunk. No review comments addressed, they're in the next patch.
Attachment #8422154 -
Attachment is obsolete: true
Attachment #8422156 -
Attachment is obsolete: true
Attachment #8436651 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
* Address review comments. * Add On{Audio,Video}EOS() to RequestSampleCallback. * Make the logic in MediaDecoderReader::RequestAudioData() a little more compact. * Make MediaDecoderReader::RequestVideoData() to the skip-to-next-keyframe properly. When we're supposed to skip, we'll call DecodeVideoFrame, and if it reports that it's not yet reached the time threshold, we'll dispatch another task to call us back again. We need to explicitly tell the MediaDecoderReader about the decode MediaTaskQueue in order to achieve this, which isn't a bad thing. This means other tasks get a chance to run (like the audio decode), and also means we'll eventually reach the next keyframe. Previously we'd bail out after skipping one frame, which was causing the webgl test to fail. * Break the reader <-> callback cycle in MediaDecoderReader::BreakCycles() rather than MediaDecoderReader::Shutdown(). This means that things that also need to override MediaDecoderReader::Shutdown() (MP4Reader specifically) no longer need to remember to *also* call MediaDecoderReader::Shutdown() inside their Shutdown() implementation. The cycle is now broken during the shutdown dance.
Attachment #8436652 -
Flags: review?(kinetik)
Updated•10 years ago
|
Attachment #8436652 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 19•10 years ago
|
||
Landed rollup: https://hg.mozilla.org/integration/mozilla-inbound/rev/3395ce618c91
Comment 20•10 years ago
|
||
Backed out for mochitest-1 hangs and leaks: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3395ce618c91&jobname=mochitest-1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f9a189da8b
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #18) > Created attachment 8436652 [details] [diff] [review] > Patch 2: Address review comments > * Break the reader <-> callback cycle in MediaDecoderReader::BreakCycles() > rather than MediaDecoderReader::Shutdown(). I made this change after my last try push, and with this change the MSE mochitests hang at shutdown.
Assignee | ||
Comment 22•10 years ago
|
||
I need to clear the Reader's task queue reference, and the MSE decoder needs pass BreakCycles() calls onto its subreaders. I also explictly call Shutdown() and BreakCycles() on the Reader used by WebAudio. Greenish: https://tbpl.mozilla.org/?tree=Try&rev=fbc367e53efe
Attachment #8437406 -
Flags: review?(kinetik)
Updated•10 years ago
|
Attachment #8437406 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 23•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cdef46c65c1
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0cdef46c65c1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 25•10 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+0] from comment #24) > https://hg.mozilla.org/mozilla-central/rev/0cdef46c65c1 This causes Bug 1024324 regression. We need to back out it soon.
Comment 26•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #25) > (In reply to Ed Morley [:edmorley UTC+0] from comment #24) > > https://hg.mozilla.org/mozilla-central/rev/0cdef46c65c1 > > This causes Bug 1024324 regression. We need to back out it soon. Sorry, I had to back this out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1583ed19bf0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 27•10 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d1583ed19bf0
Comment 28•10 years ago
|
||
I wonder why this wan't caught in test_playback.html. Although we can't play 3gp on b2g emulator, we should be still able to play mp4 (gizmo.mp4) and webm (seek.webm) files.
Comment 29•10 years ago
|
||
https://hg.mozilla.org/try/file/fbc367e53efe/content/media/test/mochitest.ini#l393 test_playback.html was disabled on B2G and Android... that's why...
Assignee | ||
Comment 30•10 years ago
|
||
Let's see, re-enabled test_playback with my patch here: https://tbpl.mozilla.org/?tree=Try&rev=5ed29feb3862 ... and re-enabled test_playback with what I think is a fix: https://tbpl.mozilla.org/?tree=Try&rev=58d7d0228396
Assignee | ||
Comment 31•10 years ago
|
||
Hmm, that's disappointing, tryserver didn't fail. Regardless, I've fixed the problem on my Otoro, patch to follow...
Comment 32•10 years ago
|
||
https://hg.mozilla.org/try/rev/5ed29feb3862 I guess you enabled the wrong test...
Assignee | ||
Comment 33•10 years ago
|
||
Oh, the skip annotation goes after the line... How odd...
Assignee | ||
Comment 34•10 years ago
|
||
Ensure that the MediaDecoderStateMachine's sample queues are empty before we call MediaDecoderReader::Shutdown().
Attachment #8441748 -
Flags: review?(kinetik)
Comment 35•10 years ago
|
||
Comment on attachment 8441748 [details] [diff] [review] 979104-b2g-shutdown.patch Review of attachment 8441748 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaDecoderStateMachine.cpp @@ +2526,5 @@ > StopPlayback(); > } > > + // Put a task in the decode queue to abort any decoding operations. > + // The reader is not supposed to put any tasks to deliver sampels into Typo @@ +2534,5 @@ > + mDecodeTaskQueue->Dispatch(task); > + > + { > + // Wait for the thread decoding to complete aborting decoding > + // operations and run any pending callbacks. This is important, as we "to abort decoding" reads a little better @@ +2535,5 @@ > + > + { > + // Wait for the thread decoding to complete aborting decoding > + // operations and run any pending callbacks. This is important, as we > + // don't want any pending tasks posted to the task queue by the reader Is there a way we can detect and assert on this?
Attachment #8441748 -
Flags: review?(kinetik) → review+
Comment 36•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #33) > Oh, the skip annotation goes after the line... How odd... This is why, when I converted the test manifest to this format, I put the directives on the same line: https://hg.mozilla.org/try/annotate/7f083546aa95/content/media/test/mochitest.ini It's a shame someone changed that.
Assignee | ||
Comment 37•10 years ago
|
||
> @@ +2535,5 @@
> > +
> > + {
> > + // Wait for the thread decoding to complete aborting decoding
> > + // operations and run any pending callbacks. This is important, as we
> > + // don't want any pending tasks posted to the task queue by the reader
>
> Is there a way we can detect and assert on this?
That's a good idea. It's not immediately obvious how to do this in a clean and threadsafe fashion, so I'll file a follow up.
Assignee | ||
Comment 38•10 years ago
|
||
Pushed again with fix merged, and rebased. https://hg.mozilla.org/integration/mozilla-inbound/rev/5519fd827576
Comment 39•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5519fd827576
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•