Closed Bug 1308071 Opened 7 years ago Closed 7 years ago

Change the type of MDSM::mInfo to Maybe<MediaInfo>

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla52

Tracking Flags:

Tracking

Status

firefox52

---

fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files)

Bug 1308071. Part 1 - Change the type of MDSM::mInfo to Maybe<MediaInfo>. 7 years ago JW Wang [:jwwang] [:jw_wang] 58 bytes, text/x-review-board-request	kaku : review+	Details
Bug 1308071. Part 2 - remove unused members. 7 years ago JW Wang [:jwwang] [:jw_wang] 58 bytes, text/x-review-board-request	kaku : review+	Details
Bug 1308071. Part 3 - early return from VisibilityChanged() if mInfo is nothing. 7 years ago JW Wang [:jwwang] [:jw_wang] 58 bytes, text/x-review-board-request	kaku : review+	Details

JW Wang [:jwwang] [:jw_wang]

Assignee

Description

•

7 years ago

This helps catch invalid usage of mInfo before reading metadata.

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

7 years ago

Assignee: nobody → jwwang

Blocks: 1295892

Priority: -- → P3

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/84796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84796/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/84798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84798/

Comment hidden (mozreview-request)

VisibilityChanged() could be called before reading metadata.

Review commit: https://reviewboard.mozilla.org/r/84800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84800/

JW Wang [:jwwang] [:jw_wang]

Assignee

Updated

•

7 years ago

Attachment #8799662 - Flags: review?(kaku)

Attachment #8799663 - Flags: review?(kaku)

Attachment #8799664 - Flags: review?(kaku)

Tzuhao Kuo [:kaku]

Comment 4

•

7 years ago

mozreview-review

Comment on attachment 8799662 [details]
Bug 1308071. Part 1 - Change the type of MDSM::mInfo to Maybe<MediaInfo>.

https://reviewboard.mozilla.org/r/84796/#review83378

