Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2306873003:
Add RTCP packet class for signaling encoder target bitrate. (Closed)
|
Created:
4 years, 3 months ago by sprang_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd RTCP packet class for signaling encoder target bitrate.
This is a proposal for a new RTCP message. Feel free to comment on the
message structure, selected type ids etc, as well as code for
serialization/deserialization. Once we agree on this, I'll continue
with wiring it up in the actual rtcp sender and receiver.
BUG=webrtc:6301
Committed: https://crrev.com/b84ad63b0a189a33abaf78a527b04f881d881e91
Cr-Commit-Position: refs/heads/master@{#14867}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed comments #
Total comments: 12
Patch Set 3 : Changed to psfb afb, with new unique id, addressed comments. #
Total comments: 9
Patch Set 4 : Comments #
Total comments: 14
Patch Set 5 : Addressed comments #Patch Set 6 : Actually enforce media ssrc 0 #
Total comments: 8
Patch Set 7 : Addressed comments #Patch Set 8 : Changed target bitrate to an XR block #
Total comments: 10
Patch Set 9 : Fixed bug, addressed comments #
Total comments: 18
Patch Set 10 : Addressed comments #
Total comments: 4
Patch Set 11 : Rebase #Patch Set 12 : Fixed bad rebase #Messages
Total messages: 38 (11 generated)
sprang@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
I also have some style comments, do you want them now or later, when format will settle? https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:55: // | S | T | Target Bitrate | 24bits for bitrate probably better to split into mantissa and exponenta, like in remb. Otherwise bitrate would be limited by 16Mbps. https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:119: size_t index = Psfb::kCommonFeedbackLength; add bitrates_.clear(); Just in case. https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2016 https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:57: rtc::Buffer short_buffer(kPacket, cropped_size); // Mutable copy. add third parameter bytes_needed (as capacity) https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:62: for (uint8_t i = 0; i < padding_bytes; ++i) { there is no need to zero padding, so might be cleaner to short_buffer.SetSize(bytes_needed); short_buffer[bytes_needed - 1] = padding_bytes; instead of the loop
https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2016 https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:55: // | S | T | Target Bitrate | On 2016/09/02 14:52:19, danilchap wrote: > 24bits for bitrate probably better to split into mantissa and exponenta, like in > remb. > Otherwise bitrate would be limited by 16Mbps. Or can we signal the bitrate in kbps instead of bps, or is the accuracy of bps needed? https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:62: // Indicates which temporal layer this bitrate concerns. Will this always be 0 in the simulcast case and instead 3 different media sources will be added? Should be clarified.
https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/09/02 15:19:28, stefan-webrtc (holmer) wrote: > 2016 Done. https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:55: // | S | T | Target Bitrate | On 2016/09/02 15:19:28, stefan-webrtc (holmer) wrote: > On 2016/09/02 14:52:19, danilchap wrote: > > 24bits for bitrate probably better to split into mantissa and exponenta, like > in > > remb. > > Otherwise bitrate would be limited by 16Mbps. > > Or can we signal the bitrate in kbps instead of bps, or is the accuracy of bps > needed? Good point. I think going for kbps is simpler, bps precision is likely not needed. https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:62: // Indicates which temporal layer this bitrate concerns. On 2016/09/02 15:19:28, stefan-webrtc (holmer) wrote: > Will this always be 0 in the simulcast case and instead 3 different media > sources will be added? Should be clarified. Yes, for simulcast there would be one of these messages per SSRC, with spatial layer 0 and only temporal layers set. For VP9 SVC, there would be an entry for each spatial and temporal layer pair. I'll add a clarifying comment. https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:119: size_t index = Psfb::kCommonFeedbackLength; On 2016/09/02 14:52:19, danilchap wrote: > add bitrates_.clear(); Just in case. Done. https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/09/02 14:52:19, danilchap wrote: > 2016 Done. https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:57: rtc::Buffer short_buffer(kPacket, cropped_size); // Mutable copy. On 2016/09/02 14:52:19, danilchap wrote: > add third parameter bytes_needed (as capacity) Done. https://codereview.webrtc.org/2306873003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:62: for (uint8_t i = 0; i < padding_bytes; ++i) { On 2016/09/02 14:52:19, danilchap wrote: > there is no need to zero padding, so might be cleaner to > short_buffer.SetSize(bytes_needed); > short_buffer[bytes_needed - 1] = padding_bytes; > instead of the loop Done.
rtcp spec discourage use of new FMT types without registration, and suggest to use application specific messages. Either Psfb APP (like remb) or generic APP message (PayloadType=204, currently unused in our implementation) packing in Psfb APP would require 4 extra bytes per message, packing generic APP wouldn't require extra bytes, since, as I understand, SenderSsrc = SourceSsrc for this message. Also, some style suggestions. https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:18: #include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h" shouldn't be needed. https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:32: struct BitrateItem { struct declaration probably counts as typedef and so should go first (before constant) in the class declaration order. https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:17: using webrtc::rtcp::TargetBitrate; using recommended inside unnamed namespace, to the top; and with full names after =. may be add using ::webrtc::test::ParseSinglePacket https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:29: rtcp::Psfb::kPacketType, Maybe refer to this constant by name TargetBitrate::kPacketType. Maybe indent this line to start where 2nd bytes start, https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:30: 0x00, 0x06, // Length: 6 + 1 word32. and this line where 3rd bytes start. https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:41: class TargetBitrateTest : public ::testing::Test { this fixture doesn't do much and probably can be avoided: ExpectBitrateItemEquals can be moved unchanged into unnamed namespace. TestPartialPacket either just move into unnamed namespace using own copy of target_bitrate, or split into Setup part (helper rtc::Buffer PartialPacket(size_t)) and test part (EXPECT_TRUE(test::ParsingSinglePacket(test_packet, &target_bitrate)); TargetBitrate member do not use any special initialization and can be redeclared in each test.
On 2016/09/05 07:50:46, danilchap wrote: > rtcp spec discourage use of new FMT types without registration, and suggest to > use application specific messages. > Either Psfb APP (like remb) > or generic APP message (PayloadType=204, currently unused in our implementation) > packing in Psfb APP would require 4 extra bytes per message, > packing generic APP wouldn't require extra bytes, since, as I understand, > SenderSsrc = SourceSsrc for this message. > You're right, this was not correctly done. I've changed to psfb / afb, like remb, since we already handle that, but added a new unique id 'BITR'. ptal
https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:49: // 8 | SSRC of media source | Will it be possible "Packet Sender != Media Source" https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:51: // 12 | Unique identifier 'B' 'T' 'R' 'T' | BITR, according to comment in the header file. https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:144: index += 4; may be += kBitrateItemSizeBytes; https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:51: static constexpr uint32_t kUniqueIdentifier = 0x42495452; // BITR. Probably better to make this constant public (and do same for Remb) so that Rtcp Parser would be able to choose right class to use for parsing.
sprang@google.com changed reviewers: + sprang@google.com
https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:18: #include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h" On 2016/09/05 07:50:46, danilchap wrote: > shouldn't be needed. Done. https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:32: struct BitrateItem { On 2016/09/05 07:50:46, danilchap wrote: > struct declaration probably counts as typedef and so should go first (before > constant) in the class declaration order. Done. https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:17: using webrtc::rtcp::TargetBitrate; On 2016/09/05 07:50:46, danilchap wrote: > using recommended inside unnamed namespace, to the top; > and with full names after =. > > may be add using ::webrtc::test::ParseSinglePacket Done. https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:29: rtcp::Psfb::kPacketType, On 2016/09/05 07:50:46, danilchap wrote: > Maybe refer to this constant by name TargetBitrate::kPacketType. > Maybe indent this line to start where 2nd bytes start, Done. https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:30: 0x00, 0x06, // Length: 6 + 1 word32. On 2016/09/05 07:50:46, danilchap wrote: > and this line where 3rd bytes start. Done. I haven't seen that style of indentation before, but makes sense. Now I get your indentation in the rtcp_receiver_unittest cl... https://codereview.webrtc.org/2306873003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:41: class TargetBitrateTest : public ::testing::Test { On 2016/09/05 07:50:46, danilchap wrote: > this fixture doesn't do much and probably can be avoided: > ExpectBitrateItemEquals can be moved unchanged into unnamed namespace. > > TestPartialPacket either just move into unnamed namespace using own copy of > target_bitrate, > or split into Setup part (helper rtc::Buffer PartialPacket(size_t)) > and test part (EXPECT_TRUE(test::ParsingSinglePacket(test_packet, > &target_bitrate)); > > TargetBitrate member do not use any special initialization and can be redeclared > in each test. Done. https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:49: // 8 | SSRC of media source | On 2016/09/05 13:48:40, danilchap wrote: > Will it be possible "Packet Sender != Media Source" Maybe not. Would you prefer to set this to 0, as with REMB? https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:51: // 12 | Unique identifier 'B' 'T' 'R' 'T' | On 2016/09/05 13:48:40, danilchap wrote: > BITR, according to comment in the header file. Done. https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:144: index += 4; On 2016/09/05 13:48:40, danilchap wrote: > may be += kBitrateItemSizeBytes; Done. https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:51: static constexpr uint32_t kUniqueIdentifier = 0x42495452; // BITR. On 2016/09/05 13:48:40, danilchap wrote: > Probably better to make this constant public (and do same for Remb) so that Rtcp > Parser would be able to choose right class to use for parsing. Done.
https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:49: // 8 | SSRC of media source | On 2016/09/05 14:38:26, sprang wrote: > On 2016/09/05 13:48:40, danilchap wrote: > > Will it be possible "Packet Sender != Media Source" > > Maybe not. Would you prefer to set this to 0, as with REMB? I think it is better than providing two same values all the time. Two SSRCs in Feedback messages because initially it was meant for receiver to give feedback to sender, but since here it is used by sender to notify receivers, two SSRCs are not needed, receiver's ssrc is irrelevant. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:45: // 0 |V=2|P| FMT=17 | PT=206 | length | FMT=15 https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:47: // 4 | SSRC of packet sender | In other rtcp classes I excluded header when counting offset, since Parse function get pointer to payload, not to header. May be do same or remove this offsets since they are not used in code directly. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:166: return kTotalHeaderLength + (bitrates_.size() * 4); may be * kBitrateItemSizeBytes instead of * 4 https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:64: RTC_DISALLOW_COPY_AND_ASSIGN(TargetBitrate); this macro should be last in the class definition, below member. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:20: using TargetBitrate = webrtc::rtcp::TargetBitrate; using ::webrtc::rtcp::TargetBitrate; https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:74: EXPECT_EQ(4u, bitrates.size()); May be 4U https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:110: EXPECT_EQ(buffer.size(), sizeof(kPacket)); expected (sizeof(kPacket)) - first, value under test - (buffer.size()) - second. Might also switch EXPECT to ASSERT because next check better not to run if size mismatch.
https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:45: // 0 |V=2|P| FMT=17 | PT=206 | length | On 2016/09/06 08:22:39, danilchap wrote: > FMT=15 Done. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:47: // 4 | SSRC of packet sender | On 2016/09/06 08:22:39, danilchap wrote: > In other rtcp classes I excluded header when counting offset, since Parse > function get pointer to payload, not to header. > May be do same or remove this offsets since they are not used in code directly. Done. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:166: return kTotalHeaderLength + (bitrates_.size() * 4); On 2016/09/06 08:22:39, danilchap wrote: > may be * kBitrateItemSizeBytes instead of * 4 Done. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:64: RTC_DISALLOW_COPY_AND_ASSIGN(TargetBitrate); On 2016/09/06 08:22:39, danilchap wrote: > this macro should be last in the class definition, below member. Done. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:20: using TargetBitrate = webrtc::rtcp::TargetBitrate; On 2016/09/06 08:22:39, danilchap wrote: > using ::webrtc::rtcp::TargetBitrate; Done. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:74: EXPECT_EQ(4u, bitrates.size()); On 2016/09/06 08:22:39, danilchap wrote: > May be 4U Done. https://codereview.webrtc.org/2306873003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:110: EXPECT_EQ(buffer.size(), sizeof(kPacket)); On 2016/09/06 08:22:39, danilchap wrote: > expected (sizeof(kPacket)) - first, value under test - (buffer.size()) - second. > Might also switch EXPECT to ASSERT because next check better not to run if size > mismatch. Done.
lgtm https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:33: uint32_t target_bitrate_bps) kbps https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:127: if (media_ssrc() != 0) { probably better to ignore than to fail parse. https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:160: uint32_t target_bitrate_bps) { target_bitrate_kpbs https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:109: target_bitrate.To(0); No need, can actually hide To/media_ssrc functions in the TargetBitrate (by putting them into private section without defintion)
https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:33: uint32_t target_bitrate_bps) On 2016/09/07 15:02:12, danilchap wrote: > kbps Done. https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:127: if (media_ssrc() != 0) { On 2016/09/07 15:02:12, danilchap wrote: > probably better to ignore than to fail parse. Sure. I'll keep the warning though. https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:160: uint32_t target_bitrate_bps) { On 2016/09/07 15:02:12, danilchap wrote: > target_bitrate_kpbs Done. https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:109: target_bitrate.To(0); On 2016/09/07 15:02:12, danilchap wrote: > No need, can actually hide To/media_ssrc functions in the TargetBitrate (by > putting them into private section without defintion) Done.
Description was changed from ========== Add RTCP packet class for signaling encoder target bitrate. This is a proposal for a new RTCP message. Feel free to comment on the message structure, selected type ids etc, as well as code for serialization/deserialization. Once we agree on this, I'll continue with wiring it up in the actual rtcp sender and receiver. BUG=webrtc:6301 ========== to ========== Add RTCP packet class for signaling encoder target bitrate. This is a proposal for a new RTCP message. Feel free to comment on the message structure, selected type ids etc, as well as code for serialization/deserialization. Once we agree on this, I'll continue with wiring it up in the actual rtcp sender and receiver. BUG=webrtc:6301 ==========
sprang@webrtc.org changed reviewers: - sprang@google.com
Changed packet type to an XR block. ptal
not lgtm (to remind myself to look again) https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h:66: static const size_t kMaxNumberOfTargetBitrateBlocks = 50; Does it make sense to have more than one BitrateBlock in XR report? (For other blocks I beleive answer is 'no' and plan to simplify ExtendedReports class because of that, replacing std::vector with rtc::Optional) https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h:27: static constexpr uint32_t kUniqueIdentifier = 0x52454D42; // 'R' 'E' 'M' 'B'. with TargetBitrate moved into ExtendedReports, this change no longer belong to this CL. https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:103: index == 1 here? https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:23: static constexpr uint8_t kBlockType = 42; According to the rfc3611 Additional block types MUST be registered with the IANA So add a TODO or some other comment it is experimental. https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:22: using ::webrtc::rtcp::TargetBitrate; remove ::webrtc:: (I was wrong name in using should always be fully-qualified, when it is subname of current namespace, it shouldn't and it is nice when it is not)
https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/extended_reports.h:66: static const size_t kMaxNumberOfTargetBitrateBlocks = 50; On 2016/09/23 13:39:49, danilchap wrote: > Does it make sense to have more than one BitrateBlock in XR report? > (For other blocks I beleive answer is 'no' and plan to simplify ExtendedReports > class because of that, replacing std::vector with rtc::Optional) You're right. I just followed the current style without giving it much thought. Changed to just an optional. https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/remb.h:27: static constexpr uint32_t kUniqueIdentifier = 0x52454D42; // 'R' 'E' 'M' 'B'. On 2016/09/23 13:39:49, danilchap wrote: > with TargetBitrate moved into ExtendedReports, this change no longer belong to > this CL. Acknowledged. https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:103: On 2016/09/23 13:39:49, danilchap wrote: > index == 1 here? Yes, this CL was screwed up, sorry. Updated. https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:23: static constexpr uint8_t kBlockType = 42; On 2016/09/23 13:39:49, danilchap wrote: > According to the rfc3611 > Additional block types MUST be registered with the IANA > So add a TODO or some other comment it is experimental. Done. https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:22: using ::webrtc::rtcp::TargetBitrate; On 2016/09/23 13:39:49, danilchap wrote: > remove ::webrtc:: (I was wrong name in using should always be fully-qualified, > when it is subname of current namespace, it shouldn't and it is nice when it is > not) Done.
looks good, just some nits https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:81: // Base RTCP header already parsed. remove this comment since TargetBitrate is no longer individual rtcp packet. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:91: (block_length + 1) * 4 - kTargetBitrateHeaderSizeBytes; may be just block_length * 4 (though rtcp spec define block_length as "length of this report block, including the header, in 32-bit words minus one" the definition "length of this report block, excluding the header, in 32-bit words" might make slightly more sense) https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:94: uint8_t block_type = block[0]; may be join this line into next one since you do not use block_type anywhere else https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:95: RTC_DCHECK_EQ(kBlockType, block_type); for DCHECK prefer same parameters order as you would use for usual operators. You probably would prefer to write "block_type == kBlockType" instead of "kBlockType == block_type" https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:99: const size_t num_items = payload_bytes / kBitrateItemSizeBytes; may be join this line with payload_bytes calculation, or move them closer to each other since this is the only line where payload_bytes is used. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:103: uint32_t bitrate = may be bitrate_kbps https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:117: bitrates_.push_back( may be add RTC_DCHECK_LE(target_bitrate_kbps, 0x00FFFFFF) https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:39: TargetBitrate() = default; move implementation of constructor & destructor into .cc file (constructor is default but not trivial because of std::vector) https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:62: const size_t kXTHeaderSize = 8; // RTCP header (4) + SSRC (4). kXRHeaderSize? (since eXtendedReports is more often abbreviated as XR)
https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:81: // Base RTCP header already parsed. On 2016/09/28 11:27:24, danilchap wrote: > remove this comment since TargetBitrate is no longer individual rtcp packet. Done. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:91: (block_length + 1) * 4 - kTargetBitrateHeaderSizeBytes; On 2016/09/28 11:27:24, danilchap wrote: > may be just block_length * 4 > (though rtcp spec define block_length as "length of this report block, including > the header, in 32-bit words minus one" > the definition "length of this report block, excluding the header, in 32-bit > words" might make slightly more sense) Done. Added comment to clarify. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:94: uint8_t block_type = block[0]; On 2016/09/28 11:27:24, danilchap wrote: > may be join this line into next one since you do not use block_type anywhere > else Done. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:95: RTC_DCHECK_EQ(kBlockType, block_type); On 2016/09/28 11:27:24, danilchap wrote: > for DCHECK prefer same parameters order as you would use for usual operators. > You probably would prefer to write "block_type == kBlockType" instead of > "kBlockType == block_type" Done. I'm usually in the habit of placing "expected" first, but I guess I doesn't really matter here since it's not an EXPECT_EQ... https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:99: const size_t num_items = payload_bytes / kBitrateItemSizeBytes; On 2016/09/28 11:27:24, danilchap wrote: > may be join this line with payload_bytes calculation, or move them closer to > each other since this is the only line where payload_bytes is used. Done. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:103: uint32_t bitrate = On 2016/09/28 11:27:24, danilchap wrote: > may be bitrate_kbps Done. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:117: bitrates_.push_back( On 2016/09/28 11:27:24, danilchap wrote: > may be add RTC_DCHECK_LE(target_bitrate_kbps, 0x00FFFFFF) Done. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:39: TargetBitrate() = default; On 2016/09/28 11:27:24, danilchap wrote: > move implementation of constructor & destructor into .cc file (constructor is > default but not trivial because of std::vector) Done. https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:62: const size_t kXTHeaderSize = 8; // RTCP header (4) + SSRC (4). On 2016/09/28 11:27:24, danilchap wrote: > kXRHeaderSize? (since eXtendedReports is more often abbreviated as XR) Done. Typo + autocomplete...
lgtm https://codereview.webrtc.org/2306873003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right): https://codereview.webrtc.org/2306873003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:97: // occupies exactly on block, we can just treat this as payload length. s/on/one/ https://codereview.webrtc.org/2306873003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:119: bitrates_.push_back( constructing & adding to vector may be combined with emplace_back function: bitrates_.emplace_back(spatial_layer, temporal_layer, target_bitrate_kbps); https://codereview.webrtc.org/2306873003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h (right): https://codereview.webrtc.org/2306873003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.h:26: static const size_t kBitrateItemSizeBytes; can be constexpr too, may be left as const. https://codereview.webrtc.org/2306873003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc (right): https://codereview.webrtc.org/2306873003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate_unittest.cc:61: TEST(TargetBitrateTest, FullPacket) { may be move this test to extended_reports_unittest
stefan, wanna take another pass?
On 2016/10/24 13:36:44, språng wrote: > stefan, wanna take another pass? ping?
On 2016/10/24 13:36:44, språng wrote: > stefan, wanna take another pass? ping?
The message format looks ok, so lgtm.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2306873003/#ps200001 (title: "Rebase")
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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/19154)
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2306873003/#ps220001 (title: "Fixed bad rebase")
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.
Description was changed from ========== Add RTCP packet class for signaling encoder target bitrate. This is a proposal for a new RTCP message. Feel free to comment on the message structure, selected type ids etc, as well as code for serialization/deserialization. Once we agree on this, I'll continue with wiring it up in the actual rtcp sender and receiver. BUG=webrtc:6301 ========== to ========== Add RTCP packet class for signaling encoder target bitrate. This is a proposal for a new RTCP message. Feel free to comment on the message structure, selected type ids etc, as well as code for serialization/deserialization. Once we agree on this, I'll continue with wiring it up in the actual rtcp sender and receiver. BUG=webrtc:6301 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add RTCP packet class for signaling encoder target bitrate. This is a proposal for a new RTCP message. Feel free to comment on the message structure, selected type ids etc, as well as code for serialization/deserialization. Once we agree on this, I'll continue with wiring it up in the actual rtcp sender and receiver. BUG=webrtc:6301 ========== to ========== Add RTCP packet class for signaling encoder target bitrate. This is a proposal for a new RTCP message. Feel free to comment on the message structure, selected type ids etc, as well as code for serialization/deserialization. Once we agree on this, I'll continue with wiring it up in the actual rtcp sender and receiver. BUG=webrtc:6301 Committed: https://crrev.com/b84ad63b0a189a33abaf78a527b04f881d881e91 Cr-Commit-Position: refs/heads/master@{#14867} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b84ad63b0a189a33abaf78a527b04f881d881e91 Cr-Commit-Position: refs/heads/master@{#14867} |
