New issue
Advanced search Search tips

Issue 866447 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Sep 4
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-08-08
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug



Sign in to add a comment

chrome://webrtc-internals tracking RTCRtpTransceiver

Project Member Reported by hbos@chromium.org, Jul 23

Issue description

When 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
 
I 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' }"
Oh, and transceiver's mid of course "{ mid:'foo', ... }"
I'll see if it can be written out over multiple lines to make it more readable.
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.
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.

Comment 6 Deleted

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
Cc: hbos@chromium.org
 Issue 789935  has been merged into this issue.
 Issue 864912  has been merged into this issue.
Labels: M-70
Current next milestone is M-70, but hoping to be able to merge fix for M-69.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Labels: Merge-Request-69 OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
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.
Project Member

Comment 13 by sheriffbot@chromium.org, Aug 7

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
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
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. 
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.
NextAction: 2018-08-08
Thank you, Pls update the bug with canary result tomorrow.
Cc: guidou@chromium.org
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.
The NextAction date has arrived: 2018-08-08
Labels: -Merge-Review-69 Merge-Approved-69
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.
Project Member

Comment 20 by bugdroid1@chromium.org, Aug 8

Labels: -merge-approved-69 merge-merged-3497
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

Labels: Merge-Merged-69
Is this finished? If not, please bump to M71.
I have a work in progress CL, if I finish it soon there should be no problem merging it into M70.
Or let me think about that, I'll update soon.
Labels: -M-69
Status: Verified (was: Started)
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