When an SCTP stream is closing, a stream reset needs
to be sent from both ends.
The remote was not sending a stream reset and quickly
opening another stream with the same StreamID could
cause SCTP errors.
Bug: webrtc:13994
Change-Id: I3abc74ddc88b3fcf7e6495d76e7d77f52280b5d1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260922
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36773}
When streams were to be reset, but there was already an ongoing
stream reset command in-flight, those streams wouldn't be properly
reset. When multiple streams were reset close to each other (within
an RTT), some streams would not have their SSNs reset, which resulted
in the stream resuming the SSN sequence. This could result in ordered
streams not delivering all messages as the receiver wouldn't deliver any
messages with SSN different from the expected SSN=0.
In WebRTC data channels, this would be triggered if multiple channels
were closed at roughly the same time, then re-opened, and continued
to be used in ordered mode. Unordered messages would still be delivered,
but the stream state could be wrong as the DATA_CHANNEL_ACK message is
sent ordered, and possibly not delivered.
There were unit tests for this, but not on the socket level using
real components, but just on the stream reset handler using mocks,
where this issue wasn't found. Also, those mocks didn't validate that
the correct parameters were provided, so that's fixed now.
The root cause was the PrepareResetStreams was only called if there
wasn't an ongoing stream reset operation in progress. One may try to
solve it by always calling PrepareResetStreams also when there is an
ongoing request, or to call it when the request has finished. One would
then realize that when the response of the outgoing stream request is
received, and CommitResetStreams is called, it would reset all paused
and (prepared) to-be-reset streams - not just the ones in the outgoing
stream request.
One cause of this was the lack of a single source of truth of the stream
states. The SendQueue kept track of which streams that were paused, but
the stream reset handler kept track of which streams that were
resetting. As that's error prone, this CL moves the source of truth
completely to the SendQueue and defining explicit stream pause states. A
stream can be in one of these possible states:
* Not paused. This is the default for an active stream.
* Pending to be paused. This is when it's about to be reset, but
there is a message that has been partly sent, with fragments
remaining to be sent before it can be paused.
* Paused, with no partly sent message. In this state, it's ready to
be reset.
* Resetting. A stream transitions into this state when it has been
paused and has been included in an outgoing stream reset request.
When this request has been responded to, the stream can really be
reset (SSN=0, MID=0).
This CL also improves logging, and adds socket tests to catch this
issue.
Bug: webrtc:13994, chromium:1320194
Change-Id: I883570d1f277bc01e52b1afad62d6be2aca930a2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261180
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36771}
This ensures that only the compilation units that actually need
ChannelManager details can see it.
Bug: webrtc:13931
Change-Id: Iddd37580c0ceceba5b7095e84b981e6a525b2800
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261200
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36762}
The field trial will remain as a kill-switch for a few weeks while
decisions about sync decoding are being made.
Change-Id: I6034d25a129404e94ab8830f51e83667e285c785
Bug: webrtc:14003
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260327
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36760}
Metrics for calculating sent/recv key frame rate:
num_encoded_frames -- number of encoded frames
num_decoded_frames -- number of decoded frames
num_send_key_frames -- number of sent keyframes
num_recv_key_frames -- number of received keyframes
Metrics for frame size (in bytes) distribution:
recv_key_frame_size_bytes -- key frame size
recv_delta_frame_size_bytes -- delta frame size
The stats are only reported if the user so requests. This is
because the stats may not be useful for regression tracking,
but mainly for adhoc analysis.
Tested:
```
~/s/w/src (frame_stats↑1|✚1) $ autoninja -C out/Debug && out/Debug/test_support_unittests --gtest_filter="PeerConnectionE2EQualityTestSmokeTest.Smoke" 2>/dev/null
Note: Google Test filter = PeerConnectionE2EQualityTestSmokeTest.Smoke
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PeerConnectionE2EQualityTestSmokeTest
[ RUN ] PeerConnectionE2EQualityTestSmokeTest.Smoke
...
RESULT num_encoded_frames: Smoke/alice-video= 31 count_biggerIsBetter
RESULT num_decoded_frames: Smoke/alice-video= 31 count_biggerIsBetter
RESULT num_send_key_frames: Smoke/alice-video= 2 count_biggerIsBetter
RESULT num_recv_key_frames: Smoke/alice-video= 2 count_biggerIsBetter
RESULT recv_key_frame_size_bytes: Smoke/alice-video= {1847,126} count_biggerIsBetter
RESULT recv_delta_frame_size_bytes: Smoke/alice-video= {1150.1034,393.66329} count_biggerIsBetter
...
RESULT num_encoded_frames: Smoke/charlie-video= 31 count_biggerIsBetter
RESULT num_decoded_frames: Smoke/charlie-video= 31 count_biggerIsBetter
RESULT num_send_key_frames: Smoke/charlie-video= 2 count_biggerIsBetter
RESULT num_recv_key_frames: Smoke/charlie-video= 2 count_biggerIsBetter
RESULT recv_key_frame_size_bytes: Smoke/charlie-video= {1847,126} count_biggerIsBetter
RESULT recv_delta_frame_size_bytes: Smoke/charlie-video= {1150.1034,393.66329} count_biggerIsBetter
...
[----------] 1 test from PeerConnectionE2EQualityTestSmokeTest (2196 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (2196 ms total)
[ PASSED ] 1 test.
```
Bug: webrtc:14019
Change-Id: I3d5ea0cf659a6299c6d94f5004cbf0c763b267e5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260924
Reviewed-by: Artem Titov <titovartem@webrtc.org>
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36759}
This breaks the link from sdp_offer_answer.cc to channel.h.
Bug: webrtc:13931
Change-Id: I75608f75713bf4e69013ac5f5b17c19e53d07519
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261060
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36757}
This also hides the existence of the classes VideoChannel and
VoiceChannel from anything that does not include "channel.h".
Bug: webrtc:13931
Change-Id: I080a692b6acfd5d2d0401ec20d59c3a684eddb05
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260944
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36746}
This extends AlwaysValidPointer to take a lambda for its default
rather than requesting a constructor.
Bug: none
Change-Id: Ied97968c3f511af15422a1eef9801d14d4ec5b96
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260580
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36745}
Also eliminate FillBitrateInfo from the Channel object.
Bug: webrtc:13931
Change-Id: I5265b7629413a1ed04898272adf26708e2ee9b8d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260469
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36744}
This queue is a more strict round robing queue, unlike the class
named RoundRobinPacketQueue. That is, we don't have the same logic to
prioritize lower-bitrate streams.
The queue time mechanism is essentially directly copied from the
previous implementation however.
Bug: webrtc:11340
Change-Id: Ie38ba8ce27c985f5f1e907cec068d6a365089bcc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260562
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36737}
This reverts commit 942cac2e9e6a205fd673dc003a051cfb3867f2e3.
Reason for revert: Reverting while downstream updates are made.
Original change's description:
> Make deletion of Connection objects more deterministic.
>
> This changes most deletion paths of Connection objects to go through
> the owner class of the Connection instances, Port.
>
> In situations where Connection objects still need to be deleted
> asynchronously, `async = true` can be passed to
> `Port::DestroyConnection` and get the same behavior as
> `Connection::Destroy` formerly gave.
>
> The `Destroy()` method still exists for downstream compatibility, but
> instead of deleting connection objects asynchronously, the deletion
> now happens synchronously via the Port class.
>
> Bug: webrtc:13892, webrtc:13865
> Change-Id: I07edb7bb5e5d93b33542581b4b09def548de9e12
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259826
> Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#36676}
Bug: webrtc:13892, webrtc:13865
Change-Id: I37a15692c8201716402ba5c10f249e4d3754ce4c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260862
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36736}
Prior to this CL, rollback did not restore FiredDirection and remote
streams were only sometimes restored. This resulted in not firing
ontrack if a track was rolled back and then added again on the same
transceiver.
Rollback also never performed OnTrack, which is incorrect because a
transceiver that goes from sendrecv to inactive will cause OnRemoveTrack
and if this is rolled back (so we become sendrecv again) then we need
OnTrack to fire.
This CL improves rollback's "memory", fires ontrack in Rollback() and
adds test coverage.
Needed to solve similar bugs in the Chromium layers as well:
https://chromium-review.googlesource.com/c/chromium/src/+/3613313
Bug: chromium:1320669
Change-Id: I655dd7d8a6b86080fe0e7c32c9e8c6434062ae91
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260330
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36734}
With the encoding parameters in the SimulcastConfig objects, it wasn't
possible to configure explicit encoding parameters when using singlecast,
required for example to use the spec standard SVC API.
Bug: webrtc:11607
Change-Id: I92b1446e772e2ecec93379dc91a3da159b8bc209
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260002
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Artem Titov <titovartem@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36731}
This reduces the visibility of the implementation details
of cricket::ChannelInterface implementations.
Bug: webrtc:13931
Change-Id: Ia720a297821c1ddc242af2b04da4f52b1e04ab6b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260560
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36727}
This limits the exposure of the implementation of ChannelInterface.
Bug: webrtc:13931
Change-Id: Ifc0fa496c210413d81ad71f44fa4040b881d092c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260561
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36725}
Before this CL, fast retransmission didn't follow the SHOULDs:
https://datatracker.ietf.org/doc/html/rfc4960#section-7.2.4
* "the sender SHOULD ignore the value of cwnd (...)"
* "(...) and SHOULD NOT delay retransmission for this single
packet."
With this CL, chunks that are eligible for fast retransmission (limited
to what can fit in a single packet) will be sent just after having
received the SACK that reported them missing and transitioned the socket
into fast recovery, and they will be sent even if the congestion window
is full.
Bug: webrtc:13969
Change-Id: I12c7e191a8ffd67973db7f083bad8a6061549fa2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259866
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36724}
