Closed Bug 1411504 Opened 2 years ago Closed 2 years ago

Prepare MediaCache::Update() to run off the main thread

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(8 files)

59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
59 bytes, text/x-review-board-request
gerald
: review+
Details
We will make some changes to make it easier to run MediaCache::Update() off the main thread.
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Attachment #8921866 - Flags: review?(gsquelart)
Attachment #8921867 - Flags: review?(gsquelart)
Attachment #8921868 - Flags: review?(gsquelart)
Attachment #8921869 - Flags: review?(gsquelart)
Attachment #8921870 - Flags: review?(gsquelart)
Attachment #8921871 - Flags: review?(gsquelart)
Attachment #8921872 - Flags: review?(gsquelart)
Attachment #8921873 - Flags: review?(gsquelart)
Comment on attachment 8921866 [details]
Bug 1411504. P1 - always require MediaCache to have a thread to run Update() loops.

https://reviewboard.mozilla.org/r/192894/#review198788

::: dom/media/MediaCache.cpp:729
(Diff revision 1)
> +    // Note it is safe to pass an invalid pointer for operator=(std::nullptr_t)
> +    // is non-virtual and it will not access |this|.
> +    ClearOnShutdown(reinterpret_cast<MediaCache*>(0x1),

This is getting scarier with every patch!
Attachment #8921866 - Flags: review?(gsquelart) → review+
Comment on attachment 8921867 [details]
Bug 1411504. P2 - merge NotifyDataStarted() and SetTransportSeekable().

https://reviewboard.mozilla.org/r/192896/#review198790
Attachment #8921867 - Flags: review?(gsquelart) → review+
Comment on attachment 8921868 [details]
Bug 1411504. P3 - handle CacheClientSeek off the main thread for we will run Update() off the main thread.

https://reviewboard.mozilla.org/r/192898/#review198794
Attachment #8921868 - Flags: review?(gsquelart) → review+
Comment on attachment 8921869 [details]
Bug 1411504. P4 - handle CacheClientResume/CacheClientSuspend off the main thread.

https://reviewboard.mozilla.org/r/192900/#review198798
Attachment #8921869 - Flags: review?(gsquelart) → review+
Comment on attachment 8921870 [details]
Bug 1411504. P5 - handle CacheClientNotifySuspendedStatusChanged/QueueSuspendedStatusUpdate off the main thread.

https://reviewboard.mozilla.org/r/192902/#review198800
Attachment #8921870 - Flags: review?(gsquelart) → review+
Comment on attachment 8921871 [details]
Bug 1411504. P6 - acquire the lock for the entire scope of Update().

https://reviewboard.mozilla.org/r/192904/#review198802

::: dom/media/MediaCache.cpp:1201
(Diff revision 1)
>    {
> -    ReentrantMonitorAutoEnter mon(mReentrantMonitor);

If you're removing this RAII lock, there's no need for the scoped block around it anymore.
...

::: dom/media/MediaCache.cpp:1468
(Diff revision 1)
>        }
>      }
>  #ifdef DEBUG
>      mInUpdate = false;
>  #endif
>    }

(... End of ex-lock-scope block)

Alternatively, you could extend this block until the end of the function, because the `actions` decl&init at the top of the functions don't need to be guarded.
Attachment #8921871 - Flags: review?(gsquelart) → review+
Comment on attachment 8921872 [details]
Bug 1411504. P7 - don't change mChannelOffset in MediaCache::Update().

https://reviewboard.mozilla.org/r/192906/#review198816

::: dom/media/MediaCache.cpp:1434
(Diff revision 1)
> +          int64_t otherChannelOffset = other->mSeekTarget != -1
> +                                         ? other->mSeekTarget
> +                                         : other->mChannelOffset;
>            if (other->mResourceID == stream->mResourceID && !other->mClosed &&
>                !other->mClient->IsSuspended() &&
> -              OffsetToBlockIndexUnchecked(other->mChannelOffset) ==
> +              OffsetToBlockIndexUnchecked(otherChannelOffset) ==
>                  OffsetToBlockIndexUnchecked(desiredOffset)) {

You only use this otherChannelOffset in a test, but that test could be short-circuited by a previous test, rendering the '?:' work unnecessary -- unless the compiler can optimize that?
So I would suggest just replacing other->mChannelOffset with the ternary inline.
Attachment #8921872 - Flags: review?(gsquelart) → review+
Comment on attachment 8921873 [details]
Bug 1411504. P8 - return an error for RecreateChannel() when the decoder is shutting down.

https://reviewboard.mozilla.org/r/192908/#review198818
Attachment #8921873 - Flags: review?(gsquelart) → review+
Comment on attachment 8921866 [details]
Bug 1411504. P1 - always require MediaCache to have a thread to run Update() loops.

https://reviewboard.mozilla.org/r/192894/#review198826

::: dom/media/MediaCache.cpp:729
(Diff revision 1)
> +    // Note it is safe to pass an invalid pointer for operator=(std::nullptr_t)
> +    // is non-virtual and it will not access |this|.
> +    ClearOnShutdown(reinterpret_cast<MediaCache*>(0x1),

Right! I will file a new bug to fix this spooky thingy in a more comfortable way.
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d1d5317f4cd
P1 - always require MediaCache to have a thread to run Update() loops. r=gerald
https://hg.mozilla.org/integration/autoland/rev/11588f7b86a4
P2 - merge NotifyDataStarted() and SetTransportSeekable(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/1f4718a7caa8
P3 - handle CacheClientSeek off the main thread for we will run Update() off the main thread. r=gerald
https://hg.mozilla.org/integration/autoland/rev/58512042b2e1
P4 - handle CacheClientResume/CacheClientSuspend off the main thread. r=gerald
https://hg.mozilla.org/integration/autoland/rev/efd62ef842c1
P5 - handle CacheClientNotifySuspendedStatusChanged/QueueSuspendedStatusUpdate off the main thread. r=gerald
https://hg.mozilla.org/integration/autoland/rev/40d5fdf2780d
P6 - acquire the lock for the entire scope of Update(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/b72ec18b1413
P7 - don't change mChannelOffset in MediaCache::Update(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/d7f7d7cb6a4b
P8 - return an error for RecreateChannel() when the decoder is shutting down. r=gerald
You need to log in before you can comment on or make changes to this bug.