Clean up the suspend-video-decoder call path

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Depends on 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(9 attachments, 2 obsolete attachments)

59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
(Assignee)

Description

2 years ago
Refactor to make the MediaDecoder focuses on the policy and the MDSM focuses on the mechanism.
(Assignee)

Updated

2 years ago
Assignee: nobody → kaku
Blocks: 1346116
Status: NEW → ASSIGNED
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)

Comment 10

2 years ago
compile error......., will upload again.
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
Attachment #8846251 - Flags: review?(jwwang)
Attachment #8846252 - Flags: review?(jwwang)
Attachment #8846253 - Flags: review?(jwwang)
Attachment #8846254 - Flags: review?(jwwang)
Attachment #8846255 - Flags: review?(jwwang)
Attachment #8846256 - Flags: review?(jwwang)
Attachment #8846257 - Flags: review?(jwwang)
Attachment #8846258 - Flags: review?(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8846252 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8846274 [details]
Bug 1346498 part 9 - move all policy codes into MediaDecoder::UpdateVideoDecodeMode();

https://reviewboard.mozilla.org/r/119380/#review121312

::: dom/media/MediaDecoder.cpp:399
(Diff revision 1)
>    , mPinnedForSeek(false)
>    , mMinimizePreroll(false)
>    , mMediaTracksConstructed(false)
>    , mFiredMetadataLoaded(false)
> -  , mElementVisible(!aOwner->IsHidden())
> +  , mIsDocumentVisible(!aOwner->IsHidden())
> +  , mIsElementVisible(!aOwner->IsHidden())

Initialize mIsElementVisible to "!aOwner->IsHidden()" is not right, however, it doesn't matter because the mIsElementVisible is not read until the MediaDecoder::NotifyOwnerActivityChanged() is call and MediaDecoder::NotifyOwnerActivityChanged() assigns the right value to mIsElementVisible.

This issue will be fixed at bug 1346116 patch 1.
https://reviewboard.mozilla.org/r/119452/diff/1#index_header

Comment 30

2 years ago
mozreview-review
Comment on attachment 8846256 [details]
Bug 1346498 part 5 - don't check mHasSuspendTaint in HandleVideoSuspendTimeout();

https://reviewboard.mozilla.org/r/119368/#review121342
Attachment #8846256 - Flags: review?(jwwang) → review+

Comment 31

2 years ago
mozreview-review
Comment on attachment 8846253 [details]
Bug 1346498 part 2 - implement the VideoDecodeMode mechanism in MDSM;

https://reviewboard.mozilla.org/r/119362/#review121340

::: dom/media/MediaDecoder.h:820
(Diff revision 3)
>  
>    // True if the decoder has a suspend taint - meaning suspend-video-decoder is
>    // disabled.
>    Canonical<bool> mHasSuspendTaint;
>  
> +  Canonical<VideoDecodeMode> mVideoDecodeMode;

I can't see the point to use Canonical/Mirror here for MediaDecoder never reads mVideoDecodeMode. You can just add a setter to MDSM.
Attachment #8846253 - Flags: review?(jwwang) → review-

Comment 32

2 years ago
mozreview-review
Comment on attachment 8846251 [details]
Bug 1346498 part 1 - extract the MediaDecoder::NotifyCompositor() method;

https://reviewboard.mozilla.org/r/119358/#review121344
Attachment #8846251 - 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)
(Assignee)

Updated

2 years ago
Attachment #8846258 - Attachment is obsolete: true
Attachment #8846258 - Flags: review?(jwwang)

Comment 41

2 years ago
mozreview-review
Comment on attachment 8846253 [details]
Bug 1346498 part 2 - implement the VideoDecodeMode mechanism in MDSM;

https://reviewboard.mozilla.org/r/119362/#review121386

