Closed Bug 1155432 Opened 5 years ago Closed 5 years ago

Remove MediaTaskQueue::Flush() usage from WMFMediaDataDecoder

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Flushing task queues is inherently dangerous, and we should remove it from our code base. This bug covers removing it from WMFMediaDataDecoder.
Depends on: 1155433
A simpler patch that doesn't rely on machinery in bug 1155433. Maybe we should uplift this, as I think it may help with the shutdown hangs we're seeing.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62011557bafc
Remove the MediaTaskQueue flushes. I don't want to wait for the machinery that bholley is working on to make task queue tasks opportunistically cancel-able, as that should not be uplifted, and I think getting rid of the flushes should help with that shutdown hang and orange and so should be uplifted.
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Attachment #8594297 - Flags: review?(jyavenard)
And I think we should rewrite this to use whatever bholley comes up with in bug 1155433, once we have that.
Comment on attachment 8594297 [details] [diff] [review]
Patch: Bespoke flush

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

What about the other platforms? They all flush the task queue when Flush() is called.

Also, this implementation doesn't take care of theoretical case where we have multiple pending Input() in the task queue (isn't that what MediaTaskQueue::Flush( would have handled?) as only the first processed Input() will clear mIsFlushing and next ones would still add samples to the queue.

::: dom/media/fmp4/wmf/WMFMediaDataDecoder.cpp
@@ +55,5 @@
>      NS_NewRunnableMethod(this, &WMFMediaDataDecoder::ProcessShutdown));
>  #ifdef DEBUG
>    if (NS_FAILED(rv)) {
>      NS_WARNING("WMFMediaDataDecoder::Shutdown() dispatch of task failed!");
>    }

this in theory seems wrong to me.
You shutdown the MFTMenager and decoder, then wait for any pending decoding to complete.
Pending decoding that could make use of mMFTManager (that is now nullptr).

Now in practice it doesn't matter as the reader would have called Flush() prior to calling Shutdown() so there's nothing to wait on anymore ; plus the ProcessShutdown() task won't be called until Decode() task has completed.

But for clarity sake, either the call to ProcessShutdown is to be done after we've waited for pending decoding to complete, or not bother waiting at all with a line explaining why.

