Closed Bug 1309162 Opened 8 years ago Closed 8 years ago

Create an audio channel wrapper object for media element

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
Other Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(7 files)

Bug 1309162 - part1 : create a separate class to handle audio channel releated stuffs.
58 bytes, text/x-review-board-request
baku
: review+
jwwang
: review+
Details
Bug 1309162 - part2 : remove audio channel code from media element.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1309162 - part3 : modify the place calling UpdateAudioChannelPlayingState().
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1309162 - part4 : remove checking for mPlayingBeforeSeek.
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1309162 - part5 : only set the audible state when stream starts playing.
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1309162 - part6 : remove useless comment.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1309162 - part7 : wrap custom policy function.
58 bytes, text/x-review-board-request
jwwang
: review+
Details 
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.
I'll start to work on this bug after finishing the bug1302350.
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 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 on attachment 8810694 [details]
Bug 1309162 - part6 : remove useless comment.

https://reviewboard.mozilla.org/r/92974/#review92992
Attachment #8810694 - Flags: review?(jwwang) → 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 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 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 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+
(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 :)
Blocks: 1320005
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+
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
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
Flags: needinfo?(alwu)
Blocks: 1321196
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)
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)
Depends on: 1338337
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: