Closed
Bug 1308071
Opened 9 years ago
Closed 9 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•9 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•9 years ago
|
Attachment #8799662 -
Flags: review?(kaku)
Attachment #8799663 -
Flags: review?(kaku)
Attachment #8799664 -
Flags: review?(kaku)
Comment 4•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
Thanks for the review!
Comment 15•9 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•9 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: 9 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
•