@@ +107,5 @@
> +    {
> +      MonitorAutoLock mon(mMonitor);
> +      MOZ_ASSERT(mIsRunning);
> +      if (mInput.empty()) {
> +        if (mIsFlushing) {

seems a bit convoluted to do all of this when we know if mIsFlushing is true then the queue is empty

@@ +124,5 @@
> +
> +    HRESULT hr = mMFTManager->Input(input);
> +    if (FAILED(hr)) {
> +      NS_WARNING("MFTManager rejected sample");
> +      mCallback->Error();

I think that in case of error, the queue should be emptied for clarity

@@ +159,5 @@
>  {
> +  MonitorAutoLock mon(mMonitor);
> +  while (!mInput.empty()) {
> +    nsRefPtr<MediaRawData> sample(mInput.front());
> +    mInput.pop();

why use an intermediary sample object?
mInput is a queue of nsRefPtr, just doing mInput.pop() is enough, as queue::pop will call the element destructor.

::: dom/media/fmp4/wmf/WMFMediaDataDecoder.h
@@ +97,5 @@
>    // This is used to approximate the decoder's position in the media resource.
>    int64_t mLastStreamOffset;
> +
> +  Monitor mMonitor;
> +  std::queue<nsRefPtr<MediaRawData>> mInput;

There's the MediaSampleQueue object defined in MP4Reader, maybe we should generalise that object

@@ +98,5 @@
>    int64_t mLastStreamOffset;
> +
> +  Monitor mMonitor;
> +  std::queue<nsRefPtr<MediaRawData>> mInput;
> +  bool mIsRunning;

I think the name mIsRunning is a bit confusing (and it got me confused trying to understand how things worked), as mRunning indicates that a sample is currently being decoded and not that the entire decoder is "running". So that Shutdown() doesn't change mRunning.

mIsDecoding would seem more appropriate to describe what this boolean represent.
Attachment #8594297 - Flags: review?(jyavenard) → review-
Comment on attachment 8594297 [details] [diff] [review]
Patch: Bespoke flush

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

::: dom/media/fmp4/wmf/WMFMediaDataDecoder.cpp
@@ +55,5 @@
>      NS_NewRunnableMethod(this, &WMFMediaDataDecoder::ProcessShutdown));
>  #ifdef DEBUG
>    if (NS_FAILED(rv)) {
>      NS_WARNING("WMFMediaDataDecoder::Shutdown() dispatch of task failed!");
>    }

I don't shutdown the MFTManager and then wait for decoding to complete. It's more like the opposite.

ProcessShutdown() is not run synchronously, it's put on the end of the task queue. So if we have a task to run ProcessDecode() already in the queue it will run before the ProcessShutdown() task runs. So it's more like the opposite; if we have a ProcessDecode() task running, it'll finish first, then we'll destroy the MFTManager.

@@ +107,5 @@
> +    {
> +      MonitorAutoLock mon(mMonitor);
> +      MOZ_ASSERT(mIsRunning);
> +      if (mInput.empty()) {
> +        if (mIsFlushing) {

We need way to signal that we call mDecoder->Flush(). We don't want to do it every time we've pushed all our input into the MFT; only when we need the decoder to drop anything it's buffered.
(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8594297 [details] [diff] [review]
> Patch: Bespoke flush
> 
> Review of attachment 8594297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What about the other platforms? They all flush the task queue when Flush()
> is called.

One at a time. The WMF backend is also the one I'm most familiar with, and the one we have that shutdown hang bug filed against.


> Also, this implementation doesn't take care of theoretical case where we
> have multiple pending Input() in the task queue (isn't that what
> MediaTaskQueue::Flush( would have handled?) as only the first processed
> Input() will clear mIsFlushing and next ones would still add samples to the
> queue.

I don't think that case is a problem.

Note there are two task queues here; the "decode" task queue, and the "video decode" task queue (WMFMediaDataDecoder::mTaskQueue). They're not the same task queue. We were flushing WMFMediaDataDecoder::mTaskQueue, not the decode task queue. Calls to WMFMediaDataDecoder::Input() happen on the decode task queue, so they won't be affected by flushing WMFMediaDataDecoder::mTaskQueue.

Also note that MediaDataDecoder::Flush() is supposed to block until it's completed flushing everything that was input before the flush. So if we had Flush() and Input() tasks in the decode task queue, it's correct for everything before the Flush() to be dropped by the time the Flush() finishes, and the Input() tasks in the decode task queue after the Flush() to not be flushed.
(In reply to Chris Pearce (:cpearce) from comment #6)

> Also note that MediaDataDecoder::Flush() is supposed to block until it's
> completed flushing everything that was input before the flush. So if we had
> Flush() and Input() tasks in the decode task queue, it's correct for
> everything before the Flush() to be dropped by the time the Flush()
> finishes, and the Input() tasks in the decode task queue after the Flush()
> to not be flushed.

but it doesn't wait for all Input() to be completed here. It only waits for the first one to complete (as the first one will clear mIsFlushing(), and any samples added by pending Input() wouldn't be dropped either.

It's the EnsureDecodeTaskDispatched started by Flush() that should clear mIsFlushing, and any samples being input between the time Flush() is called, and EnsureDecodeTaskDispatched started by flush run, should be dismissed.

or am I missing something?
Comment on attachment 8594297 [details] [diff] [review]
Patch: Bespoke flush

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

following vidyo talk...
Attachment #8594297 - Flags: review- → review+
Should uplift this once it's stuck...
Flags: needinfo?(cpearce)
Windows says: "WMFMediaDataDecoder.cpp(53) : error C2440: 'initializing' : cannot convert from 'void' to 'nsresult'", https://treeherder.mozilla.org/logviewer.html#?job_id=9048555&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b5b40f9e1d
https://hg.mozilla.org/mozilla-central/rev/f550eb7809b6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8594297 [details] [diff] [review]
Patch: Bespoke flush

Approval Request Comment
[Feature/regressing bug #]:Video playback on Windows
[User impact if declined]: I think this patch probably fixes some potential shutdown hangs on Windows after video playback.
[Describe test coverage new/current, TreeHerder]: We have lots of mochitests covering video playback.
[Risks and why]: I think it's lowish risk; it could adversely affect video playback, but the tests are looking good with this patch.
[String/UUID change made/needed]: None
Attachment #8594297 - Flags: approval-mozilla-beta?
Attachment #8594297 - Flags: approval-mozilla-aurora?
Blocks: 1129455
Comment on attachment 8594297 [details] [diff] [review]
Patch: Bespoke flush

Anything which fix the shutdown hang is good. Taking the risk.
Should be in 38 beta 7.
Attachment #8594297 - Flags: approval-mozilla-beta?
Attachment #8594297 - Flags: approval-mozilla-beta+
Attachment #8594297 - Flags: approval-mozilla-aurora?
Attachment #8594297 - Flags: approval-mozilla-aurora+
Comment on attachment 8594297 [details] [diff] [review]
Patch: Bespoke flush

[Triage Comment]
uplift to m-r as we did the 38 merge already
Attachment #8594297 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
Thanks for all your uplift landings Ryan.
Flags: needinfo?(cpearce)
No longer blocks: 1129455
Depends on: 1159456
You need to log in before you can comment on or make changes to this bug.