1125472 - When entering to dormant, mVideoRequestStatus is not cleared

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Sotaro Ikeda [:sotaro]]

When entering to dormant, mVideoRequestStatus is not cleared. It causes problem to video decoding after dormant exit.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Clear mVideoRequestStatus and mAudioRequestStatus

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8554105 [details] [diff] [review]
patch - Clear mVideoRequestStatus and mAudioRequestStatus

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2763,5 @@
>        mPendingWakeDecoder = nullptr;
>        {
>          ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>          // Wait for the thread decoding, if any, to exit.
>          DecodeTaskQueue()->AwaitIdle();

Doesn't DecodeTaskQueue()->AwaitIdle() guarantee all decoding tasks will be finished and m{Audio,Video}RequestStatus will be reset to idle?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Help increase chances of reproing failure

This patch makes it more likely to reproduce the stall.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think resetting the request status is treating the symptom, not the root cause.

I think what is happening, is that MediaDecoderStateMachine::EnsureAudioDecodeTaskQueued() (or EnsureVideoDecodeTaskQueued()) sets the video/audio request status to pending, and then dispatches a task to call DecodeAudio or DecodeVideo, and before we run that task, we switch to dormant mode, and execute RunStateMachine in the DORMANT case and call FlushDecoding(), which does a FlushAndDispatch() on the decode task queue, which drops the task to run DecodeVideo(), and so we never get out of pending state for the audio or video decode, and so playback can't continue after the seek. DecodeVideo() has a case that resets the request status if the state machine's state has changed.

I think the decode task that's being flushed is a decode task dispatched to preroll MediaData dispatched after the seek completes but before we've had a chance to switch to dormant state.

So the problem is that in FlushDecoding() we drop the task that would reset the request status field. 

I think what we need to do instead of this patch is change MediaDecoderStateMachine::FlushDecoding().

In FlushDecoding(), change:

    DecodeTaskQueue()->FlushAndDispatch(task);

to 
    DecodeTaskQueue()->Dispatch(task);
    DecodeTaskQueue()->AwaitIdle();

This means we won't drop the task that ensures we reset the decode status. It does however mean that the decoder may end up running more tasks that it did previously. This is probably OK, but it means our shutdown and seeking may not be quite so fast; but it should also mean that the Readers never have their tasks spontaneously disappear, which may in fact improve their reliability.

Bobby: Does changing the FlushAndDispatch to a Dispatch();AwaitIdle(); in MDSM:FlushDecoding() sound reasonable to you?

We should (in another bug!) perhaps remove MediaTaskQueue::FlushAndDispatch() for safety's sake; there's only one other callsite in the WMFMediaDataDecoder.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8554105 [details] [diff] [review]
patch - Clear mVideoRequestStatus and mAudioRequestStatus

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

I think we should not use MediaTaskQueue::FlushAndDispatch(). We should Dispatch() and then AwaitIdle() in FlushDecoding() instead.

Ideally we'd make ResetDecode() return a promise. But that's a more complicated patch that we should do at a later time, in another bug.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2763,5 @@
>        mPendingWakeDecoder = nullptr;
>        {
>          ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>          // Wait for the thread decoding, if any, to exit.
>          DecodeTaskQueue()->AwaitIdle();

jwwang: There's the FlushDecoding() call above that flushes the decode task queue which prevents the task that would reset the request status from running.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8554105 [details] [diff] [review]
patch - Clear mVideoRequestStatus and mAudioRequestStatus

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2763,5 @@
>        mPendingWakeDecoder = nullptr;
>        {
>          ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>          // Wait for the thread decoding, if any, to exit.
>          DecodeTaskQueue()->AwaitIdle();

Does this code predate the MediaPromise? Bug 1123983 indicates media promises will always be resolved or rejected during FlushDecoding().

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes, though it was modified recently in bug 1123983.

But also note that MediaPromises' tasks to dispatch reject/resolves are immune to flushing. That is, a MediaTaskQueue::Flush() won't flush a MediaPromise's resolve/reject tasks, they still run. However in this case the task that's responsible for *calling* RequestVideoData (a DecodeVideo() function call task) is a normal task and it *is* being Flushed. Because that task is flushed, it doesn't reset the request status fields, and they remain in pending state.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Chris Pearce (:cpearce) from comment #4)
> Bobby: Does changing the FlushAndDispatch to a Dispatch();AwaitIdle(); in
> MDSM:FlushDecoding() sound reasonable to you?

Absolutely. The whole "throw away tasks" thing always creeped me out, which is why I engineered promises to override that behavior. Getting rid of it entirely (in favor of more explicitly cancelable things in places where it's needed) seems much better.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

OK, let's see if anything breaks then:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efa4b41d6a3c

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> I think what we need to do instead of this patch is change
> MediaDecoderStateMachine::FlushDecoding().
> 
> In FlushDecoding(), change:
> 
>     DecodeTaskQueue()->FlushAndDispatch(task);
> 
> to 
>     DecodeTaskQueue()->Dispatch(task);
>     DecodeTaskQueue()->AwaitIdle();

cpearce, your change worked for me. Can you take this bug?

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes, I'll take this bug.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

Don't flush the decode task queue in MDSM::FlushDecoding().

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8554717 [details] [diff] [review]
Patch

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

r=me with a followup filed to get rid of the flushing machinery in MediaTaskQueue.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

I thought you might say that! Bug 1125970.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fde8a287433b

Comment 16

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=043a03d002f2 seems one of this changes caused a Crashtest bustage on Windows Tests like:

https://treeherder.mozilla.org/logviewer.html#?job_id=5955531&repo=mozilla-inbound

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/525b62ea9132

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out because something in the push was causing Linux32 opt mochitest-3 timeouts and Windows w-p-t failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3df250d0364b

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5997815&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5999248&repo=mozilla-inbound

Comment 19

[Sotaro Ikeda [:sotaro]]

It might be better to check-in this bug's patch separately from Bug 1123535.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=221b9517b391

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> It might be better to check-in this bug's patch separately from Bug 1123535.

Yes. I will land if the try push is green. I will try to debug the failures tomorrow.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Try in comment 20 is green for this change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d975577991ca

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ralph: will need uplift here to support dormant mode for memory reduction strategy.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Looks like this was the one responsible for the mochitest failures noted in comment 18. Backed out again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed122041670

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

I don't see any failures on m-i, and there weren't any in the try run. Can you link them here?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6033720&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6034728&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6040711&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6041592&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6037633&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6038255&repo=mozilla-inbound

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

With Sotaro's original patch, this is green, except for a web-platform test that is already known to be bad gets worse:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c412adf8db4d

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

I think what the problem with the Linux boxes, is that they still use the synchronous decode adapter. In MediaDecoderReader::RequestVideoData if the DecodeVideoFrame() call returns false (say because we shutdown the MediaResource) and we happen to have been skipping to keyframe, we'll dispatch a task to call RequestVideoData again. Since we're no longer flushing the decode task queue, this means we could never have an idle task queue since when the task is running it will always fail in the DecodeVideoFrame call and then dispatch another task to try again. So we'd time out.

This patch is based on top of my previous patch.

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=373c56dfa7c4

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8556248 [details] [diff] [review]
Patch

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

::: dom/media/MediaDecoderReader.cpp
@@ +189,5 @@
>      mReader->RequestVideoData(skip, mTimeThreshold);
>      return NS_OK;
>    }
> +
> +  void Cancel() {

Can we assert that this is only every called from the task queue? The MOZ_ASSERT above could fail if it isn't.

@@ +276,5 @@
>  }
>  
> +nsresult MediaDecoderReader::ResetDecode()
> +{
> +  nsresult res = NS_OK;

I know you just moved this, but why is this a local var? We can just return NS_OK.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yup, will assert in RequestVideoWithSkipTask::Cancel() (and Run() too) that we're on the task queue, and remove the local. Thanks!

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

I am not convinced that try push is green enough, so I push Sotaro's patch instead and follow up.

Sotaro's patch Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9888dba8f3bc

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a607fdc02c5d

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a607fdc02c5d

Comment 35

[Ralph Giles (:rillian)]

Attachment: Reset video request status on reset decode.

Sotaro's patch as landed. Requesting uplift of just this change.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Youtube playback uses more resources.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Low. This does affect non-MSE playback but is a small and isolated change.
[String/UUID change made/needed]: None.

Comment 36

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2822ac22aae1

Comment 37

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e118e867ccf