Reset the AEC on device change

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

3 years ago
No description provided.
Assignee

Comment 1

3 years ago
Posted patch Part 1, reset the processing (obsolete) — Splinter Review
Assignee: nobody → padenot
Attachment #8743927 - Flags: review?(rjesup)
Assignee

Comment 2

3 years ago
Attachment #8743928 - Flags: review?(rjesup)
Comment on attachment 8743927 [details] [diff] [review]
Part 1, reset the processing

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

::: dom/media/GraphDriver.cpp
@@ +22,5 @@
>  #define STREAM_LOG(type, msg) MOZ_LOG(gMediaStreamGraphLog, type, msg)
>  
>  // We don't use NSPR log here because we want this interleaved with adb logcat
>  // on Android/B2G
> +#define ENABLE_LIFECYCLE_LOG

will need to comment this again

::: dom/media/MediaStreamGraph.cpp
@@ +45,5 @@
>  
>  LazyLogModule gMediaStreamGraphLog("MediaStreamGraph");
>  #define STREAM_LOG(type, msg) MOZ_LOG(gMediaStreamGraphLog, type, msg)
>  
> +#define ENABLE_LIFECYCLE_LOG

ditto

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +502,5 @@
> +#define ResetProcessingIfNeeded(_processing)                      \
> +do {                                                              \
> +webrtc::_processing##Modes mode;                                  \
> +int rv = mVoEProcessing->Get##_processing##Status(enabled, mode); \
> +if (rv) {                                                         \

indent contents of do {} block

@@ +504,5 @@
> +webrtc::_processing##Modes mode;                                  \
> +int rv = mVoEProcessing->Get##_processing##Status(enabled, mode); \
> +if (rv) {                                                         \
> +  NS_WARNING("Could not get the status of the "                   \
> +   #_processing "on device change.");                             \

Add a space before 'on'

@@ +521,5 @@
> +  if (rv) {                                                       \
> +    NS_WARNING("Could not reset the status of the "               \
> +    #_processing " on device change.");                           \
> +    return;                                                       \
> +  }                                                               \

Issues with enabled and doing this if already false (per IRC)
Attachment #8743927 - Flags: review?(rjesup) → review-
Comment on attachment 8743928 [details] [diff] [review]
Part 2, remove OSX speicifc workarounds

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

::: dom/media/GraphDriver.cpp
@@ +1005,5 @@
>  #endif
>  }
>  
>  void
> +  AudioCallbackDriver::DeviceChangedCallback() {

spurious spaces
Attachment #8743928 - Flags: review?(rjesup) → review+
Assignee

Comment 5

3 years ago
Attachment #8743927 - Attachment is obsolete: true
Attachment #8743949 - Flags: review?(rjesup)
Assignee

Comment 6

3 years ago
Addressed nits.
Attachment #8743950 - Flags: review+
Comment on attachment 8743949 [details] [diff] [review]
Part 1, reset the processing

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

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +509,5 @@
> +    return;                                                         \
> +  }                                                                 \
> +                                                                    \
> +  if (enabled) {                                                    \
> +    rv = mVoEProcessing->Set##_processing##Status(!enabled);        \

false

@@ +516,5 @@
> +      #_processing " on device change.");                           \
> +      return;                                                       \
> +    }                                                               \
> +                                                                    \
> +    rv = mVoEProcessing->Set##_processing##Status(enabled);         \

true
Attachment #8743949 - Flags: review?(rjesup) → review+
Assignee

Updated

3 years ago
Attachment #8743928 - Attachment is obsolete: true

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5663efa6c13c
https://hg.mozilla.org/mozilla-central/rev/099d4414bd63
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.