::: dom/media/MediaDecoderStateMachine.cpp:3078
(Diff revision 4)
> +{
> +  nsCOMPtr<nsIRunnable> r =
> +    NewRunnableMethod<VideoDecodeMode>(this,
> +                                       &MediaDecoderStateMachine::SetVideoDecodeModeInternal,
> +                                       aMode);
> +  OwnerThread()->Dispatch(r.forget());

Call DispatchStateChange() for this is indeed a state change.
Attachment #8846253 - Flags: review?(jwwang) → review+

Comment 42

2 years ago
mozreview-review
Comment on attachment 8846254 [details]
Bug 1346498 part 3 - implement the UpdateVideoDecodeMode() policy in MediaDecoder;

https://reviewboard.mozilla.org/r/119364/#review121388
Attachment #8846254 - Flags: review?(jwwang) → review+

Comment 43

2 years ago
mozreview-review
Comment on attachment 8846255 [details]
Bug 1346498 part 4 - remove mIsVisible cannonical-mirror pair;

https://reviewboard.mozilla.org/r/119366/#review121392
Attachment #8846255 - Flags: review?(jwwang) → review+

Comment 44

2 years ago
mozreview-review
Comment on attachment 8846257 [details]
Bug 1346498 part 6 - remove mHasSuspendTaint cannonical-mirror pair;

https://reviewboard.mozilla.org/r/119370/#review121394
Attachment #8846257 - Flags: review?(jwwang) → review+

Comment 45

2 years ago
mozreview-review
Comment on attachment 8846272 [details]
Bug 1346498 part 7 - remove outdated comments in HTMLMediaElement.cpp;

https://reviewboard.mozilla.org/r/119376/#review121396
Attachment #8846272 - Flags: review?(jwwang) → review+

Comment 46

2 years ago
mozreview-review
Comment on attachment 8846273 [details]
Bug 1346498 part 8 - extract the HTMLMediaElement::NotifyDecoderActivityChanges() method;

https://reviewboard.mozilla.org/r/119378/#review121400

::: dom/html/HTMLMediaElement.h:1303
(Diff revision 2)
>    void NotifyAboutPlaying();
>  
>    already_AddRefed<Promise> CreateDOMPromise(ErrorResult& aRv) const;
>  
> +  // Pass information for deciding the video decode mode to decoder.
> +  void SetActiviyChangesToDecoder() const;

Might be good to call it "NotifyDecoderActivityChanges".
Attachment #8846273 - Flags: review?(jwwang) → review+

Comment 47

2 years ago
mozreview-review
Comment on attachment 8846274 [details]
Bug 1346498 part 9 - move all policy codes into MediaDecoder::UpdateVideoDecodeMode();

https://reviewboard.mozilla.org/r/119380/#review121404

::: dom/media/MediaDecoder.cpp:1339
(Diff revision 2)
>  
>  void
>  MediaDecoder::UpdateVideoDecodeMode()
>  {
>    // The MDSM may yet be set.
>    if (!mDecoderStateMachine) {

If this is called before mDecoderStateMachine is set, we need to propagate the changes to MDSM once it is created.
Attachment #8846274 - Flags: review?(jwwang) → review+
(Assignee)

Comment 48

2 years ago
mozreview-review-reply
Comment on attachment 8846274 [details]
Bug 1346498 part 9 - move all policy codes into MediaDecoder::UpdateVideoDecodeMode();

https://reviewboard.mozilla.org/r/119380/#review121404

> If this is called before mDecoderStateMachine is set, we need to propagate the changes to MDSM once it is created.

Will then call MediaDecoder::UpdateVideoDecodeMode() here: http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/dom/media/MediaDecoder.cpp#1480
(Assignee)

Comment 49

2 years ago
mozreview-review-reply
Comment on attachment 8846274 [details]
Bug 1346498 part 9 - move all policy codes into MediaDecoder::UpdateVideoDecodeMode();

https://reviewboard.mozilla.org/r/119380/#review121404

> Will then call MediaDecoder::UpdateVideoDecodeMode() here: http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/dom/media/MediaDecoder.cpp#1480

This issue belongs to patch-3.
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)
(Assignee)

Comment 60

2 years ago
Try looks good, thanks for review!
Keywords: checkin-needed

Comment 61

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1152ca4a710e
part 1 - extract the MediaDecoder::NotifyCompositor() method; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/caaa45f70bb7
part 2 - implement the VideoDecodeMode mechanism in MDSM; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/af2fd441e1b2
part 3 - implement the UpdateVideoDecodeMode() policy in MediaDecoder; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a42487bf567a
part 4 - remove mIsVisible cannonical-mirror pair; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/6cc263c1f298
part 5 - don't check mHasSuspendTaint in HandleVideoSuspendTimeout(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f4fc110ccde1
part 6 - remove mHasSuspendTaint cannonical-mirror pair; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/d450a061b0be
part 7 - remove outdated comments in HTMLMediaElement.cpp; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/2fbdf43912c4
part 8 - extract the HTMLMediaElement::NotifyDecoderActivityChanges() method; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/0f25d813f75d
part 9 - move all policy codes into MediaDecoder::UpdateVideoDecodeMode(); r=jwwang
Keywords: checkin-needed
Depends on: 922951
You need to log in before you can comment on or make changes to this bug.