Create an audio channel wrapper object for media element

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

Other Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(7 attachments)

(Assignee)

Description

2 years ago
Fork form bug1301055 comment20, we want to move all audio channel related code into another new wrapper object, and that can reduce the complexity of media element's code.

The media element should only handle spec-related code, and any our custom behaviors codes should be in other wrapper classes.
(Assignee)

Comment 1

2 years ago
I'll start to work on this bug after finishing the bug1302350.
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8809332 [details]
Bug 1309162 - part1 : create a separate class to handle audio channel releated stuffs.

https://reviewboard.mozilla.org/r/91894/#review92988

r+ except for the CC parts.

::: dom/html/HTMLMediaElement.h:760
(Diff revision 6)
>    void MarkAsContentSource(CallerAPI aAPI);
>  
>  protected:
>    virtual ~HTMLMediaElement();
>  
> +  class AudioChannelWrapper;

Call this AudioChannelAgentCallback.

::: dom/html/HTMLMediaElement.h:1300
(Diff revision 6)
>  
>    // The current decoder. Load() has been called on this decoder.
>    // At most one of mDecoder and mSrcStream can be non-null.
>    RefPtr<MediaDecoder> mDecoder;
>  
> +  uint32_t GetMuted() const {

You don't need this function because AudioChannelWrapper can access private members of HTMLMediaElement.

::: dom/html/HTMLMediaElement.cpp:557
(Diff revision 6)
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(AudioChannelWrapper)
> +
> +  explicit AudioChannelWrapper(HTMLMediaElement* aOwner, AudioChannel aChannel)

'explicit' is needed for one-parameter constructor only.

::: dom/html/HTMLMediaElement.cpp:558
(Diff revision 6)
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(AudioChannelWrapper)
> +
> +  explicit AudioChannelWrapper(HTMLMediaElement* aOwner, AudioChannel aChannel)
> +    : mOwner(aOwner),

Coding style:

MyClass(int aVar, int aVar2)
  : mVar(aVar)
  , mVar2(aVar2)
{
     ...
}

::: dom/html/HTMLMediaElement.cpp:1164
(Diff revision 6)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSrcStream)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSrcAttrStream)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSourcePointer)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLoadBlockedDoc)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSourceLoadCandidate)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAudioChannelAgent)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAudioChannelWrapper)

Does it really need to participate CC? If not, mAudioChannelWrapper is guaranteed to be non-null for the lifetime of HTMLMediaElement and you can save some null checks.
Attachment #8809332 - Flags: review?(jwwang) → review+

Comment 25

2 years ago
mozreview-review
Comment on attachment 8809333 [details]
Bug 1309162 - part2 : remove audio channel code from media element.

https://reviewboard.mozilla.org/r/91896/#review92990
Attachment #8809333 - Flags: review?(jwwang) → review+

Comment 26

2 years ago
mozreview-review
Comment on attachment 8810694 [details]
Bug 1309162 - part6 : remove useless comment.

https://reviewboard.mozilla.org/r/92974/#review92992
Attachment #8810694 - Flags: review?(jwwang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

2 years ago
mozreview-review
Comment on attachment 8809332 [details]
Bug 1309162 - part1 : create a separate class to handle audio channel releated stuffs.

https://reviewboard.mozilla.org/r/91894/#review95712

::: dom/html/HTMLMediaElement.cpp:595
(Diff revision 8)
> +    // but resumed it from page.
> +    SetSuspended(nsISuspendedTypes::NONE_SUSPENDED);
> +    UpdateAudioChannelPlayingState();
> +  }
> +
> +  NS_IMETHODIMP WindowVolumeChanged(float aVolume, bool aMuted) override

use the syntax:

returnType
MethodName(...)
{
 ...
Attachment #8809332 - Flags: review?(amarchesini) → review+

Comment 40

2 years ago
mozreview-review
Comment on attachment 8810328 [details]
Bug 1309162 - part3 : modify the place calling UpdateAudioChannelPlayingState().

https://reviewboard.mozilla.org/r/92680/#review95714
Attachment #8810328 - Flags: review?(amarchesini) → review+

Comment 41

2 years ago
mozreview-review
Comment on attachment 8810329 [details]
Bug 1309162 - part4 : remove checking for mPlayingBeforeSeek.

https://reviewboard.mozilla.org/r/92682/#review95716

r+, I hope we have enough tests for this :)
Attachment #8810329 - Flags: review?(amarchesini) → review+

