1487057 - Finish cleaning up audio input devices code

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P2)

Description

[Paul Adenot (:padenot)]

Last step after bug 1404977, to reach something clean so we don't have to touch it for some time.

Comment 1

[Paul Adenot (:padenot)]

This a summary of the thread on which the methods of MediaEngineWebRTCMicrophoneSource are called:

# MediaManager thread (also called Owning thread or Media thread sometimes)

> MediaEngineWebRTCMicrophoneSource (ctor)
> nsString GetName() const override;
> nsCString GetUUID() const override;
> nsresult Allocate() override;
> nsresult Deallocate() override;
> nsresult SetTrack() override;
> nsresult Start() override;
> nsresult Stop() override;
> nsresult Reconfigure() override;
> dom::MediaSourceEnum GetMediaSource() const override
> nsresult TakePhoto() override
> uint32_t GetBestFitnessDistance() const override;
> void Shutdown() override;
> nsresult ReevaluateAllocation();
> nsresult UpdateSingleSource();
> void UpdateAECSettingsIfNeeded();
> void UpdateAGCSettingsIfNeeded();
> void UpdateNSSettingsIfNeeded();
> void ApplySettings();
> bool HasEnabledTrack() const;

# Never called (but required because of inheritance)

> bool RequiresSharing();

# Main thread

> void GetSettings() const override;

# Graph thread (also called MSG thread, audio callback thread)

> void Pull() override;
> void NotifyOutputData() override;
> void NotifyInputData() override;
> void DeviceChanged() override;
> uint32_t RequestedInputChannelCount() override
> void Disconnect() override;
> void InsertInGraph();
> void PacketizeAndProcess();
> bool PassThrough() const;
> void SetPassThrough();
> uint32_t GetRequestedInputChannelCount();
> void SetRequestedInputChannelCount();

Not too bad. The issue is with the attributes in those methods, it's really weird.

Comment 2

[Paul Adenot (:padenot)]

Patches are at https://hg.mozilla.org/users/paul_paul.cx/mozilla-unified/pushloghtml?changeset=b284fb47488072289c48c2e80376e5714c499741 for now, a couple are missing still.

Comment 3

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 1 - Move audio related classes in MediaEngineWebRTC.h to their own header files, clean up includes slightly. r?pehrsons

Comment 4

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 2 - Remove RegisterForAudioMixing/NeedsMixing, they are unused. r?pehrsons

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 3 - Turn the mAllocations array into an mAllocation UniquePtr, that can be nullptr. r?pehrsons

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 4 - Remove AsyncLatencyLogger and associated code. r?pehrsons

It's not maintained and probably does not work anymore.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 5 - Remove MediaEngineWebRTCMicrophoneSource::mStarted. r?pehrsons

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 6 - Reorganize attributes and document thread access better. r?pehrsons

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 7 - Flatten mAllocation and fix some locking. r?pehrsons

Comment 10

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 8 - Split MediaEngineWebRTCMicrophoneSource in two classes, one for control one for processing. r?pehrsons

Big but not complex:
- Remove the mutex
- Move all MSG thread to a new class (AudioInputProcessing)
- Remove the WebRTCAudioDataListener class, AudioInputProcessing is the listener
- Use message passing for all modifications to the AudioInputProcessing.

Comment 11

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9007809 [details]
Bug 1487057 - Part 1 - Move audio related classes in MediaEngineWebRTC.h to their own header files, clean up includes slightly. r?pehrsons

Andreas Pehrson [:pehrsons] has approved the revision.

Comment 12

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9007810 [details]
Bug 1487057 - Part 2 - Remove RegisterForAudioMixing/NeedsMixing, they are unused. r?pehrsons

Andreas Pehrson [:pehrsons] has approved the revision.

Comment 13

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9007811 [details]
Bug 1487057 - Part 3 - Turn the mAllocations array into an mAllocation UniquePtr, that can be nullptr. r?pehrsons

Andreas Pehrson [:pehrsons] has approved the revision.

Comment 14

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9007812 [details]
Bug 1487057 - Part 4 - Remove AsyncLatencyLogger and associated code. r?pehrsons

Andreas Pehrson [:pehrsons] has approved the revision.

Comment 15

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9007813 [details]
Bug 1487057 - Part 5 - Remove MediaEngineWebRTCMicrophoneSource::mStarted.  r?pehrsons

Andreas Pehrson [:pehrsons] has approved the revision.

Comment 16

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 9 - Make SourceMediaStream::SetEnded go through the message queue so it's in the right order w.r.t. Stop. r?pehrsons

Comment 17

[Paul Adenot (:padenot)]

Attachment: Bug 1487057 - Part 10 - Remove MediaEngineWebRTCAudio::mEnabled. r?pehrsons

It was redundant with mState.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd0c87b160b
Part 1 - Move audio related classes in MediaEngineWebRTC.h to their own header files, clean up includes slightly. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6e230af698
Part 2 - Remove RegisterForAudioMixing/NeedsMixing, they are unused. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff222ff2a72
Part 3 - Turn the mAllocations array into an mAllocation UniquePtr, that can be nullptr. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/304b4f68b942
Part 4 - Remove AsyncLatencyLogger and associated code. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/746f70de2691
Part 5 - Remove MediaEngineWebRTCMicrophoneSource::mStarted.  r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/582e630a2fcb
Part 6 - Reorganize attributes and document thread access better. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/273c92182c3c
Part 7 - Flatten mAllocation and fix some locking. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/45d2c462dc92
Part 8 - Split MediaEngineWebRTCMicrophoneSource in two classes, one for control one for processing. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a790218e20
Part 9 - Make SourceMediaStream::SetEnded go through the message queue so it's in the right order w.r.t. Stop. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cf36402deed
Part 10 - Remove MediaEngineWebRTCAudio::mEnabled. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/0190c5793ffe
Part 11 - Work around the fact that EndTrack uses mCommands. r=pehrsons

Comment 19

[Paul Adenot (:padenot)]

The last patch here was reviewed on irc.

Comment 20

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/efd0c87b160b
https://hg.mozilla.org/mozilla-central/rev/7e6e230af698
https://hg.mozilla.org/mozilla-central/rev/2ff222ff2a72
https://hg.mozilla.org/mozilla-central/rev/304b4f68b942
https://hg.mozilla.org/mozilla-central/rev/746f70de2691
https://hg.mozilla.org/mozilla-central/rev/582e630a2fcb
https://hg.mozilla.org/mozilla-central/rev/273c92182c3c
https://hg.mozilla.org/mozilla-central/rev/45d2c462dc92
https://hg.mozilla.org/mozilla-central/rev/e1a790218e20
https://hg.mozilla.org/mozilla-central/rev/9cf36402deed
https://hg.mozilla.org/mozilla-central/rev/0190c5793ffe