webrtc: ensure tracks are muted initially and unmute when a packet arrives [chromium/src : main]
Philipp Hancke (Gerrit)
Philipp Hancke added 1 comment![]( "Open in Gerrit")
ready for review. The threading bits are... confusing but it seems to work?
Related details
- Guido Urdaneta
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 3 comments![]( "Open in Gerrit")
RTC_DCHECK(webrtc_receiver_);use CHECK, put behind flag if necessary
webrtc_receiver_->SetObserver(this);Put this behind the flag
Related details
- Philipp Hancke
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philipp Hancke (Gerrit)
Philipp Hancke added 3 comments![]( "Open in Gerrit")
RTC_DCHECK(webrtc_receiver_);use CHECK, put behind flag if necessary
why can I even use the webrtc DCHECK... (I assume it spills over somewhere?)
webrtc_receiver_->SetObserver(this);Put this behind the flag
not necessary since the track will be unmuted by peerconnection in the old behavior and then swallow the duplicate event but... better safe than sorry!
if (webrtc_receiver_) {Put this behind the flag
- Guido Urdaneta
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 2 comments![]( "Open in Gerrit")
webrtc_receiver_->SetObserver(nullptr);Is it OK to call this twice on two different threads?
If you call `OnFirstPacketReceived` outside the main thread it will be called first on non-main, then you post to call again on main and call it again.
crbug.com/369652659 [ Mac15 ] external/wpt/webrtc/RTCPeerConnection-getStats.https.html [ Skip Timeout ]Why is this one timing out? we have a lot of things timing out on Mac (apparently mdns issues in the bots in some cases), is it the same here?
Related details
- Philipp Hancke
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philipp Hancke (Gerrit)
Philipp Hancke added 2 comments![]( "Open in Gerrit")
webrtc_receiver_->SetObserver(nullptr);Is it OK to call this twice on two different threads?
If you call `OnFirstPacketReceived` outside the main thread it will be called first on non-main, then you post to call again on main and call it again.
This is just nulling the observer, calling it twice is ok (and happens unconditionally in the destructor).
The call originates from WebRTCs network thread:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/rtp_transceiver.cc;l=315?q=OnFirstPacketReceived%20webrtc
Lets ask Tommi whether that is a good thing?
crbug.com/369652659 [ Mac15 ] external/wpt/webrtc/RTCPeerConnection-getStats.https.html [ Skip Timeout ]Why is this one timing out? we have a lot of things timing out on Mac (apparently mdns issues in the bots in some cases), is it the same here?
yeah, you can see
[0610/234438.539335:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1278] Failed to resolve ICE candidate hostname afd366c2-561b-4677-8693-d2c37f69c5bb.local with error -1
[0610/234438.539387:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1278] Failed to resolve ICE candidate hostname afd366c2-561b-4677-8693-d2c37f69c5bb.local with error -1
in the failure logs. There is an odd
[0610/234435.393999:WARNING:third_party/webrtc/pc/peer_connection.cc:2682] 0 is not ready to use the remote candidate because the local or remote description is not set.
though...
Related details
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta voted and added 2 comments![]( "Open in Gerrit")
Votes added by Guido Urdaneta
| Code-Review | +1 |
2 comments
webrtc_receiver_->SetObserver(nullptr);Philipp HanckeIs it OK to call this twice on two different threads?
If you call `OnFirstPacketReceived` outside the main thread it will be called first on non-main, then you post to call again on main and call it again.
This is just nulling the observer, calling it twice is ok (and happens unconditionally in the destructor).
The call originates from WebRTCs network thread:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/rtp_transceiver.cc;l=315?q=OnFirstPacketReceived%20webrtc
Lets ask Tommi whether that is a good thing?
sgtm.
Also mention in the comment that nulling twice is ok.
crbug.com/369652659 [ Mac15 ] external/wpt/webrtc/RTCPeerConnection-getStats.https.html [ Skip Timeout ]Philipp HanckeWhy is this one timing out? we have a lot of things timing out on Mac (apparently mdns issues in the bots in some cases), is it the same here?
yeah, you can see
[0610/234438.539335:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1278] Failed to resolve ICE candidate hostname afd366c2-561b-4677-8693-d2c37f69c5bb.local with error -1
[0610/234438.539387:WARNING:third_party/webrtc/p2p/base/p2p_transport_channel.cc:1278] Failed to resolve ICE candidate hostname afd366c2-561b-4677-8693-d2c37f69c5bb.local with error -1
in the failure logs. There is an odd
[0610/234435.393999:WARNING:third_party/webrtc/pc/peer_connection.cc:2682] 0 is not ready to use the remote candidate because the local or remote description is not set.
though...
- Philipp Hancke
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philipp Hancke (Gerrit)
Philipp Hancke added 1 comment![]( "Open in Gerrit")
webrtc_receiver_->SetObserver(nullptr);Philipp HanckeIs it OK to call this twice on two different threads?
If you call `OnFirstPacketReceived` outside the main thread it will be called first on non-main, then you post to call again on main and call it again.
Guido UrdanetaThis is just nulling the observer, calling it twice is ok (and happens unconditionally in the destructor).
The call originates from WebRTCs network thread:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/rtp_transceiver.cc;l=315?q=OnFirstPacketReceived%20webrtc
Lets ask Tommi whether that is a good thing?
sgtm.
Also mention in the comment that nulling twice is ok.
Done. It gets a bit more interesting though, this gets called from the network thread but https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/channel_interface.h;l=79;drc=46e0a7fb3fd7c4951efd982ae2caf138e20d2a95;bpv=1;bpt=1
says the callback gets deleted after being called so unregistering is not necessary -- won't hurt either.
Related details
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomas Gunnarsson (Gerrit)
Tomas Gunnarsson added 1 comment![]( "Open in Gerrit")
webrtc_receiver_->SetObserver(nullptr);Philipp HanckeIs it OK to call this twice on two different threads?
If you call `OnFirstPacketReceived` outside the main thread it will be called first on non-main, then you post to call again on main and call it again.
Guido UrdanetaThis is just nulling the observer, calling it twice is ok (and happens unconditionally in the destructor).
The call originates from WebRTCs network thread:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/rtp_transceiver.cc;l=315?q=OnFirstPacketReceived%20webrtc
Lets ask Tommi whether that is a good thing?
Philipp Hanckesgtm.
Also mention in the comment that nulling twice is ok.
Done. It gets a bit more interesting though, this gets called from the network thread but https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/channel_interface.h;l=79;drc=46e0a7fb3fd7c4951efd982ae2caf138e20d2a95;bpv=1;bpt=1
says the callback gets deleted after being called so unregistering is not necessary -- won't hurt either.
`OnFirstPacketReceived` is always called from the signaling thread, not the network thread. The `SetFirstPacketReceivedCallback` is what's used inside webrtc, not what issues the call to OnFirstPacketReceived.
```
context()->network_thread()->BlockingCall([&]() {
<< Network thread >>
channel_->SetRtpTransport(transport_lookup(channel_->mid()));
channel_->SetFirstPacketReceivedCallback(
[thread = thread_, flag = signaling_thread_safety_, this]() mutable {
<< Network thread >>
thread->PostTask(
SafeTask(std::move(flag), [this]() {
<< Signaling thread >>
OnFirstPacketReceived();
}));
});
channel_->SetFirstPacketSentCallback(
[thread = thread_, flag = signaling_thread_safety_, this]() mutable {
thread->PostTask(
SafeTask(std::move(flag), [this]() { OnFirstPacketSent(); }));
});
});
```
Related details
- Guido Urdaneta
- Philipp Hancke
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philipp Hancke (Gerrit)
Philipp Hancke added 1 comment![]( "Open in Gerrit")
Thank you Tommi!
Removed the unregister there since it should not be necessary as
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/channel.h;drc=46e0a7fb3fd7c4951efd982ae2caf138e20d2a95;l=319
says the function is deleted (i.e. can only fire once; shouldn't we have a more general pattern for that?)
Related details
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta voted Code-Review+1![]( "Open in Gerrit")
- Philipp Hancke
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harald Alvestrand (Gerrit)
Harald Alvestrand voted and added 2 comments![]( "Open in Gerrit")
Votes added by Harald Alvestrand
| Code-Review | +1 |
2 comments
Hoping this sticks... it's been a long time nonconformance thing.
In Chromium, we tend to use OnceCallback and RepeatingCallback for the two kinds.
We don't make that distinction in WebRTC.
Related details
- Philipp Hancke
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philipp Hancke (Gerrit)
Philipp Hancke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Philipp Hancke
| Commit-Queue | +2 |
2 comments
lets try!
webrtc_receiver_->SetObserver(nullptr);I probably saw the Chromium concept. We might adopt it in WebRTC ;-)
Related details
- Tomas Gunnarsson
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
webrtc: ensure tracks are muted initially and unmute when a packet arrives
fixing a long-standing bug wherein the track was muted in ontrack but unumuted immediately after setRemoteDescription.
Also fix tests and split up the unmute test to make it easier to understand.
- M third_party/blink/renderer/modules/peerconnection/peer_connection_features.cc
- M third_party/blink/renderer/modules/peerconnection/peer_connection_features.h
- M third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc
- M third_party/blink/renderer/modules/peerconnection/rtc_rtp_receiver_impl.cc
- M third_party/blink/web_tests/TestExpectations
- D third_party/blink/web_tests/external/wpt/webrtc-stats/outbound-rtp.https-expected.txt
- M third_party/blink/web_tests/external/wpt/webrtc-stats/outbound-rtp.https.html
- M third_party/blink/web_tests/external/wpt/webrtc/RTCRtpTransceiver.https-expected.txt
- M third_party/blink/web_tests/external/wpt/webrtc/RTCRtpTransceiver.https.html
Code-Review: +1 by Guido Urdaneta, +1 by Harald Alvestrand
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The exported PR, https://github.com/web-platform-tests/wpt/pull/53146, has failed the following check(s) on GitHub:
wpt-firefox-nightly-stability (https://github.com/web-platform-tests/wpt/runs/44091650125)
These failures will block the export. They may represent new or existing problems; please take a look at the output and see if it can be fixed. Unresolved failures will be looked at by the Ecosystem-Infra sheriff after this CL has been landed in Chromium; if you need earlier help please contact blin...@chromium.org.
Any suggestions to improve this service are welcome; crbug.com/1027618.
Gerrit CL SHA: Latest
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
luci-bi...@appspot.gserviceaccount.com has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |