Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2509803004:
RTCCodecStats added (Closed)
DescriptionRTCCodecStats[1] added.
RTCStatsCollector supports "payloadType", "codec" and "clockRate".
"channels", "parameters" and "implementation" need to be supported
before closing crbug.com/659117.
[1] https://w3c.github.io/webrtc-stats/#codec-dict*
BUG=chromium:659117, chromium:627816, chromium:657854
NOTRY=True
Committed: https://crrev.com/0adb8285b101ee7e8a8e1b03200018449f606d6a
Cr-Commit-Position: refs/heads/master@{#15207}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Rebase with master #Patch Set 3 : Addressed comments #
Messages
Total messages: 47 (27 generated)
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== RTCCodecStats[1] added. RTCStatsCollector supports "payloadType", "codec" and "clockRate". "channels", "parameters" and "implementation" need to be supported before closing crbug.com/659117. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117, chromium:627816, chromium:657854 ========== to ========== RTCCodecStats[1] added. RTCStatsCollector supports "payloadType", "codec" and "clockRate". "channels", "parameters" and "implementation" need to be supported before closing crbug.com/659117. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117, chromium:627816, chromium:657854 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12611) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20197) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/13212)
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #1 (id:20001) has been deleted
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look, hta and deadbeef. A different approach to collecting RTCCodecStats from the previous CL now that other changes have been made.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think there's still a minor issue around the codec identification (sorry...). Otherwise this looks good. P.S.: The codec identification problem is actually impossible to run into with our current implementation. But it will be possible once we implement Unified Plan SDP, which hopefully will be soon. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:36: std::string RTCCodecStatsIDFromDirectionTypeAndPayload( nit: I think you meant "DirectionAndPayloadType"? https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:39: return audio ? "RTCCodec_InboundAudio_" + rtc::ToString<>(payload_type) This is close, but what's really needed is something to differentiate the transport being used, not "audio vs video". For example, if you're not bundling, this is possible: m=video 9 UDP/TLS/RTP/SAVPF 100 a=mid:VideoTrack1 a=rtpmap:100 CodecA ... m=video:9 UDP/TLS/RTP/SAVPF 100 a=mid:VideoTrack2 a=rtpmap:100 CodecB The same payload type refers to different codecs, which is possible if the tracks are using different transports and thus different RTP sessions. When I say "something to differentiate the transport being used", the transport ID in "proxy_to_transport" should be good. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:823: nullptr, "VideoContentName", false); To test the scenario I described where a payload type may be mapped to two different things (for different RTP sessions), you could create another mock video channel that uses a different transport.
This raised questions. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:39: return audio ? "RTCCodec_InboundAudio_" + rtc::ToString<>(payload_type) On 2016/11/17 22:01:51, Taylor Brandstetter wrote: > This is close, but what's really needed is something to differentiate the > transport being used, not "audio vs video". For example, if you're not bundling, > this is possible: > > m=video 9 UDP/TLS/RTP/SAVPF 100 > a=mid:VideoTrack1 > a=rtpmap:100 CodecA > ... > m=video:9 UDP/TLS/RTP/SAVPF 100 > a=mid:VideoTrack2 > a=rtpmap:100 CodecB > > The same payload type refers to different codecs, which is possible if the > tracks are using different transports and thus different RTP sessions. > > When I say "something to differentiate the transport being used", the transport > ID in "proxy_to_transport" should be good. Hmm. I do pc_->session()->voice_channel()->GetStats(...) and pc_->session()->video_channel()->GetStats(...). The session() has different transports, and e.g. GetTransportStats performs transport_controller_->GetStats with the voice, video and data channels' transport names so each channel can use a different transport (but this gives other stats than codecs). But in any given time we only have one voice, one video and one data channel in this context (std::unique_ptr)? I can set up voice to use one transport and video to use another, but I'm already distinguishing between audio and video. Are there other channels equivalent of session()->blah_channel() that I'm ignoring right now? For me to test this scenario I need to be able to add multiple Mock[Voice/Video]Channels but I can only mock the session to return a single voice/video channel. I could make the codec ID be based on transport name instead of "Audio" or "Video", that might be more correct, but both are today unique unless I'm mistaken. Does this mean that there are advertised codecs that these stats ignore because they're not part of the currently selected channels? Should codecs stats come from transport stats instead? Or is it correct to only return codecs stats that are on existing channels (disregarding things mentioned in the SDP)? Oh and there are also data channels, are there data codecs (that are not audio or video) that should be included?
On 2016/11/18 09:08:13, hbos wrote: > This raised questions. > > https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... > File webrtc/api/rtcstatscollector.cc (right): > > https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... > webrtc/api/rtcstatscollector.cc:39: return audio ? "RTCCodec_InboundAudio_" + > rtc::ToString<>(payload_type) > On 2016/11/17 22:01:51, Taylor Brandstetter wrote: > > This is close, but what's really needed is something to differentiate the > > transport being used, not "audio vs video". For example, if you're not > bundling, > > this is possible: > > > > m=video 9 UDP/TLS/RTP/SAVPF 100 > > a=mid:VideoTrack1 > > a=rtpmap:100 CodecA > > ... > > m=video:9 UDP/TLS/RTP/SAVPF 100 > > a=mid:VideoTrack2 > > a=rtpmap:100 CodecB > > > > The same payload type refers to different codecs, which is possible if the > > tracks are using different transports and thus different RTP sessions. > > > > When I say "something to differentiate the transport being used", the > transport > > ID in "proxy_to_transport" should be good. > > Hmm. I do pc_->session()->voice_channel()->GetStats(...) and > pc_->session()->video_channel()->GetStats(...). > > The session() has different transports, and e.g. GetTransportStats performs > transport_controller_->GetStats with the voice, video and data channels' > transport names so each channel can use a different transport (but this gives > other stats than codecs). > > But in any given time we only have one voice, one video and one data channel in > this context (std::unique_ptr)? I can set up voice to use one transport and > video to use another, but I'm already distinguishing between audio and video. > Are there other channels equivalent of session()->blah_channel() that I'm > ignoring right now? > > For me to test this scenario I need to be able to add multiple > Mock[Voice/Video]Channels but I can only mock the session to return a single > voice/video channel. Can you mock the session to return multiple video tracks inside the channel? I think the word "channel" refers to a concept that isn't described in the standards. > > I could make the codec ID be based on transport name instead of "Audio" or > "Video", that might be more correct, but both are today unique unless I'm > mistaken. As long as we're generating "plan B" SDP, it's impossible to create the scenario Taylor described (because all the video channels are on a single m-line), but when we do Unified Plan, we'll be able to get into this scenario. I'm sure something will break when we do, but we should make sure stats don't. > > Does this mean that there are advertised codecs that these stats ignore because > they're not part of the currently selected channels? Should codecs stats come > from transport stats instead? Or is it correct to only return codecs stats that > are on existing channels (disregarding things mentioned in the SDP)? Probably. You might want to check if the CN codecs are listed on audio, in order to see if you have them all or not. > > > > Oh and there are also data channels, are there data codecs (that are not audio > or video) that should be included? Datachannels don't have codecs (happily). The use of the term "channel" in cricket and in "datachannel" is a coincidence. (The old RTP-based datachannels had payload types. But we don't care about them.)
On 2016/11/18 09:27:48, hta-webrtc wrote: > On 2016/11/18 09:08:13, hbos wrote: > > For me to test this scenario I need to be able to add multiple > > Mock[Voice/Video]Channels but I can only mock the session to return a single > > voice/video channel. > > Can you mock the session to return multiple video tracks inside the channel? > I think the word "channel" refers to a concept that isn't described in the > standards. I think I could. But I'm not sure how to get codec information from tracks, and even if I did, that would only include codecs that are (or in the case of retaining detached tracks' stats - ever have been) in use by tracks. Not all codecs in the SDP. > > Does this mean that there are advertised codecs that these stats ignore > because > > they're not part of the currently selected channels? Should codecs stats come > > from transport stats instead? Or is it correct to only return codecs stats > that > > are on existing channels (disregarding things mentioned in the SDP)? > > Probably. You might want to check if the CN codecs are listed on audio, in order > to see if you have them all or not. CN codecs? Listed on audio? I don't find what I'm looking for exploring transport-related classes. deadbeef@ do you know where all codec information might be located?
hbos@webrtc.org changed reviewers: + magjed@webrtc.org
+magjed: Do you know more about where to find information about available codecs and payload types that could be used for stat scollection? (See discussion in this thread)
Sorry for the confusion. I guess there's really no way to test the scenario I'm describing right now. We just have to be aware that this code will need to change slightly once we go from "1" to "N" voice/video channels. With that in mind, lgtm. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:39: return audio ? "RTCCodec_InboundAudio_" + rtc::ToString<>(payload_type) On 2016/11/18 09:08:13, hbos wrote: > But in any given time we only have one voice, one video and one data channel in > this context (std::unique_ptr)? I can set up voice to use one transport and > video to use another, but I'm already distinguishing between audio and video. > Are there other channels equivalent of session()->blah_channel() that I'm > ignoring right now? No, you're right. This is what I was talking about when I said "this issue is impossible to run into until we implement 'Unified Plan' SDP". I guess there's no way to test it right now, either. > Does this mean that there are advertised codecs that these stats ignore because > they're not part of the currently selected channels? Should codecs stats come > from transport stats instead? Or is it correct to only return codecs stats that > are on existing channels (disregarding things mentioned in the SDP)? No, what you have now should be good. A "BaseChannel" is roughly equivalent to an m= section in SDP, which is roughly equivalent to an RtpTransceiver. > Oh and there are also data channels, are there data codecs (that are not audio > or video) that should be included? Nope, don't need to worry about that.
Please take a look, hta and magjed.
On 2016/11/18 19:34:15, Taylor Brandstetter wrote: > > Does this mean that there are advertised codecs that these stats ignore > because > > they're not part of the currently selected channels? Should codecs stats come > > from transport stats instead? Or is it correct to only return codecs stats > that > > are on existing channels (disregarding things mentioned in the SDP)? > > No, what you have now should be good. A "BaseChannel" is roughly equivalent to > an m= section in SDP, which is roughly equivalent to an RtpTransceiver. While the SDP offer may contain multiple m= lines, I guess the answer only has one for audio and one for video? And we should not have RTCCodecStats for stuff in the offer that is not agreed upon by the answer I take it? Then the current code does make sense. (Otherwise we'd have to generate RTCCodecStats from all supported codecs as well?)
On 2016/11/21 10:53:14, hbos wrote: > On 2016/11/18 19:34:15, Taylor Brandstetter wrote: > > > Does this mean that there are advertised codecs that these stats ignore > > because > > > they're not part of the currently selected channels? Should codecs stats > come > > > from transport stats instead? Or is it correct to only return codecs stats > > that > > > are on existing channels (disregarding things mentioned in the SDP)? > > > > No, what you have now should be good. A "BaseChannel" is roughly equivalent to > > an m= section in SDP, which is roughly equivalent to an RtpTransceiver. > > While the SDP offer may contain multiple m= lines, I guess the answer only has > one for audio and one for video? > And we should not have RTCCodecStats for stuff in the offer that is not agreed > upon by the answer I take it? Then the current code does make sense. (Otherwise > we'd have to generate RTCCodecStats from all supported codecs as well?) Answers always have the same number of m= lines as offers. In Unified Plan, there is one m= line per MediaStreamTrack. So don't design in an assumption about the number of m= lines.
On 2016/11/21 11:08:15, hta-webrtc wrote: > On 2016/11/21 10:53:14, hbos wrote: > > On 2016/11/18 19:34:15, Taylor Brandstetter wrote: > > > > Does this mean that there are advertised codecs that these stats ignore > > > because > > > > they're not part of the currently selected channels? Should codecs stats > > come > > > > from transport stats instead? Or is it correct to only return codecs stats > > > that > > > > are on existing channels (disregarding things mentioned in the SDP)? > > > > > > No, what you have now should be good. A "BaseChannel" is roughly equivalent > to > > > an m= section in SDP, which is roughly equivalent to an RtpTransceiver. > > > > While the SDP offer may contain multiple m= lines, I guess the answer only has > > one for audio and one for video? > > And we should not have RTCCodecStats for stuff in the offer that is not agreed > > upon by the answer I take it? Then the current code does make sense. > (Otherwise > > we'd have to generate RTCCodecStats from all supported codecs as well?) > > Answers always have the same number of m= lines as offers. > In Unified Plan, there is one m= line per MediaStreamTrack. So don't design in > an assumption about the number of m= lines. My bad, the number of m= lines might not be relevant. The question is if it is correct not to return RTCCodecStats for every codec mentioned in the offer/answer. I'm thinking it is, that you only return codec stats for codecs that you have created based on the negotiation, these codecs being the result, rather than the codecs listed in the SDP that are discarded. Is this correct or not?
@hta: It's true that we shouldn't write code assuming "1 video m= section, 1 audio m= section". But the problem is that's what the assumption already is, in PeerConnection's implementation and in the stats structures. If you set an offer with two video m= lines, we ignore the second one and blow up when creating the answer. So ultimately, the design of other things will have to change before this stats implementation can be updated. @hbos: I think it's fine to only return negotiated codecs. Maybe even only the codecs being used, since a codec by itself isn't very useful. It's not linked to any other stats reports by ID if it's not actively used, so you can't even know what tracks it *could* be used for.
lgtm, with a number of "please add TODO" type comments. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:36: std::string RTCCodecStatsIDFromDirectionTypeAndPayload( On 2016/11/17 22:01:51, Taylor Brandstetter wrote: > nit: I think you meant "DirectionAndPayloadType"? It's from "Direction, Type and Payload", so I guess it's OK. The alternate names are "DirectionTypePayload" and "DirectionAndTypeAndPayload". Not much readability improvement in either. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:39: return audio ? "RTCCodec_InboundAudio_" + rtc::ToString<>(payload_type) On 2016/11/18 19:34:15, Taylor Brandstetter wrote: > On 2016/11/18 09:08:13, hbos wrote: > > But in any given time we only have one voice, one video and one data channel > in > > this context (std::unique_ptr)? I can set up voice to use one transport and > > video to use another, but I'm already distinguishing between audio and video. > > Are there other channels equivalent of session()->blah_channel() that I'm > > ignoring right now? > > No, you're right. This is what I was talking about when I said "this issue is > impossible to run into until we implement 'Unified Plan' SDP". I guess there's > no way to test it right now, either. > > > Does this mean that there are advertised codecs that these stats ignore > because > > they're not part of the currently selected channels? Should codecs stats come > > from transport stats instead? Or is it correct to only return codecs stats > that > > are on existing channels (disregarding things mentioned in the SDP)? > > No, what you have now should be good. A "BaseChannel" is roughly equivalent to > an m= section in SDP, which is roughly equivalent to an RtpTransceiver. > > > Oh and there are also data channels, are there data codecs (that are not audio > > or video) that should be included? > > Nope, don't need to worry about that. > Add a TODO to update this when we handle multiple m= lines of the same media type, and we're good. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:731: true, false, *video_receiver_info.codec_payload_type); nit: this shows that the arguments to the call becomes unreadable at the call site. If the two first arguments had been enums rather than bools, this would have been more readable. If no enum is readily available, just keep it as it is. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1533: expected_video.codec_id = "RTCCodec_InboundVideo_42"; Nit: Since IDs are defined by the spec to be opaque (any unique string is conformant), the test should check that expected_video.codec_id exists in the stats record and points to a codec stats object. Otherwise, the test will break when you change the format of the video codec ID. If this is hard to do, leave a TODO. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1588: expected_audio.codec_id = "RTCCodec_OutboundAudio_42"; As above. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/stats/rtcstats... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/stats/rtcstats... webrtc/api/stats/rtcstats_objects.h:66: // stream changes codec. I don't think this is true at the moment.... direction + type + PT will give you unique and stable IDs as long as you don't have multiple m= lines using the same payload type (which, again, can only happen if you don't bundle connections). It would be more correct to say "TODO(hbos): The present codec ID assignment is not sufficient to support Unified Plan or unbundled connections in all cases."
Patchset #3 (id:80001) has been deleted
https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:36: std::string RTCCodecStatsIDFromDirectionTypeAndPayload( On 2016/11/23 07:37:35, hta-webrtc wrote: > On 2016/11/17 22:01:51, Taylor Brandstetter wrote: > > nit: I think you meant "DirectionAndPayloadType"? > > It's from "Direction, Type and Payload", so I guess it's OK. The alternate names > are "DirectionTypePayload" and "DirectionAndTypeAndPayload". Not much > readability improvement in either. Hm, I replaced "Type" with "Media" as in media type. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:39: return audio ? "RTCCodec_InboundAudio_" + rtc::ToString<>(payload_type) On 2016/11/23 07:37:35, hta-webrtc wrote: > On 2016/11/18 19:34:15, Taylor Brandstetter wrote: > > On 2016/11/18 09:08:13, hbos wrote: > > > But in any given time we only have one voice, one video and one data channel > > in > > > this context (std::unique_ptr)? I can set up voice to use one transport and > > > video to use another, but I'm already distinguishing between audio and > video. > > > Are there other channels equivalent of session()->blah_channel() that I'm > > > ignoring right now? > > > > No, you're right. This is what I was talking about when I said "this issue is > > impossible to run into until we implement 'Unified Plan' SDP". I guess there's > > no way to test it right now, either. > > > > > Does this mean that there are advertised codecs that these stats ignore > > because > > > they're not part of the currently selected channels? Should codecs stats > come > > > from transport stats instead? Or is it correct to only return codecs stats > > that > > > are on existing channels (disregarding things mentioned in the SDP)? > > > > No, what you have now should be good. A "BaseChannel" is roughly equivalent to > > an m= section in SDP, which is roughly equivalent to an RtpTransceiver. > > > > > Oh and there are also data channels, are there data codecs (that are not > audio > > > or video) that should be included? > > > > Nope, don't need to worry about that. > > > > Add a TODO to update this when we handle multiple m= lines of the same media > type, and we're good. Done. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:731: true, false, *video_receiver_info.codec_payload_type); On 2016/11/23 07:37:35, hta-webrtc wrote: > nit: this shows that the arguments to the call becomes unreadable at the call > site. If the two first arguments had been enums rather than bools, this would > have been more readable. > > If no enum is readily available, just keep it as it is. > No enum is readily available, keeping as-is. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1533: expected_video.codec_id = "RTCCodec_InboundVideo_42"; On 2016/11/23 07:37:35, hta-webrtc wrote: > Nit: Since IDs are defined by the spec to be opaque (any unique string is > conformant), the test should check that expected_video.codec_id exists in the > stats record and points to a codec stats object. Otherwise, the test will break > when you change the format of the video codec ID. > > If this is hard to do, leave a TODO. > Done here and three other places. (in/out audio/video) https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1588: expected_audio.codec_id = "RTCCodec_OutboundAudio_42"; On 2016/11/23 07:37:35, hta-webrtc wrote: > As above. Done. https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/stats/rtcstats... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2509803004/diff/40001/webrtc/api/stats/rtcstats... webrtc/api/stats/rtcstats_objects.h:66: // stream changes codec. On 2016/11/23 07:37:35, hta-webrtc wrote: > I don't think this is true at the moment.... direction + type + PT will give you > unique and stable IDs as long as you don't have multiple m= lines using the same > payload type (which, again, can only happen if you don't bundle connections). > > It would be more correct to say "TODO(hbos): The present codec ID assignment is > not sufficient to support Unified Plan or unbundled connections in all cases." Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2509803004/#ps100001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
Description was changed from ========== RTCCodecStats[1] added. RTCStatsCollector supports "payloadType", "codec" and "clockRate". "channels", "parameters" and "implementation" need to be supported before closing crbug.com/659117. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117, chromium:627816, chromium:657854 ========== to ========== RTCCodecStats[1] added. RTCStatsCollector supports "payloadType", "codec" and "clockRate". "channels", "parameters" and "implementation" need to be supported before closing crbug.com/659117. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117, chromium:627816, chromium:657854 NOTRY=True ==========
The CQ bit was checked by hbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Landing with NOTRY=True because android_more_configs is red for unrelated reasons, everything else green.
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479897032359570,
"parent_rev": "b0b805896aafdb02b95147cdcfd12b7503eb71e6", "commit_rev":
"51343f20bf408e29a5f76b11d440fb2f783b9762"}
Message was sent while issue was closed.
Description was changed from ========== RTCCodecStats[1] added. RTCStatsCollector supports "payloadType", "codec" and "clockRate". "channels", "parameters" and "implementation" need to be supported before closing crbug.com/659117. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117, chromium:627816, chromium:657854 NOTRY=True ========== to ========== RTCCodecStats[1] added. RTCStatsCollector supports "payloadType", "codec" and "clockRate". "channels", "parameters" and "implementation" need to be supported before closing crbug.com/659117. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117, chromium:627816, chromium:657854 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== RTCCodecStats[1] added. RTCStatsCollector supports "payloadType", "codec" and "clockRate". "channels", "parameters" and "implementation" need to be supported before closing crbug.com/659117. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117, chromium:627816, chromium:657854 NOTRY=True ========== to ========== RTCCodecStats[1] added. RTCStatsCollector supports "payloadType", "codec" and "clockRate". "channels", "parameters" and "implementation" need to be supported before closing crbug.com/659117. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117, chromium:627816, chromium:657854 NOTRY=True Committed: https://crrev.com/0adb8285b101ee7e8a8e1b03200018449f606d6a Cr-Commit-Position: refs/heads/master@{#15207} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0adb8285b101ee7e8a8e1b03200018449f606d6a Cr-Commit-Position: refs/heads/master@{#15207} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
