Closed Bug 1405962 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

Once more step toward enabling OMT data delivery.
Assignee: nobody → jwwang
Blocks: 1382978
Priority: -- → P3
Attachment #8915507 - Flags: review?(gsquelart)
Attachment #8915508 - Flags: review?(gsquelart)
Comment on attachment 8915508 [details]
Bug 1405962. P2 - use thread-safe ref counting.

https://reviewboard.mozilla.org/r/186724/#review192026
Attachment #8915508 - Flags: review?(gsquelart) → review+
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.
Attachment #8915507 - Flags: review?(gsquelart) → review+
Issues addressed. Thanks for the review.
Pushed by jwwang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/8d6aae7d3ebb
https://hg.mozilla.org/mozilla-central/rev/acdec618988d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1428688
No longer blocks: 1428688
See Also: → 1466569
You need to log in before you can comment on or make changes to this bug.