Correct the logic of resuming from AudioChannel.

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year 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

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

Comment 3

a year 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

a year 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

a year 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

a year ago
Keywords: checkin-needed

Comment 8

a year 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

a year 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)
Flags: needinfo?(alwu)
(Assignee)

Comment 11

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34a2a7ffa7b3
Status: NEW → RESOLVED
Last Resolved: a year 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.