1120247 - MediaCodecReader::ResetDecode() needs to reject(cancel) any pending promises

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

I'm pretty sure MediaCodecReader::ResetDecode() should reject(cancel) any pending promises when it runs.

I suspect we need to set some sort of threadsafe flag to prevent any running decode tasks from returning output, and then flush decoding, then reject the promises with CANCELED.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: bug-1120247.v01.patch

1. Return cancel by promise in ResetDecode().
2. In addition, I change the AwaitIdle() to Flush() again because at bug 1114910 comment 7, "Remove self dispatch code" and "Flush() -> AwaiteIdle()" should be done together. But at last, I kept the code of dispatch to self, forgot to change back the Flush() function.

Comment 2

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

Comment on attachment 8550244 [details] [diff] [review]
bug-1120247.v01.patch

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

Thanks!

::: dom/media/omx/MediaCodecReader.cpp
@@ +759,5 @@
>  {
>    if (CheckAudioResources()) {
> +    mAudioTrack.mTaskQueue->Flush();
> +    MonitorAutoLock al(mAudioTrack.mTrackMonitor);
> +    if(!mAudioTrack.mAudioPromise.IsEmpty()) {

Space between "if" and "(", i.e.:

if (!mAudioTrack.mAudioPromise.IsEmpty())

Not:

if(!mAudioTrack.mAudioPromise.IsEmpty())

@@ +768,5 @@
>    }
>    if (CheckVideoResources()) {
> +    mVideoTrack.mTaskQueue->Flush();
> +    MonitorAutoLock al(mVideoTrack.mTrackMonitor);
> +    if(!mVideoTrack.mVideoPromise.IsEmpty()) {

Space between "if" and "(".

Comment 3

[Benjamin Chen [:bechen]]

Attachment: bug-1120247.v01.patch

r=cpearce
try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c19dae66fd51

Comment 4

[Carsten Book [:Tomcat]]

bechen, do you know if the  debug static analysis build failure related to this change from the try run ?

Comment 5

[Benjamin Chen [:bechen]]

(In reply to Carsten Book [:Tomcat] from comment #4)
> bechen, do you know if the  debug static analysis build failure related to
> this change from the try run ?

From 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c19dae66fd51&filter-searchStr=OS%20X%2010.7%2064-bit%20try%20debug%20static%20analysis%20build

I don't think it is related to my patch because my patch only works on b2g platform.
But at bug 1091467 comment 12, https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c358be71032, 
It shows the same error.

Do you know where I can find out more details like build logs, static analysis logs?
Because for now I can only see the logs from comment 3. That doesn't tell me where is the failure of static analysis.

Comment 6

[Benjamin Chen [:bechen]]

Push to try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33c95d115f87

Comment 7

[Benjamin Chen [:bechen]]

Raise checkin-needed again based on comment 5, comment 6.

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Carsten Book [:Tomcat]]

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