::: dom/media/MediaDecoderStateMachine.h:310
(Diff revision 1)
>  
>    MediaStatistics GetStatistics();
>  
> -  // This is called on the state machine thread and audio thread.
> -  // The decoder monitor must be obtained before calling this.
> -  bool HasAudio() const {
> +  bool HasAudio() const { return mInfo.ref().HasAudio(); }
> +  bool HasVideo() const { return mInfo.ref().HasVideo(); }
> +  MediaInfo& Info() { return mInfo.ref(); }

Should be constified.

::: dom/media/MediaDecoderStateMachine.cpp:315
(Diff revision 1)
> +    MediaInfo& info = Info();
>  
> -    if (mMaster->mInfo.mMetadataDuration.isSome()) {
> +    if (info.mMetadataDuration.isSome()) {
>        mMaster->RecomputeDuration();
> -    } else if (mMaster->mInfo.mUnadjustedMetadataEndTime.isSome()) {
> +    } else if (info.mUnadjustedMetadataEndTime.isSome()) {
>        RefPtr<Master> master = mMaster;
>        Reader()->AwaitStartTime()->Then(OwnerThread(), __func__,
> -        [master] () {
> +        [master, &info] () {
>            NS_ENSURE_TRUE_VOID(!master->IsShutdown());
> -          TimeUnit unadjusted = master->mInfo.mUnadjustedMetadataEndTime.ref();
> +          TimeUnit unadjusted = info.mUnadjustedMetadataEndTime.ref();
>            TimeUnit adjustment = master->mReader->StartTime();
> -          master->mInfo.mMetadataDuration.emplace(unadjusted - adjustment);
> +          info.mMetadataDuration.emplace(unadjusted - adjustment);

Not really a problem, but since you use StateObject::Info() everywhere, why do you use a temporary reference here?

Attachment #8799662 - Flags: review?(kaku) → review+

Tzuhao Kuo [:kaku]

Comment 5

•

7 years ago

mozreview-review

Comment on attachment 8799664 [details]
Bug 1308071. Part 3 - early return from VisibilityChanged() if mInfo is nothing.

https://reviewboard.mozilla.org/r/84800/#review83402

Attachment #8799664 - Flags: review?(kaku) → review+

Tzuhao Kuo [:kaku]

Comment 6

•

7 years ago

mozreview-review

Comment on attachment 8799663 [details]
Bug 1308071. Part 2 - remove unused members.

https://reviewboard.mozilla.org/r/84798/#review83396

Attachment #8799663 - Flags: review?(kaku) → review+

JW Wang [:jwwang] [:jw_wang]

Assignee

Comment 7

•

7 years ago

mozreview-review

Comment on attachment 8799662 [details]
Bug 1308071. Part 1 - Change the type of MDSM::mInfo to Maybe<MediaInfo>.

https://reviewboard.mozilla.org/r/84796/#review83440

::: dom/media/MediaDecoderStateMachine.h:310
(Diff revision 1)
>  
>    MediaStatistics GetStatistics();
>  
> -  // This is called on the state machine thread and audio thread.
> -  // The decoder monitor must be obtained before calling this.
> -  bool HasAudio() const {
> +  bool HasAudio() const { return mInfo.ref().HasAudio(); }
> +  bool HasVideo() const { return mInfo.ref().HasVideo(); }
> +  MediaInfo& Info() { return mInfo.ref(); }

Will do.

::: dom/media/MediaDecoderStateMachine.cpp:315
(Diff revision 1)
> +    MediaInfo& info = Info();
>  
> -    if (mMaster->mInfo.mMetadataDuration.isSome()) {
> +    if (info.mMetadataDuration.isSome()) {
>        mMaster->RecomputeDuration();
> -    } else if (mMaster->mInfo.mUnadjustedMetadataEndTime.isSome()) {
> +    } else if (info.mUnadjustedMetadataEndTime.isSome()) {
>        RefPtr<Master> master = mMaster;
>        Reader()->AwaitStartTime()->Then(OwnerThread(), __func__,
> -        [master] () {
> +        [master, &info] () {
>            NS_ENSURE_TRUE_VOID(!master->IsShutdown());
> -          TimeUnit unadjusted = master->mInfo.mUnadjustedMetadataEndTime.ref();
> +          TimeUnit unadjusted = info.mUnadjustedMetadataEndTime.ref();
>            TimeUnit adjustment = master->mReader->StartTime();
> -          master->mInfo.mMetadataDuration.emplace(unadjusted - adjustment);
> +          info.mMetadataDuration.emplace(unadjusted - adjustment);

So I can capture |info| at #322.

However, it is not clear what keeps |info| valid in the lambda. I will declare |auto& info = master->mInfo.ref()| inside the lambda to make it clear.

Tzuhao Kuo [:kaku]

Comment 8

•

7 years ago

mozreview-review-reply

Comment on attachment 8799662 [details]
Bug 1308071. Part 1 - Change the type of MDSM::mInfo to Maybe<MediaInfo>.

https://reviewboard.mozilla.org/r/84796/#review83440

> So I can capture |info| at #322.
> 
> However, it is not clear what keeps |info| valid in the lambda. I will declare |auto& info = master->mInfo.ref()| inside the lambda to make it clear.

Since you have captured master, how about just replacing |master->mInfo| to |master->Info()|?

JW Wang [:jwwang] [:jw_wang]

Assignee

Comment 9

•

7 years ago

mozreview-review-reply

Comment on attachment 8799662 [details]
Bug 1308071. Part 1 - Change the type of MDSM::mInfo to Maybe<MediaInfo>.

https://reviewboard.mozilla.org/r/84796/#review83440

> Since you have captured master, how about just replacing |master->mInfo| to |master->Info()|?

We can't. |info| needs to be a mutable reference in the lambda while we want MDSM::Info() to be a const function (and thus returning a const reference).

Tzuhao Kuo [:kaku]

Comment 10

•

7 years ago

mozreview-review-reply

Comment on attachment 8799662 [details]
Bug 1308071. Part 1 - Change the type of MDSM::mInfo to Maybe<MediaInfo>.

https://reviewboard.mozilla.org/r/84796/#review83440

> We can't. |info| needs to be a mutable reference in the lambda while we want MDSM::Info() to be a const function (and thus returning a const reference).

Right, thanks!

Comment hidden (mozreview-request)

Comment on attachment 8799662 [details]
Bug 1308071. Part 1 - Change the type of MDSM::mInfo to Maybe<MediaInfo>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84796/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8799663 [details]
Bug 1308071. Part 2 - remove unused members.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84798/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8799664 [details]
Bug 1308071. Part 3 - early return from VisibilityChanged() if mInfo is nothing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84800/diff/1-2/

JW Wang [:jwwang] [:jw_wang]

Assignee

Comment 14

•

7 years ago

Thanks for the review!

Pulsebot

Comment 15

•

7 years ago

Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ec690c69bbc
Part 1 - Change the type of MDSM::mInfo to Maybe<MediaInfo>. r=kaku
https://hg.mozilla.org/integration/autoland/rev/44ab9ccc6c9d
Part 2 - remove unused members. r=kaku
https://hg.mozilla.org/integration/autoland/rev/d052f81bef3c
Part 3 - early return from VisibilityChanged() if mInfo is nothing. r=kaku

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 16

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/8ec690c69bbc
https://hg.mozilla.org/mozilla-central/rev/44ab9ccc6c9d
https://hg.mozilla.org/mozilla-central/rev/d052f81bef3c

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox52: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla52

You need to log in before you can comment on or make changes to this bug.

Bugzilla

Quick Search

Change the type of MDSM::mInfo to Maybe<MediaInfo>

Categories

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

Tracking

()

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(3 files)

Description

Updated

Comment 1

Comment 2

Comment 3

Updated

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Comment 15

Comment 16