The old version of these returns -1 when the value is not set.
Optional is better.
Bug: webrtc:42220231
Change-Id: Ideb0f51fd8bb7b5aa490743eb3b5d95998efbd1f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/374483
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43786}
The new type PriorityValue is a strong 16-bit integer matching RFC 8831
requirements that can be built from a Priority enum.
The value is now propagated and used by the SCTP transport, but enabling
the feature still requires a field trial for now.
Bug: webrtc:42225365
Change-Id: I56c9f48744c70999a8c2d01415a08a0b6761df4b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/357941
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42695}
After we're done sending all the messages, if the channel was in closing
state, then we start closing the association at the SCTP level, which
allows transitioning to the closed state.
Bug: chromium:40072842
Change-Id: I81b26b4137593b8feeb4bd9a2563cdfd67e1049e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/344421
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Auto-Submit: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41962}
This CL removes the send buffers (but not the receive buffer) from
SctpDataChannel and increases the send buffer in DcSctpSocket instead.
The reasons are:
1) Simplify the code. This additional buffering was strictly needed
before we migrated away from usrsctp, as that send buffer was very
limited in size (by design). But with the migration to dcSCTP, it's
no longer needed, so it just adds complexity.
2) Make `RTCDataChannel::bufferedAmount` correct. Before this CL, it
represented just the data buffered in SctpDataChannel, and not the
data accepted by the SCTP socket, but not yet put on the wire. This
makes it hard for clients to know when a message has ever been sent.
3) Better handle draining data on data channel close. While this is not
implemented in dcSCTP, having a single buffer makes this easier to
add.
While most of this CL is straightforward, the handling of bufferedAmount
in the signaling thread (in RTCDataChannel in Blink), is a bit special.
The number returned by `RTCDataChannel::bufferedAmount` is not what the
true value is inside the SCTP socket, but an eventual consistent view
of that value. When a message is sent, the value is incremented and:
- Before this change: When a message was put on the SCTP socket, the
view's value was decremented. Which made the view reflect what was
buffered outside the SCTP socket, and that buffering is now gone.
- After this change: SctpDataChannel will track what RTCDataChannel
will think it is, and provide updates to that number as we are
notified that it's reduced - by setting a "low threshold" callback
trigger.
A bonus with the new behavior is that it will be eventually consistent
and auto-heal also in error conditions - when messages are dropped due
to errors (bad input, bad state, etc). Previously, the bufferedAmount
value could drift away from the correct value on errors.
Note that a big chunk of unit tests were removed with this CL, as those
tested how the buffering behaved. Now, there is no buffering, so the
removed test cases represent a simpler interface.
This CL has been extensively tested with data channel benchmarks that
use the bufferedAmount thresholds (in Javascript).
Bug: chromium:40072842
Change-Id: I1a6a4af6b6e1116832f5028f989ce9f44683d229
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/343361
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41945}
This code was extracted to make the next following CL easier to review.
This CL simply exposes the getters, setters and callbacks to set the
buffered amount low threshold on a specific SCTP stream. It will be
used in a follow-up CL, but is just boilerplate.
Bug: chromium:40072842
Change-Id: Iccd72208b369ddc252cc5886f6446b9c2ceeb0b1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/343360
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41943}
Before this change, calling buffered_amount only included what was
buffered on top of what was already buffered in the SCTP socket. With
the defaults, the SCTP socket can buffer up to 2MB of data (that is not
put on the wire) before the additional external bufferering in
SctpDataChannel will be used. The buffering that I am working on
removing completely.
Until it's removed completely, to avoid the issue reported in
crbug.com/41221056, include the bytes buffered in the SCTP socket to
what is returned when calling RTCDataChannel::buffered_amount.
This means that when this value is zero, it can be safe to know that all
bytes have been sent, but not necessarily acknowledged. And calling
close will not discard any messages.
This is a stopgap solution, but as functional as the proper solution
that removes all additional buffering. Follow-up CLs will merely improve
this solution.
Bug: chromium:41221056
Change-Id: I06edd52188d3bf13a17827381a15a4730722685a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/342520
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41898}
Before this CL, the StreamId class represented either a valid SCTP
stream ID, or "nothing", which means that it was a wrapped
absl::optional. Since created data channels don't have a SCTP stream ID
until it's known whether this peer will use odd or even numbers, the
"nothing" value was used for that state.
This unfortunately made it a bit hard to work with objects of this type,
as one always had to check if it contained a value. And even if a caller
would check this, and then pass the StreamId to a different function,
that function would have to do the check itself (often as a RTC_DCHECK)
since the passed StreamId always could have that state.
This CL simply extracts the "absl::optional" part of it, forcing holders
to wrap it in an optional type - when it can be "nothing". But allowing
the other code to just pass StreamId that can't be "nothing". That
simplifies the code a bit, potentially removing some bugs.
Bug: chromium:41221056
Change-Id: I93104cdd5d2f5fc1dbeb9d9dfc4cf361f11a9d68
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/342440
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41880}
Instead, use BlockingCall to match with how unregistration is done.
This is needed because the ThreadWrapper implementation in Chromium, overriding the Thread implementation in WebRTC, does not order sent (blocking) tasks along with posted tasks.
That makes the functional difference that Thread1 posting and sending
tasks to Thread2, can not assume that the tasks run in the order they
were posted and sent. I.e. in this case:
// Running on Thread1.
thread2->PostTask([](){ Foo(); });
thread2->BlockingCall([](){ Bar(); });
Thread2 may actually execute Bar() first, and then Foo().
Bug: chromium:1470992
Change-Id: I1f83f12ce39c09279c0f2b3bc71c3a33e2cb16c5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/317700
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40624}
If the caller calls RegisterObserver() on the network thread while the
state is not kOpen but there are queued received data, those received
data will be immediately delivered to the observer before the state is
transitioned to kOpen, which may break the observer's assertions and
cause problems.
The problem turns out to be that, when SctpDataChannel::RegisterObserver
calls DeliverQueuedReceivedData(), the data will be passed to the
observer without checking the |state_| first, meanwhile
SctpDataChannel::UpdateState does effectively check the state and
null-check |observer_| before delivering the received data. This CL
fixes this by simply making DeliverQueuedReceivedData() also check
`state_ == kOpen`. In case the state transitions to kOpen after
RegisterObserver() is called, the first DeliverQueuedReceivedData()
call will be no-op, while the second DeliverQueuedReceivedData() call
will do the work.
Bug: chromium:1442696
Change-Id: If25ce6a038d704939b1a8ae73d7ced110448b050
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/304687
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40036}
This is especially needed for datachannels that get created in
response to an OPEN message and RegisterObserver() is called from
within the OnDataChannel callback. More details in the associated bug.
Bug: webrtc:15165
Change-Id: I833db6c3c503623d482808dc5a02f03b9821a5f6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/304721
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40032}
This reverts commit 298313534d.
Changes from the original commit:
* Call OnTransportClosed() from TeardownDataChannelTransport_n()
(same as before the original commit)
* Not call OnTransportClosed() from OnTransportChanged() when its
called with nullptr (also preserving the behaviour from before
the original commit).
Original change's description:
> Revert "Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code."
>
> This reverts commit 2ec6a6c578.
>
> Reason for revert: It breaks WPT tests (e.g. https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/1361972/overview) blocking the roll into Chromium.
>
> Original change's description:
> > Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code.
> >
> > * DCC = DataChannelController.
> >
> > * Consolidate steps to set the mid and transport name. They're now
> > set at the same time and without a separate PostTask.
> > * Transport sink is now consistently set in DCC
> > * Order of notifications for setting up the transport is now the same
> > regardless of the first time the transport is being set or if it's
> > being replaced.
> > * Made set_data_channel_transport() private.
> >
> > Bug: webrtc:11547
> > Change-Id: I39e89c6e269e6f06d55981d7944678bf23c8817a
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300562
> > Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> > Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
> > Cr-Commit-Position: refs/heads/main@{#39859}
>
> Bug: webrtc:11547
> Change-Id: I0d8d7453b71be80fbf1b7eba7d161336e29de091
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301360
> Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org>
> Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
> Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#39864}
Bug: webrtc:11547
Change-Id: I8ebbc3d3a12786dff2096350a77e03e98466ff00
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301702
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39884}
This reverts commit 2ec6a6c578.
Reason for revert: It breaks WPT tests (e.g. https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/1361972/overview) blocking the roll into Chromium.
Original change's description:
> Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code.
>
> * DCC = DataChannelController.
>
> * Consolidate steps to set the mid and transport name. They're now
> set at the same time and without a separate PostTask.
> * Transport sink is now consistently set in DCC
> * Order of notifications for setting up the transport is now the same
> regardless of the first time the transport is being set or if it's
> being replaced.
> * Made set_data_channel_transport() private.
>
> Bug: webrtc:11547
> Change-Id: I39e89c6e269e6f06d55981d7944678bf23c8817a
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300562
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#39859}
Bug: webrtc:11547
Change-Id: I0d8d7453b71be80fbf1b7eba7d161336e29de091
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301360
Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org>
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#39864}
* DCC = DataChannelController.
* Consolidate steps to set the mid and transport name. They're now
set at the same time and without a separate PostTask.
* Transport sink is now consistently set in DCC
* Order of notifications for setting up the transport is now the same
regardless of the first time the transport is being set or if it's
being replaced.
* Made set_data_channel_transport() private.
Bug: webrtc:11547
Change-Id: I39e89c6e269e6f06d55981d7944678bf23c8817a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300562
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39859}
One problem with the existing Send() method is that it has a return
value that is problematic for a fully async implementation.
A second problem with Send() is that the return value is bool and not
RTCError (webrtc:13289), which is why OnSendComplete() uses RTCError.
Also, start deprecating `bool Send()` in favor of `void SendAsync()` and
adding `network_safety_` flag for posting async operations to the
network thread. This flag also takes over from the
`connected_to_transport_` which can now be removed.
Bug: webrtc:11547, webrtc:13289
Change-Id: I87bbc7e9b964a52684bdfe0e6ebc5230be254e8b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299760
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39817}
As is, send() might return false while error() would indicate OK.
Bug: none
Change-Id: Ia303701148e86e1bcaf70cc54e689a3ff7f5a184
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300822
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39809}
This is to make sure that thread checks on the signaling thread inside
the ObserverAdapter, don't dereference the `channel_` object which
may have gone away.
(using No-try: true since the internal bots are behind)
No-try: True
Bug: webrtc:11547
Change-Id: I8f1dbf266cfc3f69fea8598a5db9baf82e4db0af
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300601
Reviewed-by: Jeremy Leconte <jleconte@google.com>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39787}
This reverts commit 7f16fcda0f.
Reason for reland: Re-landing after addressing issues in downstream
code and hardening the ObserverAdapter from situations where attempted
usage of data channel proxies could occur after shutting down the
peer connection and terminating the network thread.
Original change's description:
> Revert "[DataChannel] Send and receive packets on the network thread."
>
> This reverts commit fe53fec24e.
>
> Reason for revert: Speculative revert, may be breaking downstream project
>
> Original change's description:
> > [DataChannel] Send and receive packets on the network thread.
> >
> > This updates sctp channels, including work that happens between the
> > data channel controller and the transport, to run on the network
> > thread. Previously all network traffic related to data channels was
> > routed through the signaling thread before going to either the network
> > thread or the caller's thread (e.g. js thread in chrome). Now the
> > calls can go straight from the network thread to the JS thread with
> > enabling a special flag on the observer (see below) and similarly
> > calls to send data, involve 2 threads instead of 3.
> >
> > * Custom data channel observer adapter implementation that
> > maintains compatibility with existing observer implementations in
> > that notifications are delivered on the signaling thread.
> > The adapter can be explicitly disabled for implementations that
> > want to optimize the callback path and promise to not block the
> > network thread.
> > * Remove the signaling thread copy of data channels in the controller.
> > * Remove several PostTask operations that were needed to keep things
> > in sync (but the need has gone away).
> > * Update tests for the controller to consistently call
> > TeardownDataChannelTransport_n to match with production.
> > * Update stats collectors (current and legacy) to fetch the data
> > channel stats on the network thread where they're maintained.
> > * Remove the AsyncChannelCloseTeardown test since the async teardown
> > step has gone away.
> > * Remove `sid_s` in the channel code since we only need the network
> > state now.
> > * For the custom observer support (with and without data adapter) and
> > maintain compatibility with existing implementations, added a new
> > proxy macro that allows an implementation to selectively provide
> > its own implementation without being proxied. This is used for
> > registering/unregistering a data channel observer.
> > * Update the data channel proxy to map most methods to the network
> > thread, avoiding the interim jump to the signaling thread.
> > * Update a plethora of thread checkers from signaling to network.
> >
> > Bug: webrtc:11547
> > Change-Id: Ib4cff1482e31c46008e187189a79e967389bc518
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299142
> > Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> > Reviewed-by: Henrik Boström <hbos@webrtc.org>
> > Cr-Commit-Position: refs/heads/main@{#39760}
>
> Bug: webrtc:11547
> Change-Id: Id0d65594bf727ccea5c49093c942b09714d101ad
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300341
> Auto-Submit: Andrey Logvin <landrey@webrtc.org>
> Owners-Override: Andrey Logvin <landrey@webrtc.org>
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
> Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#39764}
Bug: webrtc:11547
Change-Id: I47dfa7e7168be0cd2faab4f8f3ebf110c3728af5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300360
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39786}
This reverts commit fe53fec24e.
Reason for revert: Speculative revert, may be breaking downstream project
Original change's description:
> [DataChannel] Send and receive packets on the network thread.
>
> This updates sctp channels, including work that happens between the
> data channel controller and the transport, to run on the network
> thread. Previously all network traffic related to data channels was
> routed through the signaling thread before going to either the network
> thread or the caller's thread (e.g. js thread in chrome). Now the
> calls can go straight from the network thread to the JS thread with
> enabling a special flag on the observer (see below) and similarly
> calls to send data, involve 2 threads instead of 3.
>
> * Custom data channel observer adapter implementation that
> maintains compatibility with existing observer implementations in
> that notifications are delivered on the signaling thread.
> The adapter can be explicitly disabled for implementations that
> want to optimize the callback path and promise to not block the
> network thread.
> * Remove the signaling thread copy of data channels in the controller.
> * Remove several PostTask operations that were needed to keep things
> in sync (but the need has gone away).
> * Update tests for the controller to consistently call
> TeardownDataChannelTransport_n to match with production.
> * Update stats collectors (current and legacy) to fetch the data
> channel stats on the network thread where they're maintained.
> * Remove the AsyncChannelCloseTeardown test since the async teardown
> step has gone away.
> * Remove `sid_s` in the channel code since we only need the network
> state now.
> * For the custom observer support (with and without data adapter) and
> maintain compatibility with existing implementations, added a new
> proxy macro that allows an implementation to selectively provide
> its own implementation without being proxied. This is used for
> registering/unregistering a data channel observer.
> * Update the data channel proxy to map most methods to the network
> thread, avoiding the interim jump to the signaling thread.
> * Update a plethora of thread checkers from signaling to network.
>
> Bug: webrtc:11547
> Change-Id: Ib4cff1482e31c46008e187189a79e967389bc518
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299142
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#39760}
Bug: webrtc:11547
Change-Id: Id0d65594bf727ccea5c49093c942b09714d101ad
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300341
Auto-Submit: Andrey Logvin <landrey@webrtc.org>
Owners-Override: Andrey Logvin <landrey@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39764}
This handles a corner case whereby an OnStateChange implementation
synchronously calls UnregisterObserver, which would (before this CL)
delete the observer adapter.
(Using No-Try since an import bot won't pass until this CL lands)
No-Try: True
Bug: webrtc:11547
Change-Id: I33a13495aad6151fdd76becfa9a2c8672d80d825
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300280
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39761}
This updates sctp channels, including work that happens between the
data channel controller and the transport, to run on the network
thread. Previously all network traffic related to data channels was
routed through the signaling thread before going to either the network
thread or the caller's thread (e.g. js thread in chrome). Now the
calls can go straight from the network thread to the JS thread with
enabling a special flag on the observer (see below) and similarly
calls to send data, involve 2 threads instead of 3.
* Custom data channel observer adapter implementation that
maintains compatibility with existing observer implementations in
that notifications are delivered on the signaling thread.
The adapter can be explicitly disabled for implementations that
want to optimize the callback path and promise to not block the
network thread.
* Remove the signaling thread copy of data channels in the controller.
* Remove several PostTask operations that were needed to keep things
in sync (but the need has gone away).
* Update tests for the controller to consistently call
TeardownDataChannelTransport_n to match with production.
* Update stats collectors (current and legacy) to fetch the data
channel stats on the network thread where they're maintained.
* Remove the AsyncChannelCloseTeardown test since the async teardown
step has gone away.
* Remove `sid_s` in the channel code since we only need the network
state now.
* For the custom observer support (with and without data adapter) and
maintain compatibility with existing implementations, added a new
proxy macro that allows an implementation to selectively provide
its own implementation without being proxied. This is used for
registering/unregistering a data channel observer.
* Update the data channel proxy to map most methods to the network
thread, avoiding the interim jump to the signaling thread.
* Update a plethora of thread checkers from signaling to network.
Bug: webrtc:11547
Change-Id: Ib4cff1482e31c46008e187189a79e967389bc518
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299142
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39760}
* Rename id_ -> id_s_, add id_n_ and thread guards.
* Same for getters, sid() -> sid_s(), add sid_n()
As more things migrate over to the network thread, we'll only need the
_n variant.
Bug: webrtc:11547
Change-Id: Ic998330f4c81b0f6833967631ac70edc2ca2301c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299141
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39724}
* Change data channel creation code to return RTCError for more
detailed/accurate errors.
* Move DataChannelController::sid_allocator_ to the network thread.
* Add a temporary duplicate vector of channels on the network thread.
This will eventually be the main vector.
* Delete one test that turns out to be racy (as long as we're using
both the signaling and network threads).
Bug: webrtc:11547, webrtc:12796
Change-Id: I93ab721a09872d075046a907df60e8aee4263371
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298624
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39719}
This flag isn't needed for sctp data channels.
Bug: none
Change-Id: I07b8ba2c5186729b8a5edb4d2bba7b800335ab5d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299074
Auto-Submit: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39701}
Make DataChannelController's AddSctpDataStream and
RemoveSctpDataStream be required to be called on the network thread.
This moves blocking calls within those methods over to the
SctpDataChannel class instead.
For production code there's no functional change in this CL. However, this CL:
1) Introduces an actual dedicated network thread to
DataChannelController and SctpDataChannel tests.
2) Removes two data_channel_transport() checks inside DCC that
were being done on the wrong thread (signaling) and
3) introduces a network calling block to SctpDataChannel, where more
network thread related work needs to be done and can be bundled.
(to be done in follow-up CLs).
Bug: webrtc:11547
Change-Id: I6787ac395e61d4a25ae3a74a123e3357cbb46b54
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298052
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39688}
This is a small tweak to explicitly remove this second construction
step from SctpDataChannel (async call to OnTransportReady) and move
it over to DataChannelController, which is where OnTransportReady()
is called from otherwise.
Bug: webrtc:11547
Change-Id: Ie86fa85cbb79b405248f88b47d5920c7f163dba6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297921
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39627}
This avoids a couple of layers of error code conversion, reduces
dependency on cricket error types and allows us to preserve error
information from dcsctp. Along the way remove SendDataResult.
Bug: none
Change-Id: I1ad18a8f0b2fb181745b19c49f36f270708720c0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298305
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39619}
These log statements may have been useful when the initial code was
being written but now it's essentially dead code except for when
debugging while working on the code (and then, enabling the log
statements is simple).
Low-Coverage-Reason: CL modifies VERBOSE log lines that aren't currently covered.
Bug: none
Change-Id: Id9a45fe53574d39ff3feba08c596e0ac4ce294fc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297760
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39604}
All tests do this already except for RTCStatsCollectorTest.
Bug: none
Change-Id: I318f45a2c79b3d07ca6c92902ebb4f0622ec3200
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297862
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39576}
This struct only contains two member variables now and there isn't
much value added by having it.
Low-Coverage-Reason: No change in coverage, CL modifies uncovered RTC_LOG lines.
Bug: none
Change-Id: I924d450f4c8f8e49b1cfeabaebee9fd5235a90cc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297360
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39563}
This includes:
* SignalDataChannelTransportWritable_s
* SignalDataChannelTransportReceivedData_s
* SignalDataChannelTransportChannelClosing_s
* Removing sigslot::has_slots<> inheritance from SctpDataChannel
Instead, we use the existing sctp_data_channels_ vector of channels
known to the DCC to deliver the callbacks.
Bug: webrtc:11943, webrtc:11547
Change-Id: I7935d7505856eedf04981b8ba665ef8419166c1d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297100
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39557}
This removes one sigslot and also simplifies the teardown procedure
of a data channel when the channel is closed by the transport.
In this case we no longer need an additional async teardown task that
releases the last remaining reference to the channel.
Bug: webrtc:11943, webrtc:11547
Change-Id: I1c170349a6cbb3cb3c5a47d284e3a3d416c92b11
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/296981
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39551}
Instead there are direct member variables for the various relevant
states, some weren't needed, some can be const but the `id` member
in particular needs special handling and can't be const.
For dealing with the stream id, we now have SctpSid. A class that does range validation, checks thread safety, handles the special `-1` case (for what's essentially an unsigned 16 bit int). Using a special type
for this also has the effect that range checking happens more
consistently (although I'm not modifying the structs in api/).
With upcoming steps of avoiding thread hops, the ID may need to
migrate to the network thread, which the thread checks will help with.
Along the way, update SctpSidAllocator to use flat_set instead of std::set and moving some of the sctp data channel code to the cc file
to help with more accurately tracking code coverage.
Bug: webrtc:11547
Change-Id: Iea6e7647ab8f93052044c5afbcc449115206b4e9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/296444
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39539}
If a data channel object was closed (via calling Close()) right after
construction and before attaching to a transport, it would never
transition to the `kClosed` state. This addresses that corner case,
which caused a DCHECK to trigger but might also cause a situation
whereby more than one DC instance existed for a given sctp sid.
Bug: chromium:1421534
Change-Id: Id757c0528f929f2e2daa5343236d7f62e309f6cc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/296341
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39513}
This moves SctpDataChannel construction a step closer to RAII by moving the error checks out of SctpDataChannel::Init() and not construct an SctpDataChannel instance unless error checks have been done first in SctpDataChannel::Create.
Ideally the Init() method shouldn't be needed but there is test code that constructs an SctpDataChannel instance without running the Init()
steps but they're required by the SctpDataChannel::Create() path.
Bug: none
Change-Id: I8498693063c28355f901d27c4fe7bd45b7d4be26
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295860
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39467}
DataChannelController used WeakPtr to clear outstanding references
upon destruction - except for the case of SctpDataChannel where we
had a pointer+flag for the same purpose. This change updates
SctpDataChannel and FakeDataChannelController to use a consistent
approach.
Bug: none
Change-Id: I0248471c241365a2c0de76afbb37302115650194
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295820
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39464}
It turns out that there were several sigslot instances across data
channel, pc and stats classes that in practice only served as means
to update two counters in RTCStatsCollector. There's already a
notification path that's suitable.
This also fixes a case where the PC instance sat in the middle
of notifications from datachannels to the datachannel controller.
Bug: webrtc:11943
Change-Id: Ic60b76021584019f82085f6651230fe2fe82d465
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295781
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39456}
BlockingCall doesn't take rtc::Location parameter and thus most of the dependencies on location can be removed
Bug: webrtc:11318
Change-Id: I91a17e342dd9a9e3e2c8f7fbe267474c98a8d0e5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/274620
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38045}
Since the lifetime of an SctpDataChannel is not strictly controlled
by its controller, the controller might go away before the channel
does. This CL guards against this.
Bug: webrtc:13931
Change-Id: I07046fe896d1a66bf89287429beb0587382a13a9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261940
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36852}
The state machine for handling resets couldn't handle resets
happening from both sides at the same time.
Bug: webrtc:13994
Change-Id: I2c268e54f4c5c9858913faef91ff00f6af956e99
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261305
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36799}
Also update API proxy Create() factory functions to accept the inner
reference counted object via scoped_refptr instead of a raw pointer.
This is to avoid accidentally creating and deleting an object when
passing an inner object to a proxy class.
Consider something like:
auto proxy = MyProxy::Create(
signaling_thread(), make_ref_counted<Foo>());
Bug: webrtc:13464, webrtc:12701
Change-Id: I55ccfff43bbc164a5e909b2c9020e306ebb09075
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/256010
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36261}
According to https://w3c.github.io/webrtc-pc/#datachannel-send it should
return an error, definitely not close the data channel.
While we should probably return an RTCError will better information, this
would break the API and will be done later.
Bug: webrtc:13289
Change-Id: I90baf012440fbe2a38a826cf50b50b2b668fd7ff
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/237180
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35306}
