Closed
Bug 1308071
Opened 7 years ago
Closed 7 years ago
Change the type of MDSM::mInfo to Maybe<MediaInfo>
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(3 files)
This helps catch invalid usage of mInfo before reading metadata.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8799662 -
Flags: review?(kaku)
Attachment #8799663 -
Flags: review?(kaku)
Attachment #8799664 -
Flags: review?(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+
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+
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+
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.
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()|?
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).
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
Thanks for the review!
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
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.
Description
•