Finish cleaning up audio input devices code

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 fixed)

Details

Attachments

(10 attachments)

46 bytes, text/x-phabricator-request
pehrsons
: review+
Details | Review
46 bytes, text/x-phabricator-request
pehrsons
: review+
Details | Review
46 bytes, text/x-phabricator-request
pehrsons
: review+
Details | Review
46 bytes, text/x-phabricator-request
pehrsons
: review+
Details | Review
46 bytes, text/x-phabricator-request
pehrsons
: review+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

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

Updated

10 months ago
Priority: -- → P2
Assignee

Comment 1

9 months ago
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.
Assignee

Comment 6

9 months ago
It's not maintained and probably does not work anymore.
Assignee

Comment 10

9 months ago
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 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.
Attachment #9007809 - Flags: review+
Comment on attachment 9007810 [details]
Bug 1487057 - Part 2 - Remove RegisterForAudioMixing/NeedsMixing, they are unused. r?pehrsons

Andreas Pehrson [:pehrsons] has approved the revision.
Attachment #9007810 - Flags: review+
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.
Attachment #9007811 - Flags: review+
Comment on attachment 9007812 [details]
Bug 1487057 - Part 4 - Remove AsyncLatencyLogger and associated code. r?pehrsons

Andreas Pehrson [:pehrsons] has approved the revision.
Attachment #9007812 - Flags: review+
Comment on attachment 9007813 [details]
Bug 1487057 - Part 5 - Remove MediaEngineWebRTCMicrophoneSource::mStarted.  r?pehrsons

Andreas Pehrson [:pehrsons] has approved the revision.
Attachment #9007813 - Flags: review+

Comment 18

8 months ago
Pushed by paul@paul.cx:
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
Assignee

Comment 19

8 months ago
The last patch here was reviewed on irc.
Assignee

Updated

8 months ago
Depends on: 1503536
You need to log in before you can comment on or make changes to this bug.