Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2484193002:
Expose RtpCodecParameters to VideoMediaInfo stats. (Closed)
DescriptionExpose RtpCodecParameters to VideoMediaInfo stats.
Payload type -> RtpCodecParameters maps added for sender and receiver
side. It contains information that will be needed for RTCCodecStats[1]
dictionaries.
Video[Sender/Receiver]Info is updated with current codec payload type
for every stream which can be used to look up the codec in
VideoMediaInfo.
A similar change should be made for VoiceMediaInfo and
Voice[Sender/Receiver]Info.
[1] https://w3c.github.io/webrtc-stats/#codec-dict*
BUG=chromium:659117
Committed: https://crrev.com/a65704b5c905c97970f28eb3d1df7369a700efdf
Cr-Commit-Position: refs/heads/master@{#15060}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments #
Total comments: 7
Patch Set 3 : Addressed magjed's comments #
Messages
Total messages: 29 (15 generated)
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look deadbeef and hta. I though I'd update these layers separately before revisiting RTCCodecStats. This feels like a step in the right direction, but RtpCodecParameters does not have RTCCodecStats' "parameters" and "implementation". If a cricket::VideoCodec (extends cricket::Codec) would be exposed instead of RtpCodecParameters we would also have access to cricket::Codec::params (of type typedef std::map<std::string, std::string> CodecParameterMap) which might be a map version of what "parameters" (a string) should be? But that would require further changes. Neither RtpCodecParameters or VideoCodec has "implementation". Which makes sense, but now that is set on a per-stream basis, which doesn't make sense. (Also is clockrate hard-coded to kVideoCodecClockrate in all cases or is that just the default?)
hbos@webrtc.org changed reviewers: + magjed@webrtc.org
And please take a look magjed.
On 2016/11/08 12:01:37, hbos wrote: > Please take a look deadbeef and hta. > > I though I'd update these layers separately before revisiting RTCCodecStats. > > This feels like a step in the right direction, but RtpCodecParameters does not > have RTCCodecStats' "parameters" and "implementation". If a cricket::VideoCodec > (extends cricket::Codec) would be exposed instead of RtpCodecParameters we would > also have access to cricket::Codec::params (of type typedef > std::map<std::string, std::string> CodecParameterMap) which might be a map > version of what "parameters" (a string) should be? But that would require > further changes. > > Neither RtpCodecParameters or VideoCodec has "implementation". Which makes > sense, but now that is set on a per-stream basis, which doesn't make sense. > > (Also is clockrate hard-coded to kVideoCodecClockrate in all cases or is that > just the default?) RtpCodecParameters will eventually have "parameters" (in the form of "sdpFmtpLine"), but likely not "implementation". We could probably just replace RtpCodecParameters with another structure (possibly one that contains RtpCodecParameters) when that time comes. And yes, the clock rates for all video codecs we support are 90000. I'm not an RTP person so I don't know why.
https://codereview.webrtc.org/2484193002/diff/1/webrtc/media/base/videoengine... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/2484193002/diff/1/webrtc/media/base/videoengine... webrtc/media/base/videoengine_unittest.h:427: EXPECT_EQ(DefaultCodec().id, *info.senders[0].codec_payload_type); nit: This will crash if codec_payload_type if null (I think), so may want to change the above EXPECT_TRUE to an ASSERT_TRUE. https://codereview.webrtc.org/2484193002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2484193002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1427: } I think you can delete this block, since there shouldn't be any difference between the stream-specific codec and the common one. GetRtpParameters doesn't even fill any codecs in.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
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/...
PTAL deadbeef, magjed. On 2016/11/08 23:55:28, Taylor Brandstetter wrote: > RtpCodecParameters will eventually have "parameters" (in the form of > "sdpFmtpLine"), but likely not "implementation". We could probably just replace > RtpCodecParameters with another structure (possibly one that contains > RtpCodecParameters) when that time comes. > > And yes, the clock rates for all video codecs we support are 90000. I'm not an > RTP person so I don't know why. Acknowledged. I'll see that as future work. https://codereview.webrtc.org/2484193002/diff/1/webrtc/media/base/videoengine... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/2484193002/diff/1/webrtc/media/base/videoengine... webrtc/media/base/videoengine_unittest.h:427: EXPECT_EQ(DefaultCodec().id, *info.senders[0].codec_payload_type); On 2016/11/08 23:55:44, Taylor Brandstetter wrote: > nit: This will crash if codec_payload_type if null (I think), so may want to > change the above EXPECT_TRUE to an ASSERT_TRUE. Done. Here and for receivers. https://codereview.webrtc.org/2484193002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2484193002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1427: } On 2016/11/08 23:55:44, Taylor Brandstetter wrote: > I think you can delete this block, since there shouldn't be any difference > between the stream-specific codec and the common one. GetRtpParameters doesn't > even fill any codecs in. Oh, that makes things simpler. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm fwiw, the 90000 frequency is the least common denominator (?) for 15, 29.97, 30, 60 and 120 - it can be divided using inter division into all the commonly used frame rates. 29.97 is the one that is responsible for the number being so ridiculously high. https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/base/videoen... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/base/videoen... webrtc/media/base/videoengine_unittest.h:427: EXPECT_EQ(DefaultCodec().id, *info.senders[0].codec_payload_type); Not for this CL, but: we've had several issues where one codec has been known by multiple payload types for various reasons. Now that there's an explicit map from payload type to codec description, perhaps we can think about taking the "id" field out of the codec?
lgtm. And thanks for the nice bit of trivia about the clock rate, Harald. :)
PTAL magjed
https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/base/videoen... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/base/videoen... webrtc/media/base/videoengine_unittest.h:436: EXPECT_NE(info.send_codecs.end(), info.send_codecs.find(DefaultCodec().id)); nit: I would prefer this: EXPECT_EQ(0, info.send_codecs.count(DefaultCodec().id)); https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/base/videoen... webrtc/media/base/videoengine_unittest.h:461: EXPECT_NE(info.receive_codecs.end(), nit: Same here, use map::count. https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1405: std::make_pair(codec_params.payload_type, codec_params)); This webrtc::RtpCodecParameters looks heavy, you can use std::move(codec_params) or just inline 'codec.ToCodecParameters()' instead? Also, are we assuming here that we get no conflicting payload types as keys?
PTAL jäddan https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/base/videoen... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/base/videoen... webrtc/media/base/videoengine_unittest.h:436: EXPECT_NE(info.send_codecs.end(), info.send_codecs.find(DefaultCodec().id)); On 2016/11/14 08:58:13, magjed_webrtc wrote: > nit: I would prefer this: > EXPECT_EQ(0, info.send_codecs.count(DefaultCodec().id)); Done. ("EXPECT_EQ(1U", not 0.) https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/base/videoen... webrtc/media/base/videoengine_unittest.h:461: EXPECT_NE(info.receive_codecs.end(), On 2016/11/14 08:58:13, magjed_webrtc wrote: > nit: Same here, use map::count. Done. https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2484193002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1405: std::make_pair(codec_params.payload_type, codec_params)); On 2016/11/14 08:58:13, magjed_webrtc wrote: > This webrtc::RtpCodecParameters looks heavy, you can use std::move(codec_params) > or just inline 'codec.ToCodecParameters()' instead? > Also, are we assuming here that we get no conflicting payload types as keys? Done. Yes, type (audio/video) + direction (send/receive) + payload type should uniquely identify RtpCodecParameters.
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
lgtm
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/2484193002/#ps80001 (title: "Addressed magjed's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Expose RtpCodecParameters to VideoMediaInfo stats. Payload type -> RtpCodecParameters maps added for sender and receiver side. It contains information that will be needed for RTCCodecStats[1] dictionaries. Video[Sender/Receiver]Info is updated with current codec payload type for every stream which can be used to look up the codec in VideoMediaInfo. A similar change should be made for VoiceMediaInfo and Voice[Sender/Receiver]Info. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117 ========== to ========== Expose RtpCodecParameters to VideoMediaInfo stats. Payload type -> RtpCodecParameters maps added for sender and receiver side. It contains information that will be needed for RTCCodecStats[1] dictionaries. Video[Sender/Receiver]Info is updated with current codec payload type for every stream which can be used to look up the codec in VideoMediaInfo. A similar change should be made for VoiceMediaInfo and Voice[Sender/Receiver]Info. [1] https://w3c.github.io/webrtc-stats/#codec-dict* BUG=chromium:659117 Committed: https://crrev.com/a65704b5c905c97970f28eb3d1df7369a700efdf Cr-Commit-Position: refs/heads/master@{#15060} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a65704b5c905c97970f28eb3d1df7369a700efdf Cr-Commit-Position: refs/heads/master@{#15060} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
