Closed Bug 1669570 Opened 9 months ago Closed 8 months ago

Rename MediaController thread so it doesn't collide with existing classes or files

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
84 Branch
Tracking Status
firefox84 --- fixed

People

(Reporter: bryce, Assigned: bryce)

References

Details

Attachments

(2 files)

Bug 1653638 changed the name of the 'MediaPlayback' thread to 'MediaController' as that thread per the comment at https://phabricator.services.mozilla.com/D85543

However, 'MediaController' is the name of an object used to control media elements with little relation to the media controller thread. This can lead to confusion when encountering one or the other, for example the following log

[Child 3385495, MediaController #2] WARNING: 'aIndex >= GetSize()', file /home/blah/dev/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsDeque.h, line 357

could be attributed to the MediaController class, rather than something taking place on the the thread (a mistake I made). Similarly, when I tried to find the source of that warning by searching for 'MediaController' in searchfox[0] the line I'm looking for is hidden by many results relating to the class.

Ideally we use a name that has no collisions so that logs are unambiguously attributable to the thread. This rules out 'MediaManager' as such a class exists.

'MediaSupervisor' or 'MediaDirector' would work.

[0] https://searchfox.org/mozilla-central/search?q=MediaController&path=&case=false&regexp=false

Summary: Rename MediaController thread to something else → Rename MediaController thread from MediaController so it doesn't collide with existing classes or files
Summary: Rename MediaController thread from MediaController so it doesn't collide with existing classes or files → Rename MediaController thread so it doesn't collide with existing classes or files

:jya, given the above do you have a preferred name for this thread?

Flags: needinfo?(jyavenard)

See also bug 1658614 for work to keep our profiler thread names correct through renames.

See Also: → 1658614

I don't see how there could be confusion to be honest, NS_WARNING has always shown the process name and the thread name.

It's never about a class name in there.

I changed from Playback to Controller because it's not always used for playback, it's just by webaudio or webrtc, it's also used by the viarous IPC actors

If you feel a name is more appropriate. That thread is used to control media related tasks ; it doesn't actually perform process intensive tasks unlike the Decoder or Encore thread.

What about instead of renaming the thread, we change the prefix instead from "Media" to "TMedia"
https://searchfox.org/mozilla-central/source/dom/media/VideoUtils.cpp#243-260

Flags: needinfo?(jyavenard)

(In reply to Jean-Yves Avenard [:jya] from comment #3)

I don't see how there could be confusion to be honest, NS_WARNING has always shown the process name and the thread name.

It's never about a class name in there.

Several threads are named because they're used by classes or involve functionality related to certain classes.

  • MediaDecoderStateMachine is both a thread and a class -- the class runs on the thread.
  • MediaPDecoder is a thread that is used by PlatformDecoderModules and related functionality.

When seeing these names in logs it makes sense to assume the warning involves classes related to the thread name because these threads are named to indicate a relationship to these classes. But for MediaController the thread and the class don't really have a relationship, despite the similar names. In the wild I think this could get even more confusing, as users may have less context than we do and risk further confusion.

I changed from Playback to Controller because it's not always used for playback, it's just by webaudio or webrtc, it's also used by the viarous IPC actors

If you feel a name is more appropriate. That thread is used to control media related tasks ; it doesn't actually perform process intensive tasks unlike the Decoder or Encore thread.

What about instead of renaming the thread, we change the prefix instead from "Media" to "TMedia"
https://searchfox.org/mozilla-central/source/dom/media/VideoUtils.cpp#243-260

I worry this would still have an association with the media controller. We'd have TMediaDecoderStateMachine, TMediaPlatformDecoder, etc and those would be threads tied to certain classes, while TMediaController would have little association to the MediaController.

This renames the thread and identifiers derived from the thread's name. This is
to avoid ambiguity over if the thread relates to the MediaController class,
which it does not.

Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/721003f093fb
Rename MediaController thread to MediaSupervisor. r=jya
https://hg.mozilla.org/integration/autoland/rev/8201964e47e7
Rename MeidaController to MediaSupervisor for the profiler. r=alwu
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
You need to log in before you can comment on or make changes to this bug.