Correct the logic of resuming from AudioChannel.

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 |.
(Assignee)

Updated

2 years ago
Assignee: nobody → kaku
Blocks: 1244768, 1301426
Depends on: 1309162
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8816062 [details]
Bug 1321497 - correct the logic of resuming from AudioChannel;

https://reviewboard.mozilla.org/r/96854/#review97056
Attachment #8816062 - Flags: review?(jwwang) → review+

Comment 4

2 years ago
mozreview-review
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
}
Attachment #8816062 - Flags: review?(alwu) → review+
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1c6c9a6c9b16
correct the logic of resuming from AudioChannel; r=alwu,jwwang
Keywords: checkin-needed
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
Flags: needinfo?(kaku)
(Assignee)

Comment 10

2 years ago
@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?
Flags: needinfo?(kaku) → needinfo?(alwu)

Updated

2 years ago
Blocks: 1322384

Updated

2 years ago
Flags: needinfo?(alwu)
(Assignee)

Comment 11

2 years ago
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 hidden (mozreview-request)

Comment 13

2 years ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/34a2a7ffa7b3
correct the logic of resuming from AudioChannel; r=alwu,jwwang

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34a2a7ffa7b3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.