Remove 'Media' Prefix from things we're going to hoist to XPCOM

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(4 attachments)

No description provided.
We'll want this for MediaTaskQueue too.
Summary: Rename MediaPromise to MozPromise → Remove 'Media' Prefix from things we're going to hoist to XPCOM
(In reply to Bobby Holley (:bholley) from comment #1)
> We'll want this for MediaTaskQueue too.

\0/


One gotcha is that MediaTaskQueue initializes MSCOM with "multithreaded" mode [1], cos that's what media needs, but not all users of MSCOM use this mode [2]. My (limited) understanding is that mixing MSCOM objects across compartment models is a perf disaster, and can cause some functions to fail.

So new users of MozTaskQueue would need to be careful when using Win32 APIs... Maybe add a comment to the header about what MSCOM compartment model the threads are init'd with?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/ThreadPoolCOMListener.cpp#16
[2] http://mxr.mozilla.org/mozilla-central/search?string=coinitialize
That only affects SharedThreadPool, right? I was going to leave that for now for various reasons, though maybe I should hoist it too.
Yes that only affect SharedThreadPool threads... Can MediaTaskQueues run on other thread pools now?
(In reply to Chris Pearce (:cpearce) from comment #5)
> Yes that only affect SharedThreadPool threads... Can MediaTaskQueues run on
> other thread pools now?

I was planning to make MediaTaskQueue run on an nsIThreadPool instead of a SharedThreadPool, but maybe we should just hoist SharedThreadPool. We'll see.
Otherwise this name will collide with the rename of MediaTaskQueue to TaskQueue.
It's also a better naming convention, because it generalizes to things that are
owned by an AbstractThread that is not a Task Queue.
Attachment #8634914 - Flags: review?(gsquelart)
I did my a quick best-effort pass to fix up the most egregious ordering
problems. I left some big pre-existing messes alone.
Attachment #8634916 - Flags: review?(gsquelart)
Comment on attachment 8634913 [details] [diff] [review]
Part 1 - Rename MediaPromise to MozPromise. v1

Review of attachment 8634913 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nano-nits.

::: dom/media/MozPromise.h
@@ +810,1 @@
>   * variable for a class waiting on a media promise.

This "media" escaped!

::: dom/media/mediasource/SourceBufferContentManager.h
@@ +7,5 @@
>  #ifndef MOZILLA_SOURCEBUFFERCONTENTMANAGER_H_
>  #define MOZILLA_SOURCEBUFFERCONTENTMANAGER_H_
>  
>  #include "MediaData.h"
> +#include "MozPromise.h"

This #include list is almost sorted, up to you if you want to move MozPromise.h down one line for OCD comfort. (And optionally move the nsString.h below up to make the list fully sorted; unless it's kept at the end to show it's not in the same area as media files; in which case maybe you'd want to more MozPromise.h further down to separate it from the media crowd). But then #include lists are inconsistently sorted across the project, so it probably doesn't matter until a mass sorting happens. This review comment is now entirely too long for what it's worth.
Attachment #8634913 - Flags: review?(gsquelart) → review+
(In reply to Gerald Squelart [:gerald] from comment #11)
> Comment on attachment 8634913 [details] [diff] [review]

> crowd). But then #include lists are inconsistently sorted across the
> project, so it probably doesn't matter until a mass sorting happens. This
> review comment is now entirely too long for what it's worth.

Everything in the new MSE or new media architecture have #include sorted.
I sorted them in Part 4, as well as I could.
Comment on attachment 8634914 [details] [diff] [review]
Part 2 - Rename "TaskQueue()" accessor to "OwnerThread()". v1

Review of attachment 8634914 [details] [diff] [review]:
-----------------------------------------------------------------

r+
I've noticed you've also renamed "TaskQueue()" to just "Queue()" in TestMozPromise.cpp, should it be mentioned in the patch description?
Attachment #8634914 - Flags: review?(gsquelart) → review+
Comment on attachment 8634915 [details] [diff] [review]
Part 3 - Rename MediaTaskQueue to TaskQueue. v1

Review of attachment 8634915 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the usual optional reordering request (ignore if done in next patch).

::: dom/media/MediaDecoderStateMachine.h
@@ +97,5 @@
>  
>  namespace mozilla {
>  
>  class AudioSegment;
> +class TaskQueue;

Ordering please (if not already done in following patch).

::: dom/media/MediaFormatReader.h
@@ +10,5 @@
>  #include "mozilla/Atomics.h"
>  #include "mozilla/Maybe.h"
>  #include "MediaDataDemuxer.h"
>  #include "MediaDecoderReader.h"
> +#include "TaskQueue.h"

Ordering please (if not already done in following patch).

::: dom/media/TaskQueue.cpp
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "TaskQueue.h"

Ordering please (if not already done in following patch).

::: dom/media/omx/MediaCodecReader.cpp
@@ +30,5 @@
>  
>  #include "gfx2DGlue.h"
>  
>  #include "MediaStreamSource.h"
> +#include "TaskQueue.h"

Ordering please (if not already done in following patch).

::: dom/media/platforms/agnostic/eme/SamplesWaitingForKey.h
@@ +6,5 @@
>  
>  #ifndef SamplesWaitingForKey_h_
>  #define SamplesWaitingForKey_h_
>  
> +#include "TaskQueue.h"

Ordering please (if not already done in following patch).

::: dom/media/systemservices/MediaSystemResourceManager.h
@@ +23,5 @@
>  } // namespace media
>  
>  class MediaSystemResourceClient;
>  class MediaSystemResourceReservationListener;
> +class TaskQueue;

Ordering please (if not already done in following patch).
Attachment #8634915 - Flags: review?(gsquelart) → review+
Comment on attachment 8634916 [details] [diff] [review]
Part 4 - Move various includes into the mozilla namespace. v1

Review of attachment 8634916 [details] [diff] [review]:
-----------------------------------------------------------------

Like.
Attachment #8634916 - Flags: review?(gsquelart) → review+
(In reply to Gerald Squelart [:gerald] from comment #11)
> This "media" escaped!

Good catch!

(In reply to Gerald Squelart [:gerald] from comment #14)
> I've noticed you've also renamed "TaskQueue()" to just "Queue()" in
> TestMozPromise.cpp, should it be mentioned in the patch description?

Ok.

Thanks for the quick review!
You need to log in before you can comment on or make changes to this bug.