Support async MediaDecoderReaders

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

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

5 years ago
Copy paste fail, I meant bug 973408 not bug 962385.
Depends on: 973408
No longer depends on: 962385
(Assignee)

Updated

5 years ago
Depends on: 1002266
(Assignee)

Comment 2

5 years ago
Posted patch WIP Patch (obsolete) — Splinter Review
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

5 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

5 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

5 years ago
Filed bug 1009999 to follow up on making HTMLMediaElement.readyState/playing event dispatch more reliable.
(Assignee)

Comment 6

5 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

5 years ago
Now that try is working again...
https://tbpl.mozilla.org/?tree=Try&rev=1a96caa215ef
(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

5 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)
(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

5 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.
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.
Attachment #8422156 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 13

5 years ago
Note to self: do not regress Bug 1008012 when landing this.
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

5 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

5 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

5 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

5 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)
Attachment #8436652 - Flags: review?(kinetik) → review+
(Assignee)

Comment 21

5 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

5 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)
Attachment #8437406 - Flags: review?(kinetik) → review+
https://hg.mozilla.org/mozilla-central/rev/0cdef46c65c1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(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

5 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 → ---
Depends on: 1025768
Depends on: 1024324
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.
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

5 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

5 years ago
Hmm, that's disappointing, tryserver didn't fail. Regardless, I've fixed the problem on my Otoro, patch to follow...
https://hg.mozilla.org/try/rev/5ed29feb3862
I guess you enabled the wrong test...
(Assignee)

Comment 33

5 years ago
Oh, the skip annotation goes after the line... How odd...
(Assignee)

Comment 34

5 years ago
Ensure that the MediaDecoderStateMachine's sample queues are empty before we call MediaDecoderReader::Shutdown().
Attachment #8441748 - Flags: review?(kinetik)
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+
(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

5 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.
https://hg.mozilla.org/mozilla-central/rev/5519fd827576
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 1028742
Depends on: 1028748
Depends on: 1028144
Depends on: 1029094

Updated

5 years ago
Depends on: 1030527
Depends on: 1024405
(Assignee)

Updated

5 years ago
Depends on: 1031178
(Assignee)

Updated

5 years ago
Blocks: 1032633
(Assignee)

Updated

5 years ago
Blocks: 1033055
Depends on: 1033123
Depends on: 1024517
Depends on: 1052378
(Assignee)

Updated

5 years ago
Depends on: 1053008
You need to log in before you can comment on or make changes to this bug.