chrome://webrtc-internals tracking RTCRtpTransceiver |
||||||||||
Issue descriptionWhen adding/modifying/removing RTCRtpTransceivers as a result of API calls, e.g: - RTCPeerConnection.addTrack() (and addStream()) - RTCPeerConnection.removeTrack() (and removeStream()) - RTCPeerConnection.addTransceiver() - RTCPeerConnection.setLocalDescription() - RTCPeerConnection.setRemoteDescription() This should show up in chrome://webrtc-internals in a way that makes sense. It used to be addStream/removeStream/setRemoteDescription. But RTCRtpTransceiver and Unified Plan makes the picture a little more complicated. As of filing this bug, the events shows up as "transceiverAdded/Modified/Removed (addTrack/removeTrack/setRemoteDescription): <serialized view of the transceiver>" but it's only working properly in Plan B. It would be nice to know... 1. Which API caused the modification? E.g. "addTrack" 2. Which transceiver was modified? E.g. "getTransceivers()[0]" 3. What is the new state of the transceiver? Stuff that are likely to change: - transceiver.currentDirection - transceiver.sender.track
,
Jul 23
Oh, and transceiver's mid of course "{ mid:'foo', ... }"
,
Jul 23
I'll see if it can be written out over multiple lines to make it more readable.
,
Jul 23
traditionally webrtc-internals has been doing an API trace -- e.g. that addStream was called and with what arguments. You can understand what happens by the timing of these. See https://testrtc.com/webrtc-api-trace/ For events like icegatheringstatechange it shows the icegatheringstate. transceiverAdded/Modified/Removed seems to fit the latter more -- you want so expose that a transceiver changed state. This is because addTrack was called but that should not be part of the event -- this is visible from the timeline. So i'd suggest re-adding addStream/addTrack and having transceiverAdded/Modified/Removed as event name with a serialized transceiver. The serialization should show the index in getTransceivers() as part of the event as well as the mid -- which is potentially unset before SLD.
,
Jul 23
transceiverAdded/Modified/Removed does not correlate to any event though. Even the existing events like "ontrack" only fire for some of the transceivers, it does not give a complete picture of what happened. The only API that correspond to the transceiver modification is SLD or SRD. Wouldn't "setLocal/RemoteDescription(offer/answer)" containing a complete list of modified transcievers if you click the arrow be more useful than an imaginary event name for each transceiver that was updated? It would be hard to keep track of which call to SLD or which call to SRD the change correspond to if it was an "transceiverModified" event. Typically people would do both SRD(offer) and SRD(answer) in quick succession.
,
Jul 23
addStream/onAddStream and removeStream/onRemoveStream no longer makes sense and have been removed. But we should add them back as addTrack and removeTrack. onAddStream should be replaced by ontrack, and onRemoveStream should either be replaced by track.onmute or covered by other transceiver modification events. The state of a transceiver will be shown similar to this: [1]. With transceiverAdded/Modified/Removed events that do necessarily correspond to an API call, but in the case of asynchronous operations don't necessarily correspond to an event firing in the spec (e.g. ontrack doesn't fire in setLocalDescription, but SLD can be used to modify the currentDirection of a transceiver). This bug is for those events, we should file a separate bug for addTrack/removeTrack/ontrack/track.onmute. [1] https://photos.app.goo.gl/dBwUxUAjeDLd9KpGA
,
Jul 31
,
Jul 31
Issue 864912 has been merged into this issue.
,
Jul 31
Current next milestone is M-70, but hoping to be able to merge fix for M-69.
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9 commit 991b3057a19fa59f5b2606dd7ccc37fa268f8ce9 Author: Henrik Boström <hbos@chromium.org> Date: Tue Aug 07 19:50:53 2018 PeerConnectionTracker: Improved events for sender/receiver/transceiver. This implements the "transceiverAdded" and "transceiverModified" events that show up in chrome://webrtc-internals/, as described under "Option C: API Events and Transceiver Added/Modified Events" of design doc https://docs.google.com/document/d/1rsJSvHulqdbc3kfs70-PvRFrCYYuNP3W1t6XAQRLiEg/edit?usp=sharing. That is when transceivers (Unified Plan) is used. If Plan B is used, the events shows up as "senderAdded", "senderModified", "senderRemoved", "receiverAdded", "receiverModified" or "receiverRemoved" instead. This CL does not add the API Events for "addTrack", "removeTrack", "ontrack", "onmute", etc. That will require a follow-up CL. However, the transceiver events does include information about what API caused the transceiver to be modified. Here is an example of how the events show up in chrome://webrtc-internals/ when performing a call[1]: - In Plan B: https://photos.app.goo.gl/SMukRLW6KuHPZVC78 https://photos.app.goo.gl/CV2jK8n4y1VrSiqR8 - In Unified Plan: https://photos.app.goo.gl/zbsjfizQSizwdPwA8 https://photos.app.goo.gl/CwCibJ19vRR9P7gY7 https://photos.app.goo.gl/MWWDZEdZvyiXT4o49 This CL also performs the following related changes: 1. RTCPeerConnectionHandler::rtp_receivers_ changes type from a map to a vector so that each receiver has an index corresponding to its insertion order. 2. RTCPeerConnection::GetTransceiverIndex() obtains the index of the transceiver (or sender or receiver if Plan B). 3. RTCRtpSenderOnlyTransceiver and RTCRtpReceiverOnlyTransceiver is updated to use template<> as to allow these helper classes inside of peer_connection_tracker_unittest.cc. 4. peer_connection_tracker_unittest.cc is refactored to minimize the amount of duplicate code to set up tests. Various fake classes and helpers are added that should probably be moved into its own file in the future (https://crbug.com/868868) but I don't want to mess around with BUILD files to make merging easier. 5. PeerConnectionTrackerTest is lacking a lot of test coverage. This CL adds extensive test coverage for all the Track*Transceiver() methods. [1] https://codepen.io/anon/pen/OwpYvw?editors=0010 This sets up two RTCPeerConnections and performs two offer/answer cycles. In the first, two tracks are added to both PCs. In the second, one track is removed from each PC. Bug: 866447 Change-Id: I33df774e8b2b22df6890a90a80e7d83e0aec0524 Reviewed-on: https://chromium-review.googlesource.com/1154975 Commit-Queue: Henrik Boström <hbos@chromium.org> Reviewed-by: Harald Alvestrand <hta@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Cr-Commit-Position: refs/heads/master@{#581314} [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/BUILD.gn [add] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/fake_rtc_rtp_transceiver.cc [add] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/fake_rtc_rtp_transceiver.h [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/peer_connection_tracker.cc [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/peer_connection_tracker.h [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/peer_connection_tracker_unittest.cc [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/rtc_peer_connection_handler.cc [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/rtc_peer_connection_handler.h [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/rtc_rtp_receiver.cc [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/rtc_rtp_receiver.h [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/rtc_rtp_receiver_unittest.cc [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/rtc_rtp_sender.cc [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/content/renderer/media/webrtc/rtc_rtp_sender.h [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/third_party/blink/public/platform/web_rtc_rtp_receiver.h [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/third_party/blink/public/platform/web_rtc_rtp_sender.h [modify] https://crrev.com/991b3057a19fa59f5b2606dd7ccc37fa268f8ce9/third_party/blink/renderer/platform/testing/testing_platform_support_with_web_rtc.cc
,
Aug 7
Requesting 991b3057a19fa59f5b2606dd7ccc37fa268f8ce9 (comment #11) be merged into M69. It updates the internal chrome page chrome://webrtc-internals/ to better reflect the new APIs shipped in M69 and would be useful for developers to have access to in Dev and Stable alongside the new APIs that already made the cut.
,
Aug 7
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 7
The change is not yet baked in canary, landed 111 mins back and it seems to be a big change. How safe and critical is to merge this change to M69? Pls note for M69 we're only taking fully safe and critical merges in at this stage.
,
Aug 7
Most of the size of the CL is comprised of unittest code and helper classes used by the tests. The essential changes are the peer_connection_tracker.[h/cc] changes (+160-77) that turns some attributes into strings, and updating calling places of the tracker. These code paths are heavily exercised by the majority of WebRTC tests. The rest of the CL, while modifying many files, are mostly simple refactoring that should be harmless. I deem the CL to be both safe and important to have in M69, despite the late landing.
,
Aug 7
Thank you, Pls update the bug with canary result tomorrow.
,
Aug 8
I verified in Chrome Canary 70.0.3516.0 on Windows that this works as expected, and +guidou@ verified that merging this to M69 would be clean. Requesting the merge to be approved so that we can merge before today's Beta cut.
,
Aug 8
The NextAction date has arrived: 2018-08-08
,
Aug 8
Approving merge to M69 branch 3497 based on comment #15 & #17. Please merge ASAP so we can pick it up for tomorrow's Beta release. Thank you.
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5f12e79cf04a3362242957c0e71d50baead6d55a commit 5f12e79cf04a3362242957c0e71d50baead6d55a Author: Henrik Boström <hbos@chromium.org> Date: Wed Aug 08 15:08:31 2018 PeerConnectionTracker: Improved events for sender/receiver/transceiver. This implements the "transceiverAdded" and "transceiverModified" events that show up in chrome://webrtc-internals/, as described under "Option C: API Events and Transceiver Added/Modified Events" of design doc https://docs.google.com/document/d/1rsJSvHulqdbc3kfs70-PvRFrCYYuNP3W1t6XAQRLiEg/edit?usp=sharing. That is when transceivers (Unified Plan) is used. If Plan B is used, the events shows up as "senderAdded", "senderModified", "senderRemoved", "receiverAdded", "receiverModified" or "receiverRemoved" instead. This CL does not add the API Events for "addTrack", "removeTrack", "ontrack", "onmute", etc. That will require a follow-up CL. However, the transceiver events does include information about what API caused the transceiver to be modified. Here is an example of how the events show up in chrome://webrtc-internals/ when performing a call[1]: - In Plan B: https://photos.app.goo.gl/SMukRLW6KuHPZVC78 https://photos.app.goo.gl/CV2jK8n4y1VrSiqR8 - In Unified Plan: https://photos.app.goo.gl/zbsjfizQSizwdPwA8 https://photos.app.goo.gl/CwCibJ19vRR9P7gY7 https://photos.app.goo.gl/MWWDZEdZvyiXT4o49 This CL also performs the following related changes: 1. RTCPeerConnectionHandler::rtp_receivers_ changes type from a map to a vector so that each receiver has an index corresponding to its insertion order. 2. RTCPeerConnection::GetTransceiverIndex() obtains the index of the transceiver (or sender or receiver if Plan B). 3. RTCRtpSenderOnlyTransceiver and RTCRtpReceiverOnlyTransceiver is updated to use template<> as to allow these helper classes inside of peer_connection_tracker_unittest.cc. 4. peer_connection_tracker_unittest.cc is refactored to minimize the amount of duplicate code to set up tests. Various fake classes and helpers are added that should probably be moved into its own file in the future (https://crbug.com/868868) but I don't want to mess around with BUILD files to make merging easier. 5. PeerConnectionTrackerTest is lacking a lot of test coverage. This CL adds extensive test coverage for all the Track*Transceiver() methods. [1] https://codepen.io/anon/pen/OwpYvw?editors=0010 This sets up two RTCPeerConnections and performs two offer/answer cycles. In the first, two tracks are added to both PCs. In the second, one track is removed from each PC. Bug: 866447 Change-Id: I33df774e8b2b22df6890a90a80e7d83e0aec0524 Reviewed-on: https://chromium-review.googlesource.com/1154975 Commit-Queue: Henrik Boström <hbos@chromium.org> Reviewed-by: Harald Alvestrand <hta@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#581314}(cherry picked from commit 991b3057a19fa59f5b2606dd7ccc37fa268f8ce9) Reviewed-on: https://chromium-review.googlesource.com/1166962 Reviewed-by: Henrik Boström <hbos@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#495} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/BUILD.gn [add] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/fake_rtc_rtp_transceiver.cc [add] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/fake_rtc_rtp_transceiver.h [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/peer_connection_tracker.cc [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/peer_connection_tracker.h [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/peer_connection_tracker_unittest.cc [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/rtc_peer_connection_handler.cc [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/rtc_peer_connection_handler.h [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/rtc_rtp_receiver.cc [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/rtc_rtp_receiver.h [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/rtc_rtp_receiver_unittest.cc [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/rtc_rtp_sender.cc [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/content/renderer/media/webrtc/rtc_rtp_sender.h [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/third_party/blink/public/platform/web_rtc_rtp_receiver.h [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/third_party/blink/public/platform/web_rtc_rtp_sender.h [modify] https://crrev.com/5f12e79cf04a3362242957c0e71d50baead6d55a/third_party/blink/renderer/platform/testing/testing_platform_support_with_web_rtc.cc
,
Aug 8
,
Aug 30
Is this finished? If not, please bump to M71.
,
Aug 30
I have a work in progress CL, if I finish it soon there should be no problem merging it into M70.
,
Aug 30
Or let me think about that, I'll update soon.
,
Sep 4
Actually let me split this bug up so that the remaining work is tracked separately, this is doing two things and having multiple milestones gets confusing. This bug was fixed in M-70 and merged into M-69 and refers to tracking transceivers, as the title states. For the remainder of the work I put M-71: https://crbug.com/880254 This refers to adding more event types, tracking the API calls themselves not just the transceiver addition/removal that are the effects of this API calls. Closing this bug. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by hbos@chromium.org
, Jul 23I suggest: "addTrack: [new] getTransceivers()[0]: { sender:{track:'X'}, receiver:{track:'Y'}, direction:'sendrecv', currentDirection:null }" ... "setRemoteDescription(answer): [modified] getTransceivers()[0]: { sender:{track:'X'}, receiver:{track:'Y'}, direction:'sendrecv', currentDirection:'sendonly' }"