Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly remove observer upon deconstruction #8

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

kanat
Copy link
Collaborator

@kanat kanat commented Sep 20, 2023

@kanat kanat merged commit 829f351 into patch/m114 Sep 20, 2023
@kanat kanat deleted the patch/android/memory-leak branch September 20, 2023 18:11
kanat added a commit that referenced this pull request Sep 20, 2023
kanat added a commit that referenced this pull request Sep 20, 2023
kanat added a commit that referenced this pull request Mar 25, 2024
(cherry picked from commit 829f351)
(cherry picked from commit 01ed14c)
kanat pushed a commit that referenced this pull request Apr 5, 2024
allow listen-only mode in AudioUnit, adjust when category changes (#2)

release mic when category changes (#5)

Change defaults to iOS defaults (#7)

Sync audio session config (#8)

feat: support bypass voice processing for iOS. (#15)

Remove MacBookPro audio pan right code (#22)

fix: Fix can't open mic alone when built-in AEC is enabled. (#29)

feat: add audio device changes detect for windows. (#41)

fix Linux compile (#47)

AudioUnit: Don't rely on category switch for mic indicator to turn off (#52)

Stop recording on mute (turn off mic indicator) (#55)

Cherry pick audio selection from m97 release (#35)

[Mac] Allow audio device selection (#21)

RTCAudioDeviceModule.outputDevice / inputDevice getter and setter (#80)

Co-authored-by: Hiroshi Horie <[email protected]>
Co-authored-by: David Zhao <[email protected]>
kanat added a commit that referenced this pull request Apr 5, 2024
* Audio Device Optimization

allow listen-only mode in AudioUnit, adjust when category changes (#2)

release mic when category changes (#5)

Change defaults to iOS defaults (#7)

Sync audio session config (#8)

feat: support bypass voice processing for iOS. (#15)

Remove MacBookPro audio pan right code (#22)

fix: Fix can't open mic alone when built-in AEC is enabled. (#29)

feat: add audio device changes detect for windows. (#41)

fix Linux compile (#47)

AudioUnit: Don't rely on category switch for mic indicator to turn off (#52)

Stop recording on mute (turn off mic indicator) (#55)

Cherry pick audio selection from m97 release (#35)

[Mac] Allow audio device selection (#21)

RTCAudioDeviceModule.outputDevice / inputDevice getter and setter (#80)

Co-authored-by: Hiroshi Horie <[email protected]>
Co-authored-by: David Zhao <[email protected]>

* fix compilation errors

---------

Co-authored-by: CloudWebRTC <[email protected]>
Co-authored-by: Hiroshi Horie <[email protected]>
Co-authored-by: David Zhao <[email protected]>
santhoshvai pushed a commit that referenced this pull request Nov 20, 2024
This was a fun bug which proved to be challenging to find a good
solution for. The issue comes from the combination of partial
reliability and stream resetting, which are covered in different RFCs,
and where they don't refer to each other...

Stream resetting (RFC 6525) is used in WebRTC for closing a Data
Channel, and is done by signaling to the receiver that the stream
sequence number (SSN) should be set to zero (0) at some time. Partial
reliability (RFC 3758) - and expiring messages that will not be
retransmitted - is done by signaling that the SSN should be set to a
certain value at a certain TSN, as the messages up until the provided
SSN are not to be expected to be sent again.

As these two functionalities both work by signaling to the receiver
what the next expected SSN should be, they need to do it correctly not
to overwrite each others' intent. And here was the bug. An example
scenario where this caused issues, where we are Z (the receiver),
getting packets from the sender (A):

 5  A->Z          DATA (TSN=30, B, SID=2, SSN=0)
 6          Z->A  SACK (Ack=30)
 7  A->Z          DATA (TSN=31, E, SID=2, SSN=0)
 8  A->Z          RE_CONFIG (REQ=30, TSN=31, SID=2)
 9          Z->A  RE_CONFIG (RESP=30, Performed)
10          Z->A  SACK (Ack=31)
11  A->Z          DATA (TSN=32, SID=1)
12  A->Z          FORWARD_TSN (TSN=32, SID=2, SSN=0)

Let's assume that the path Z->A had packet loss and A never really
received our responses (#6, #9, #10) in time.

At #5, Z receives a DATA fragment, which it acks, and at #7 the end of
that message. The stream is then reset (#8) which it signals that it
was performed (#9) and acked (#10), and data on another stream (2) was
received (#11). Since A hasn't received any ACKS yet, and those chunks
on SID=2 all expired, A sends a FORWARD-TSN saying that "Skip to TSN=32,
and don't expect SID=2, SSN=0". That makes the receiver expect the SSN
on SID=2 to be SSN=1 next time at TSN=32.

But that's not good at all - A reset the stream at #8 and will want to
send the next message on SID=2 using SSN=0 - not 1. The FORWARD-TSN
clearly can't have a TSN that is beyond the stream reset TSN for that
stream.

This is just one example - combining stream resetting and partial
reliability, together with a lossy network, and different variants of
this can occur, which results in the receiver possibly not delivering
packets because it expects a different SSN than the one the sender is
later using.

So this CL adds "breakpoints" to how far a FORWARD-TSN can stretch. It
will simply not cross any Stream Reset last assigned TSNs, and only when
a receiver has acked that all TSNs up till the Stream Reset last
assigned TSN has been received, it will proceed expiring chunks after
that.

Bug: webrtc:14600
Change-Id: Ibae8c9308f5dfe8d734377d42cce653e69e95731
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/321600
Commit-Queue: Victor Boivie <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#40829}
santhoshvai pushed a commit that referenced this pull request Nov 20, 2024
(cherry picked from commit 829f351)
(cherry picked from commit 01ed14c)
santhoshvai pushed a commit that referenced this pull request Nov 20, 2024
* Audio Device Optimization

allow listen-only mode in AudioUnit, adjust when category changes (#2)

release mic when category changes (#5)

Change defaults to iOS defaults (#7)

Sync audio session config (#8)

feat: support bypass voice processing for iOS. (#15)

Remove MacBookPro audio pan right code (#22)

fix: Fix can't open mic alone when built-in AEC is enabled. (#29)

feat: add audio device changes detect for windows. (#41)

fix Linux compile (#47)

AudioUnit: Don't rely on category switch for mic indicator to turn off (#52)

Stop recording on mute (turn off mic indicator) (#55)

Cherry pick audio selection from m97 release (#35)

[Mac] Allow audio device selection (#21)

RTCAudioDeviceModule.outputDevice / inputDevice getter and setter (#80)

Co-authored-by: Hiroshi Horie <[email protected]>
Co-authored-by: David Zhao <[email protected]>

* fix compilation errors

---------

Co-authored-by: CloudWebRTC <[email protected]>
Co-authored-by: Hiroshi Horie <[email protected]>
Co-authored-by: David Zhao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant