1321497 - Correct the logic of resuming from AudioChannel.

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

This should be a regression from bug 1309162, although it does not fail any test case... except the one that is going to be added in bug 1244768 and bug 1301426.

The logic here http://searchfox.org/mozilla-central/rev/ef3cede9a5b050fffade9055ca274654f2bc719e/dom/html/HTMLMediaElement.cpp#780 was moved from http://searchfox.org/mozilla-central/rev/13a45548c9a1eeb292ce54113a7cdf7263772ce3/dom/html/HTMLMediaElement.cpp#6039 with one more check | mOwner->Paused |

However, it should be | NOT mOwner->Paused |.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1321497 - correct the logic of resuming from AudioChannel;

Review commit: https://reviewboard.mozilla.org/r/96854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96854/

Comment 2

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=16776c254887f72b153ca6df4786cf98f77e17d3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddff1e10c6b1c9568e9fc530e217343095c94038

Comment 3

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

Comment on attachment 8816062 [details]
Bug 1321497 - correct the logic of resuming from AudioChannel;

https://reviewboard.mozilla.org/r/96854/#review97056

Comment 4

[Alastor Wu [:alwu]]

Comment on attachment 8816062 [details]
Bug 1321497 - correct the logic of resuming from AudioChannel;

https://reviewboard.mozilla.org/r/96854/#review97106

::: dom/html/HTMLMediaElement.cpp:780
(Diff revision 1)
>    }
>  
>    void
>    Resume()
>    {
> -    if (!IsSuspended() && mOwner->Paused()) {
> +    if (!IsSuspended() && !mOwner->Paused()) {

Sorry for the error in bug 1309162.

I don't want to resume mOwner if the it doesn't be paused, so here we should modify the codition to the following. 

if (!IsSuspended() || !mOwner->Paused()) {
  // no need to be resumed
}

Comment 5

[Tzuhao Kuo [:kaku]]

Comment on attachment 8816062 [details]
Bug 1321497 - correct the logic of resuming from AudioChannel;

https://reviewboard.mozilla.org/r/96854/#review97106

> Sorry for the error in bug 1309162.
> 
> I don't want to resume mOwner if the it doesn't be paused, so here we should modify the codition to the following. 
> 
> if (!IsSuspended() || !mOwner->Paused()) {
>   // no need to be resumed
> }

Sure.

Comment 6

[Tzuhao Kuo [:kaku]]

Comment on attachment 8816062 [details]
Bug 1321497 - correct the logic of resuming from AudioChannel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96854/diff/1-2/

Comment 7

[Tzuhao Kuo [:kaku]]

Try looks pretty good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21108075de598f088c39bffdac7c9ca26c9cafce
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39e36925f8a7257d90e54c78e4d3fc06bb065ed0

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c6c9a6c9b16
correct the logic of resuming from AudioChannel; r=alwu,jwwang

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_mediaPlayback_suspended_multipleAudio.js:

https://hg.mozilla.org/integration/autoland/rev/b0b97ce213be8cf067558146079756b0bb610370

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1c6c9a6c9b16991dc4605aef989f1ae0c8ff8820
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7575017&repo=autoland

[task 2016-12-01T16:07:06.284531Z] 16:07:06     INFO - TEST-PASS | toolkit/content/tests/browser/browser_mediaPlayback_suspended_multipleAudio.js | The blocked audio can't be playback. - true == true - 
[task 2016-12-01T16:07:06.286217Z] 16:07:06     INFO - - both audio should be resumed at the same time -
[task 2016-12-01T16:07:06.287833Z] 16:07:06     INFO - Buffered messages finished
[task 2016-12-01T16:07:06.290879Z] 16:07:06     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/browser/browser_mediaPlayback_suspended_multipleAudio.js | The suspeded state of autoplay audio is correct. - 2 == 0 - 
[task 2016-12-01T16:07:06.293401Z] 16:07:06     INFO - Stack trace:
[task 2016-12-01T16:07:06.295142Z] 16:07:06     INFO - chrome://mochikit/content/tests/BrowserTestUtils/content-task.js line 52 > eval:check_all_audio_suspended:10
[task 2016-12-01T16:07:06.296977Z] 16:07:06     INFO - chrome://mochikit/content/tests/BrowserTestUtils/content-task.js:null:53

Comment 10

[Tzuhao Kuo [:kaku]]

@Alastor, 

Looks like the logic in comment4 breaks the test case here http://searchfox.org/mozilla-central/rev/dc8cf05768b83a6ef0b4039edd6efddd56ee4109/toolkit/content/tests/browser/browser_mediaPlayback_suspended_multipleAudio.js#24

Sorry that I have few knolodwage about the AudioChannel API, do you have any idea about this?

Comment 11

[Tzuhao Kuo [:kaku]]

Per discussion offline, we're going to rollback the logic of HTMLMediaElement::AudioChannelAgentCallback::Resume() (http://searchfox.org/mozilla-central/rev/a8b5f53e7df90df655a0982e94087ee83290c22e/dom/html/HTMLMediaElement.cpp#795) to where it was from, HTMLMediaElement::ResumeFromAudioChannel() (http://searchfox.org/mozilla-central/rev/13a45548c9a1eeb292ce54113a7cdf7263772ce3/dom/html/HTMLMediaElement.cpp#6037). The change was introduced in bug 1309162.

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8816062 [details]
Bug 1321497 - correct the logic of resuming from AudioChannel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96854/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34a2a7ffa7b3
correct the logic of resuming from AudioChannel; r=alwu,jwwang

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34a2a7ffa7b3