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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Bobby Holley (:bholley)]

We'll want this for MediaTaskQueue too.

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cb7599fa11b

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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

Comment 4

[Bobby Holley (:bholley)]

That only affects SharedThreadPool, right? I was going to leave that for now for various reasons, though maybe I should hoist it too.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes that only affect SharedThreadPool threads... Can MediaTaskQueues run on other thread pools now?

Comment 6

[Bobby Holley (:bholley)]

(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.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1 - Rename MediaPromise to MozPromise. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Rename "TaskQueue()" accessor to "OwnerThread()". v1

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.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 3 - Rename MediaTaskQueue to TaskQueue. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 4 - Move various includes into the mozilla namespace. v1

I did my a quick best-effort pass to fix up the most egregious ordering
problems. I left some big pre-existing messes alone.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 12

[Jean-Yves Avenard [:jya]]

(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.

Comment 13

[Bobby Holley (:bholley)]

I sorted them in Part 4, as well as I could.

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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?

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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).

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 17

[Bobby Holley (:bholley)]

(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!

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc714b35dd04
https://hg.mozilla.org/integration/mozilla-inbound/rev/e522f33ca3e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3cad5f3d77
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc592979091

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc714b35dd04
https://hg.mozilla.org/mozilla-central/rev/e522f33ca3e6
https://hg.mozilla.org/mozilla-central/rev/9f3cad5f3d77
https://hg.mozilla.org/mozilla-central/rev/9fc592979091