1405962 - Give MediaCache a thread to run data callbacks from the HTTP channel

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

Once more step toward enabling OMT data delivery.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1405962. P1 - give MediaCache a thread on which we will run data callbacks from the HTTP channel.

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

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1405962. P2 - use thread-safe ref counting.

These listeners will be AddRefed/Released off the main thread when
OMT data delivery is enabled.

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

Comment 3

[Gerald Squelart (he/him)]

Comment on attachment 8915508 [details]
Bug 1405962. P2 - use thread-safe ref counting.

https://reviewboard.mozilla.org/r/186724/#review192026

Comment 4

[Gerald Squelart (he/him)]

Comment on attachment 8915507 [details]
Bug 1405962. P1 - give MediaCache a thread on which we will run data callbacks from the HTTP channel.

https://reviewboard.mozilla.org/r/186722/#review192020

::: dom/media/ChannelMediaResource.cpp:304
(Diff revision 1)
>    // Fires an initial progress event.
>    owner->DownloadProgressed();
>  
> +  // TODO: Don't turn this on until we fix all data races.
> +  nsCOMPtr<nsIThreadRetargetableRequest> retarget;
> +  if (Preferences::GetBool("media.omt_data_delievery.enabled", false) &&

"delievery" -> "delivery"

For this OMT feature, I would expect this pref to stay available for a while after shipping, so we have a quick panic button in case things go wrong in the field.
So I would suggest you make it a proper pref: Add it in all.js and MediaPrefs.h.

::: dom/media/MediaCache.cpp:274
(Diff revision 1)
>  #endif
>    {
>      NS_ASSERTION(NS_IsMainThread(), "Only construct MediaCache on main thread");
>      MOZ_COUNT_CTOR(MediaCache);
>      MediaCacheFlusher::RegisterMediaCache(this);
> +    NS_NewNamedThread("MediaCache", getter_AddRefs(mThread));

I'm guessing you will allow&support a null mThread.
Still, please check the return value and handle NS_FAILED(rv), if only to display a warning: At least it shows you have thought about this situation and decided there is nothing special to do.

::: dom/media/MediaCache.cpp:439
(Diff revision 1)
> +  // The thread on which we will run data callbacks from the channels.
> +  nsCOMPtr<nsIThread> mThread;

If you will support a null mThread, please add a mention about that in the comment.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8915507 [details]
Bug 1405962. P1 - give MediaCache a thread on which we will run data callbacks from the HTTP channel.

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

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8915508 [details]
Bug 1405962. P2 - use thread-safe ref counting.

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

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Issues addressed. Thanks for the review.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d6aae7d3ebb
P1 - give MediaCache a thread on which we will run data callbacks from the HTTP channel. r=gerald
https://hg.mozilla.org/integration/autoland/rev/acdec618988d
P2 - use thread-safe ref counting. r=gerald

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8d6aae7d3ebb
https://hg.mozilla.org/mozilla-central/rev/acdec618988d