Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1202833003:
Added fields for configuration information to the protobuf format (Closed)
|
Created:
5 years, 6 months ago by terelius Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded fields for configuration information to the protobuf format
in the ACMDump. The ACMDump interface itself is not updated, so there
is no way (yet) to actually write the configuration fields.
BUG=
Committed: https://crrev.com/6e355af34802baf5e0a0ad67eb33e58c423a7a39
Cr-Commit-Position: refs/heads/master@{#9519}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Fixed most of the comments and changed to use the map feature where possible. #Patch Set 3 : Minor change in comments. #Patch Set 4 : Reverted use of maps since not supported by protobuf compiler #
Total comments: 3
Patch Set 5 : Minor fix of whitespace #Messages
Total messages: 17 (3 generated)
terelius@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org, stefan@webrtc.org
Please, see comments inline. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:89: // TODO(terelius): Video and audiostreams could in principle share ssrc, Nit: Please, write SSRC capitalized in comments. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:99: optional RtcpConfig rtcp_config = 3; I'm not a protobuf expert, but why is there no field = 2? https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:107: // List of decoders associated with the stream; End sentence with '.', not ';' https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:128: // SSRCs to use for the RTX streams. The comment says SSRCs (in plural). But the field is scalar, right? https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:142: // The interesting Rtcp packets are the ones that the sender receives Nit: Rtcp -> RTCP, when written in comments.
It might be a good idea to ask one of the experienced protobuf guys to have a look at this as well. See comments inline. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:98: // disabled if there's no config present. One of the experienced protobuf guys told me "Don’t make the presence/absence of a field semantically meaningful for proto messages". It might be a good idea to add a boolean to indicate if RTX is enabled or not for clarity. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:102: repeated RtxMap rtx_map = 4; Maybe we can use a map here instead of a seperate message? See https://developers.google.com/protocol-buffers/docs/proto#maps . https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:105: repeated RtpHeaderExtension header_extensions = 5; This could be another candidate to use the Map feature https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:108: repeated DecoderConfig decoders = 6; Maybe another map? https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:114: optional string name = 1; // required I think it would be good to keep comments on a seperate line, like in the rest of the file. Also a blank line between the different variables would be nice.
https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:13: nit: Do we need multiple (2) empty lines between each message? https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:89: // TODO(terelius): Video and audiostreams could in principle share ssrc, audio streams https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:114: optional string name = 1; // required On 2015/06/24 11:27:12, ivoc wrote: > I think it would be good to keep comments on a seperate line, like in the rest > of the file. Also a blank line between the different variables would be nice. And start with a capital letter and end with a period. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:150: enum RtcpMode {KRTCPCOMPOUND = 1; KRTCPREDUCEDSIZE = 2;} Not knowing the style guide for protobufs, is this the suggested way of writing constants? Seems like a mix of kRtcpCompound and RTCP_COMPOUND.
https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:89: // TODO(terelius): Video and audiostreams could in principle share ssrc, On 2015/06/24 11:07:55, hlundin-webrtc wrote: > Nit: Please, write SSRC capitalized in comments. Done. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:99: optional RtcpConfig rtcp_config = 3; On 2015/06/24 11:07:55, hlundin-webrtc wrote: > I'm not a protobuf expert, but why is there no field = 2? I (mostly) modeled the config fields on the VideoReceiveStream::Config struct, which has a field local_ssrc (in addition to the remote_ssrc). As far as I can determine, the local_ssrc is only used for sending RTCP so it made sense to locate it together with the other RTCP settings. However I still left "space" for it in the ACMDumpConfigEvent in case this assumption turns out to be incorrect. Not sure whether this is a good or bad idea. The numbers themselves are not important except that each field needs a unique tag and that tags <16 use a more compact coding than larger tags. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:107: // List of decoders associated with the stream; On 2015/06/24 11:07:55, hlundin-webrtc wrote: > End sentence with '.', not ';' Done. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:128: // SSRCs to use for the RTX streams. On 2015/06/24 11:07:55, hlundin-webrtc wrote: > The comment says SSRCs (in plural). But the field is scalar, right? You're right. I'll consider a complete rewrite of the documentation for RtxConfig. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:142: // The interesting Rtcp packets are the ones that the sender receives On 2015/06/24 11:07:55, hlundin-webrtc wrote: > Nit: Rtcp -> RTCP, when written in comments. Done.
On 2015/06/24 11:51:19, terelius wrote: > https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... > File webrtc/modules/audio_coding/main/acm2/dump.proto (right): > > https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... > webrtc/modules/audio_coding/main/acm2/dump.proto:89: // TODO(terelius): Video > and audiostreams could in principle share ssrc, > On 2015/06/24 11:07:55, hlundin-webrtc wrote: > > Nit: Please, write SSRC capitalized in comments. > > Done. > > https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... > webrtc/modules/audio_coding/main/acm2/dump.proto:99: optional RtcpConfig > rtcp_config = 3; > On 2015/06/24 11:07:55, hlundin-webrtc wrote: > > I'm not a protobuf expert, but why is there no field = 2? > > I (mostly) modeled the config fields on the VideoReceiveStream::Config struct, > which has a field local_ssrc (in addition to the remote_ssrc). As far as I can > determine, the local_ssrc is only used for sending RTCP so it made sense to > locate it together with the other RTCP settings. However I still left "space" > for it in the ACMDumpConfigEvent in case this assumption turns out to be > incorrect. Not sure whether this is a good or bad idea. > > The numbers themselves are not important except that each field needs a unique > tag and that tags <16 use a more compact coding than larger tags. > > https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... > webrtc/modules/audio_coding/main/acm2/dump.proto:107: // List of decoders > associated with the stream; > On 2015/06/24 11:07:55, hlundin-webrtc wrote: > > End sentence with '.', not ';' > > Done. > > https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... > webrtc/modules/audio_coding/main/acm2/dump.proto:128: // SSRCs to use for the > RTX streams. > On 2015/06/24 11:07:55, hlundin-webrtc wrote: > > The comment says SSRCs (in plural). But the field is scalar, right? > > You're right. I'll consider a complete rewrite of the documentation for > RtxConfig. > > https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... > webrtc/modules/audio_coding/main/acm2/dump.proto:142: // The interesting Rtcp > packets are the ones that the sender receives > On 2015/06/24 11:07:55, hlundin-webrtc wrote: > > Nit: Rtcp -> RTCP, when written in comments. > > Done. Should you have uploaded a new patch-set with these comments?
https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:13: On 2015/06/24 11:46:16, stefan-webrtc (holmer) wrote: > nit: Do we need multiple (2) empty lines between each message? Personally I think it improves readability since it is easier to see where each message ends, especially when the different fields of a message are separated by empty lines too. The amount of white space is not specified by the protobuf style guide. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:98: // disabled if there's no config present. On 2015/06/24 11:27:12, ivoc wrote: > One of the experienced protobuf guys told me "Don’t make the presence/absence of > a field semantically meaningful for proto messages". It might be a good idea to > add a boolean to indicate if RTX is enabled or not for clarity. The presence/absence of RTX settings in the C++ code is used to turn RTX on or off. I think it makes sense to use the same principle in the protobuf, i.e. the RTX fields in the *ReceiveStream::Config should be set if and only if an RTX setting is stored in the protobuf. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:102: repeated RtxMap rtx_map = 4; On 2015/06/24 11:27:12, ivoc wrote: > Maybe we can use a map here instead of a seperate message? See > https://developers.google.com/protocol-buffers/docs/proto#maps . I've changed to use the map feature instead of an explicit list of pairs. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:105: repeated RtpHeaderExtension header_extensions = 5; On 2015/06/24 11:27:12, ivoc wrote: > This could be another candidate to use the Map feature I've changed to use map instead of explicit list of pairs. This makes the protobuf specification shorter, but I'm unsure of whether this will have any advantage in the C++ interface since e.g. VideoReceiveStream::Config uses a std::vector of structs/pairs. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:108: repeated DecoderConfig decoders = 6; On 2015/06/24 11:27:12, ivoc wrote: > Maybe another map? Changed, but see previous comment on maps. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:114: optional string name = 1; // required On 2015/06/24 11:46:16, stefan-webrtc (holmer) wrote: > On 2015/06/24 11:27:12, ivoc wrote: > > I think it would be good to keep comments on a seperate line, like in the rest > > of the file. Also a blank line between the different variables would be nice. > > And start with a capital letter and end with a period. For now I made my comments consistent with previous comments in this file, i.e. on a separate line starting with capital letter and ending with a period. The exception is keywords like required and optional, which still use lower case. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:150: enum RtcpMode {KRTCPCOMPOUND = 1; KRTCPREDUCEDSIZE = 2;} On 2015/06/24 11:46:16, stefan-webrtc (holmer) wrote: > Not knowing the style guide for protobufs, is this the suggested way of writing > constants? Seems like a mix of kRtcpCompound and RTCP_COMPOUND. You're right. I'll change to RTCP_COMPOUND, etc.
Please have a second look at my CL. I changed most of the things you requested and explained why I am unsure about the rest.
lgtm with small nits. https://codereview.webrtc.org/1202833003/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/dump.proto:88: 3 empty lines is a bit much, I guess 2 is okay if we do it consistently. https://codereview.webrtc.org/1202833003/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/dump.proto:120: I guess there should be one more empty line here
lgtm https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:13: On 2015/06/26 11:20:26, terelius wrote: > On 2015/06/24 11:46:16, stefan-webrtc (holmer) wrote: > > nit: Do we need multiple (2) empty lines between each message? > > Personally I think it improves readability since it is easier to see where each > message ends, especially when the different fields of a message are separated by > empty lines too. > > The amount of white space is not specified by the protobuf style guide. Acknowledged. https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:114: optional string name = 1; // required On 2015/06/26 11:20:26, terelius wrote: > On 2015/06/24 11:46:16, stefan-webrtc (holmer) wrote: > > On 2015/06/24 11:27:12, ivoc wrote: > > > I think it would be good to keep comments on a seperate line, like in the > rest > > > of the file. Also a blank line between the different variables would be > nice. > > > > And start with a capital letter and end with a period. > > For now I made my comments consistent with previous comments in this file, i.e. > on a separate line starting with capital letter and ending with a period. The > exception is keywords like required and optional, which still use lower case. sgtm https://codereview.webrtc.org/1202833003/diff/50001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/50001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/dump.proto:151: // The interesting RTCP packets are the ones that the sender receives. This isn't entirely true. The sender also sends RTCP to the receiver (sender reports, SR). However, those are of lower (no) interest from a BWE perspective.
lgtm https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... File webrtc/modules/audio_coding/main/acm2/dump.proto (right): https://codereview.webrtc.org/1202833003/diff/1/webrtc/modules/audio_coding/m... webrtc/modules/audio_coding/main/acm2/dump.proto:99: optional RtcpConfig rtcp_config = 3; On 2015/06/24 11:51:18, terelius wrote: > On 2015/06/24 11:07:55, hlundin-webrtc wrote: > > I'm not a protobuf expert, but why is there no field = 2? > > I (mostly) modeled the config fields on the VideoReceiveStream::Config struct, > which has a field local_ssrc (in addition to the remote_ssrc). As far as I can > determine, the local_ssrc is only used for sending RTCP so it made sense to > locate it together with the other RTCP settings. However I still left "space" > for it in the ACMDumpConfigEvent in case this assumption turns out to be > incorrect. Not sure whether this is a good or bad idea. > > The numbers themselves are not important except that each field needs a unique > tag and that tags <16 use a more compact coding than larger tags. Acknowledged.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, henrik.lundin@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1202833003/#ps70001 (title: "Minor fix of whitespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1202833003/70001
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6e355af34802baf5e0a0ad67eb33e58c423a7a39 Cr-Commit-Position: refs/heads/master@{#9519} |