Comment 42

2 years ago
mozreview-review
Comment on attachment 8810330 [details]
Bug 1309162 - part5 : only set the audible state when stream starts playing.

https://reviewboard.mozilla.org/r/92684/#review95718
Attachment #8810330 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 43

2 years ago
(In reply to Andrea Marchesini [:baku] from comment #41)
> r+, I hope we have enough tests for this :)

Yes, we have a test to check that (browserElement_AudioChannelSeeking.js).
In addition, now hiding tab audio indicator will be delayed for 3 seconds (bug1232357), so we don't need to worry about the indicator blinking during seeking :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1320005

Comment 51

2 years ago
mozreview-review
Comment on attachment 8814765 [details]
Bug 1309162 - part7 : wrap custom policy function.

https://reviewboard.mozilla.org/r/95858/#review95882
Attachment #8814765 - Flags: review?(jwwang) → review+

Comment 52

2 years ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b09c6adcfa5
part1 : create a separate class to handle audio channel releated stuffs. r=baku,jwwang
https://hg.mozilla.org/integration/autoland/rev/16ceabbf8eef
part2 : remove audio channel code from media element. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/b5ab5eae225d
part3 : modify the place calling UpdateAudioChannelPlayingState(). r=baku
https://hg.mozilla.org/integration/autoland/rev/fe1771c91309
part4 : remove checking for mPlayingBeforeSeek. r=baku
https://hg.mozilla.org/integration/autoland/rev/2015de6577af
part5 : only set the audible state when stream starts playing. r=baku
https://hg.mozilla.org/integration/autoland/rev/17c744162fbe
part6 : remove useless comment. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/3f0ad2d2c594
part7 : wrap custom policy function. r=jwwang
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 62

2 years ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17d4303e4f86
part1 : create a separate class to handle audio channel releated stuffs. r=baku,jwwang
https://hg.mozilla.org/integration/autoland/rev/bd25005744ed
part2 : remove audio channel code from media element. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/37f5c9c13c2f
part3 : modify the place calling UpdateAudioChannelPlayingState(). r=baku
https://hg.mozilla.org/integration/autoland/rev/67431dae9ec8
part4 : remove checking for mPlayingBeforeSeek. r=baku
https://hg.mozilla.org/integration/autoland/rev/583e3edec539
part5 : only set the audible state when stream starts playing. r=baku
https://hg.mozilla.org/integration/autoland/rev/eef982c27c7b
part6 : remove useless comment. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/716be76c9a2f
part7 : wrap custom policy function. r=jwwang
(Assignee)

Updated

2 years ago
Flags: needinfo?(alwu)

Updated

2 years ago
Blocks: 1321196

Updated

2 years ago
Blocks: 1321497
Alastor, I'm trying to understand the AutoNoJSAPI and its comments about IsCallerChrome in NotifyAudioChannelAgent.  There are no IsCallerChrome checks in audiochannel.  What is this AutoNoJSAPI trying to prevent, exactly?
Flags: needinfo?(alwu)
(Assignee)

Comment 65

2 years ago
That is previous modification, transfer NI to baku.
Flags: needinfo?(alwu) → needinfo?(amarchesini)
> checks in audiochannel.  What is this AutoNoJSAPI trying to prevent, exactly?

We wanted to be sure that any AudioChannelAgent method was exposed only to chrome code. But at some point, we found out that there was a content operation able to trigger an AudioChannelAgent method (not directly, of course). Instead removing the assertion, I preferred to add AutoNoJSAPI. I don't remember the detail, but in case, I can remove AudioNoJSAPI an push to try to see if this assertion is triggered by some tests.
Flags: needinfo?(amarchesini)
Please do.  I'm not aware of what assertion you're talking about.
Flags: needinfo?(amarchesini)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8a0f9acd005502b23c50fab333d3ea5bd3caee2&selectedJob=75942019

Green on treeherder. I think we can remove that AutoNoJSAPI.
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1338337
You need to log in before you can comment on or make changes to this bug.