Piggybacking simulcast id and ALR experiment id into video content type extension.

Piggybacking simulcast id and ALR experiment id into video content type extension.
Use it to slice UMA video receive statis.

BUG=8032
Review-Url: https://codereview.webrtc.org/2986893002
Cr-Commit-Position: refs/heads/master@{#19598}
Committed: https://chromium.googlesource.com/external/webrtc/+/6d5b4d6fe13951a7f3f6ed3d3b29bc601920e381

[ilnik, 2017-07-26 13:07:23]

ilnik@webrtc.org changed reviewers:
+ sprang@webrtc.org

[sprang_webrtc, 2017-07-26 14:13:43]

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
File webrtc/api/video/video_content_type.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.cc:18: */
Remove

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:21: static const VideoContentType
UNSPECIFIED;
Maybe declare these as constexpr instead?

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:24: operator uint8_t() const;
Is this really needed? Would prefer to have the code access content_type (or a
getter method) directly.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:35: void SetSimulcastId(const uint8_t&
simulcast_id);
Pass uint8_t parameters by value rather than reference, here and above.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:40: bool IsScreenshare();
Looks like these three methods can be const?

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:44: uint8_t content_type;
Make it private and have a setter method?

https://codereview.webrtc.org/2986893002/diff/1/webrtc/common_video/include/v...
File webrtc/common_video/include/video_frame.h (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/common_video/include/v...
webrtc/common_video/include/video_frame.h:63: mutable VideoContentType
content_type_ = VideoContentType::UNSPECIFIED;
why mutable?

https://codereview.webrtc.org/2986893002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log2text.cc:467: << static_cast<uint8_t>(
This is probably not what you want; I think it will be printed as single char
with the ascii value of videoContentType...

https://codereview.webrtc.org/2986893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:234: if (data.size() ==
1 && VideoContentType::IsValidContentType(data[0])) {
Maybe be lenient when parsing? Mask out the lowest 6 bit which we know how to
deal with and warn that it contains unknown data.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/modules/video_coding/g...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/modules/video_coding/g...
webrtc/modules/video_coding/generic_encoder.cc:16: #include
"webrtc/modules/pacing/alr_detector.h"
Don't add dependency between modules. If we need this only for the experiment
name, then move that to some shared lower level dependency instead.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:113: // call, any received content type
may be used.
> "is not changed during the call" 
Unfortunately this is not necessarily correct. I think we need to update these
histograms as soon as the content type changes. :/

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:217: for (auto it :
content_specific_stats_) {
const auto& it ?

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:219: auto stats = it.second;
auto is good for |it| here, since its type is kinda verbose.
For |content_type| and |stats| I would prefer explicit types however. It's not
obvious here what type |it.second| has...

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:714: }
return max.value_or(-1);

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.h:104: rtc::Optional<int64_t> max;
Why int64_t instead of int?

[ilnik, 2017-07-26 14:15:23]

https://codereview.webrtc.org/2986893002/diff/20001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2986893002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2707: EXPECT_EQ(1,
metrics::NumSamples(video_prefix_sliced + "EndToEndDelayInMs"));
Need to fix this test. S0 comes at the very end of the metric name.

[ilnik, 2017-07-26 14:49:49]

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
File webrtc/api/video/video_content_type.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.cc:18: */
On 2017/07/26 14:13:42, sprang_webrtc wrote:
> Remove

Done.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:21: static const VideoContentType
UNSPECIFIED;
On 2017/07/26 14:13:42, sprang_webrtc wrote:
> Maybe declare these as constexpr instead?

In that case, I have to initialize it right here, but the class declaration is
yet incomplete here.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:24: operator uint8_t() const;
On 2017/07/26 14:13:42, sprang_webrtc wrote:
> Is this really needed? Would prefer to have the code access content_type (or a
> getter method) directly.

That is done, so we could replace this class with a plain enum class as it was
before, once we decide to remove all this non-standardized staff.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:35: void SetSimulcastId(const uint8_t&
simulcast_id);
On 2017/07/26 14:13:42, sprang_webrtc wrote:
> Pass uint8_t parameters by value rather than reference, here and above.

Done.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:40: bool IsScreenshare();
On 2017/07/26 14:13:42, sprang_webrtc wrote:
> Looks like these three methods can be const?

Done.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/api/video/video_conten...
webrtc/api/video/video_content_type.h:44: uint8_t content_type;
On 2017/07/26 14:13:42, sprang_webrtc wrote:
> Make it private and have a setter method?

For some usages it's easier to have it accessible (like getting a slicing
index).

https://codereview.webrtc.org/2986893002/diff/1/webrtc/common_video/include/v...
File webrtc/common_video/include/video_frame.h (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/common_video/include/v...
webrtc/common_video/include/video_frame.h:63: mutable VideoContentType
content_type_ = VideoContentType::UNSPECIFIED;
On 2017/07/26 14:13:42, sprang_webrtc wrote:
> why mutable?

Because it's modified in VCMEncodedFrameCallback::OnEncodedImage which gets
const reference of a video_frame.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log2text.cc:467: << static_cast<uint8_t>(
On 2017/07/26 14:13:42, sprang_webrtc wrote:
> This is probably not what you want; I think it will be printed as single char
> with the ascii value of videoContentType...

Yep, fixed.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:234: if (data.size() ==
1 && VideoContentType::IsValidContentType(data[0])) {
On 2017/07/26 14:13:43, sprang_webrtc wrote:
> Maybe be lenient when parsing? Mask out the lowest 6 bit which we know how to
> deal with and warn that it contains unknown data.

I think this static one-liner function will be optimized quite well.

https://codereview.webrtc.org/2986893002/diff/1/webrtc/modules/video_coding/g...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/modules/video_coding/g...
webrtc/modules/video_coding/generic_encoder.cc:16: #include
"webrtc/modules/pacing/alr_detector.h"
On 2017/07/26 14:13:43, sprang_webrtc wrote:
> Don't add dependency between modules. If we need this only for the experiment
> name, then move that to some shared lower level dependency instead.

We also need |AlrDetector::ParseAlrSettingsFromFieldTrial()|
This is now non trivial logic. If you have time, could you please move it and
experiment names out of that file somewhere else?

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.cc:113: // call, any received content type
may be used.
On 2017/07/26 14:13:43, sprang_webrtc wrote:
> > "is not changed during the call" 
> Unfortunately this is not necessarily correct. I think we need to update these
> histograms as soon as the content type changes. :/

This is a big problem, I realize now. What if several senders have different
experiment groups. This method should be one big critical section between all
receive statistic proxies.

Experiment should be set to "" at the end of the method. This way it will be
something strange only during reporting these values (I assume nothing else will
report UMA metrics? How about audio?

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2986893002/diff/1/webrtc/video/receive_statisti...
webrtc/video/receive_statistics_proxy.h:104: rtc::Optional<int64_t> max;
On 2017/07/26 14:13:43, sprang_webrtc wrote:
> Why int64_t instead of int?

No particular reason. Changed to int.

[ilnik, 2017-08-23 08:33:46]

Description was changed from

==========
Piggybacking simulcast id and ALR experiment id into video content type
extension.
Use it to slice UMA video receive statis.

BUG=8032
==========

to

==========
Piggybacking simulcast id and ALR experiment id into video content type
extension.
Use it to slice UMA video receive statis.

BUG=8032
==========

[ilnik, 2017-08-23 15:12:56]

ilnik@webrtc.org changed reviewers:
+ philipel@webrtc.org

[ilnik, 2017-08-23 15:13:33]

+philipel for alr_detector configuration-related changes.

[sprang_webrtc, 2017-08-24 09:13:16]

Moving in the direction, think this is going to be great.
Some more comments though

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:29: }
\n

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:37: content_type = (content_type &
0b11000111) | ((experiment_id & 0b111) << 3);
Could you document the format in a comment at the top of this file?
I'm thinking along the lines of how it's done for RTP wire format parsing.
Then define some static mask constants.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:44: uint8_t content_type;
Should we add a SetScreenshare(bool) as well and make |content_type| private?

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/common_video/incl...
File webrtc/common_video/include/video_frame.h (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame.h:57: mutable VideoContentType
content_type_ = VideoContentType::UNSPECIFIED;
Why does it need to be mutable?

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:93: computed_results[experiment_name] =
ret;
Looks like you can drop this insertion?

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.h (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.h:52: int group_id = 0;
Which value means N/A? Maybe easier if 0 is undefined? Could also create a
constant for that value.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:318: // the receive side before
reporting fake field trial.
-field trial
I think it would be ok to not subtract as well. Just use 1-indexing for
experiment ids when reporting.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:386: 
Why isn't this reported in the same way as for the other content type aspects
above?

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:656: }
I think you can just do 

auto it = content_specific_stats_.emplace(last_content_content_type_,
ContentSpecificStats()).first;

Or use a utility method for getting with lazy initalization.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.h:115: SampleCounter
interframe_delay_counter;
Thinking more about this, I think it would be good with a few extra stats. One
that comes to mind:

* decode and/or render fps
* resolution and/or pixel count
* qp and/or bitrate

Let's discuss.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy_unittest.cc:757: }
Please add tests for stats sliced on layer and experiment as well.

[ilnik, 2017-08-25 12:35:07]

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:29: }
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> \n

Done.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:37: content_type = (content_type &
0b11000111) | ((experiment_id & 0b111) << 3);
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> Could you document the format in a comment at the top of this file?
> I'm thinking along the lines of how it's done for RTP wire format parsing.
> Then define some static mask constants.

Done.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:44: uint8_t content_type;
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> Should we add a SetScreenshare(bool) as well and make |content_type| private?

Can't do. It will make this class to be considered complex by chromium style
analyzer, which makes it and everything it's included in (e.g. RTPVideoHeader
and bunch others) to require a out-of-line constructors.

This will require a broad refactoring of core rtp related code. I would rather
not do that for such a simple reason.

SetScreenshare is not needed, as this struct is treated as an enum in encoders:
it's simply initialized to UNSPECIFIED or SCREENSHARE.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/common_video/incl...
File webrtc/common_video/include/video_frame.h (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame.h:57: mutable VideoContentType
content_type_ = VideoContentType::UNSPECIFIED;
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> Why does it need to be mutable?

Because it's modified in the VCMEncodedFrameCallback, yet frame there is a const
reference.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:93: computed_results[experiment_name] =
ret;
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> Looks like you can drop this insertion?

No. Currently the function may exit below if it fails to parse a string.
In that case map will already contain correct empty value. Future calls will
never attempt to parse the string again.

If this insertion is removed, incorrect strings will be parsed on each call.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.h (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.h:52: int group_id = 0;
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> Which value means N/A? Maybe easier if 0 is undefined? Could also create a
> constant for that value.

Rephrased the comment. It doesn't matter which value will be used as N/A. All
maters is that we have 3 bits and one reserved value. It leaves 7 possible
groups here.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:318: // the receive side before
reporting fake field trial.
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> -field trial
> I think it would be ok to not subtract as well. Just use 1-indexing for
> experiment ids when reporting.

Come on, we are writing in C++, not pascal. Things should be numbered from 0 =)

Also, updated the comment to not mention fake field trials, not used now.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:386: 
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> Why isn't this reported in the same way as for the other content type aspects
> above?

Because currently we can't get slice on both simulcast id and experiment id.
Thus, if the are slicing on experiment id, we aggregate all the different
spatial layers. There's a problem if the stream received both screenshare and
video during the call, but it never happens currently.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:656: }
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> I think you can just do 
> 
> auto it = content_specific_stats_.emplace(last_content_content_type_,
> ContentSpecificStats()).first;
> 
> Or use a utility method for getting with lazy initalization.

Redone using plain [] operator. According to documentation, if there's no entry
for the key, default will be constructed and reference to that returned. Just
that I need.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.h:115: SampleCounter
interframe_delay_counter;
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> Thinking more about this, I think it would be good with a few extra stats. One
> that comes to mind:
> 
> * decode and/or render fps
> * resolution and/or pixel count
> * qp and/or bitrate
> 
> Let's discuss.

qp, media bitrate, and resolution are added sliced on simulcast or experiment.
Decode fps can be inferred from interframe delay.

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/2986893002/diff/120001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy_unittest.cc:757: }
On 2017/08/24 09:13:16, sprang_webrtc wrote:
> Please add tests for stats sliced on layer and experiment as well.

Done.

[ilnik, 2017-08-25 13:43:18]

The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-25 13:43:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-25 13:47:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-25 13:47:01]

Dry run: Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/24325)

[sprang_webrtc, 2017-08-25 13:56:29]

Looks good, but see comment about content type changing during call.

https://codereview.webrtc.org/2986893002/diff/140001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/140001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:69: std::string
UmaExperimentSuffixConentType(VideoContentType content_type) {
nit: UmaExperimentSuffixForContentType

https://codereview.webrtc.org/2986893002/diff/140001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:224:
UmaExperimentSuffixConentType(last_content_type_);
I think these might unfortunately change during the call. We might not
necessarily negotiate a new receive stream when the source changes. :/

[ilnik, 2017-08-25 14:14:27]

https://codereview.webrtc.org/2986893002/diff/140001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/140001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:69: std::string
UmaExperimentSuffixConentType(VideoContentType content_type) {
On 2017/08/25 13:56:29, sprang_webrtc wrote:
> nit: UmaExperimentSuffixForContentType

Done.

https://codereview.webrtc.org/2986893002/diff/140001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:224:
UmaExperimentSuffixConentType(last_content_type_);
On 2017/08/25 13:56:29, sprang_webrtc wrote:
> I think these might unfortunately change during the call. We might not
> necessarily negotiate a new receive stream when the source changes. :/

Done.

[ilnik, 2017-08-25 16:29:36]

The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-25 16:29:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-25 16:34:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-25 16:34:34]

Dry run: Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/21760)
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/11864)

[sprang_webrtc, 2017-08-28 07:58:30]

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:139: for (auto it :
content_specific_stats_) {
I think it would be good with a comment outlining clearly how we're trying to
slice this. For instance, for the metric Foo we get the following slices:

WebRTC.Video.Foo.S[0-3]
WebRTC.Video.Foo.ExperimentGroup[0-7]
WebRTC.Video.Screenshare.Foo.S[0-3]
WebRTC.Video.Screenshare.Foo.ExperimentGroup[0-7]

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:150: std::string uma_suffix =
UmaSpatialSuffixForContentType(content_type);
Will this work? If we slice on experiment and spatial layer separately (per
content type), say we want to show the stats for spatial layer 1, shouldn't we
then aggregate all content specific stats where spatial layer is 1?

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:153: // names. Since experiment can't
change during call, we could report 4*2
Experiments can change during a call....

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:156: int idx =
content_type.content_type & 0x07;
This is starting to get cluttered. Could we move the actual histogram reporting
to a separate method, taking a ContentSpecificStats reference and prefix/suffix
as argument?

Or some way to merge it with the reporting below, so that we only report say
EndToEndDelayInMs in only one place.

[ilnik, 2017-08-28 08:02:42]

The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-28 08:02:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-28 08:08:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-28 08:09:00]

Dry run: Try jobs failed on following builders:
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/29011)

[ilnik, 2017-08-28 14:54:34]

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:139: for (auto it :
content_specific_stats_) {
On 2017/08/28 07:58:30, sprang_webrtc wrote:
> I think it would be good with a comment outlining clearly how we're trying to
> slice this. For instance, for the metric Foo we get the following slices:
> 
> WebRTC.Video.Foo.S[0-3]
> WebRTC.Video.Foo.ExperimentGroup[0-7]
> WebRTC.Video.Screenshare.Foo.S[0-3]
> WebRTC.Video.Screenshare.Foo.ExperimentGroup[0-7]

Should be more clear now.

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:150: std::string uma_suffix =
UmaSpatialSuffixForContentType(content_type);
On 2017/08/28 07:58:30, sprang_webrtc wrote:
> Will this work? If we slice on experiment and spatial layer separately (per
> content type), say we want to show the stats for spatial layer 1, shouldn't we
> then aggregate all content specific stats where spatial layer is 1?

Done.

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:153: // names. Since experiment can't
change during call, we could report 4*2
On 2017/08/28 07:58:30, sprang_webrtc wrote:
> Experiments can change during a call....

Done.

https://codereview.webrtc.org/2986893002/diff/200001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:156: int idx =
content_type.content_type & 0x07;
On 2017/08/28 07:58:30, sprang_webrtc wrote:
> This is starting to get cluttered. Could we move the actual histogram
reporting
> to a separate method, taking a ContentSpecificStats reference and
prefix/suffix
> as argument?
> 
> Or some way to merge it with the reporting below, so that we only report say
> EndToEndDelayInMs in only one place.

Reporting below is removed (except for 3 already existing metrics to not change
them).

[ilnik, 2017-08-28 15:12:55]

The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-28 15:12:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[sprang_webrtc, 2017-08-28 16:25:03]

Looking much better! Just a few more comments, most of them nits.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:126: << stream_duration_sec;
Check that we have received at least one frame before reporting this. I think
it's all too common that applications create a bunch of streams that are never
used.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:218: // Calculate experiment specific
metrics:
: ?

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:225: // calculate aggregated metrics
(no suffixes. Aggregated on everything).
nit: s/calculate/Calculate

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:229:
aggregated_stats[content_type].Add(it.second);
We shouldn't do the addition if ss == 0 && exp == 0, right? Otherwise we add it
to itself.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:233: // for the metric Foo we report
the following slices:
nit: Capital F

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:722:
++content_specific_stats->frame_counts.delta_frames;
nit: Use {} for if/else

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:764: if (!max || *max < sample) {
nit: maybe more readable with if (!max || sample > *max)

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:788: }
nit: return max.value_or(-1);

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy_unittest.cc:909: }
Can you verify the actual values for any of these metrics?

[ilnik, 2017-08-29 07:47:16]

ilnik@webrtc.org changed reviewers:
+ tommi@webrtc.org

[ilnik, 2017-08-29 07:50:32]

ilnik@webrtc.org changed reviewers:
+ asapersson@webrtc.org

[ilnik, 2017-08-29 07:52:07]

+tommi for:
webrtc/api/
webrtc/BUILD.gn
trivial changes in webrtc/modules/rtp_rtcp
trivial changes in webrtc/common_video/include/video_frame.h

+asapersson for:
webrtc/system_wrappers/include/metrics.h

+philipel for:
webrtc/modules/pacing/alr_detector.*

[ilnik, 2017-08-29 07:56:27]

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:126: << stream_duration_sec;
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> Check that we have received at least one frame before reporting this. I think
> it's all too common that applications create a bunch of streams that are never
> used.

Done.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:218: // Calculate experiment specific
metrics:
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> : ?

Done.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:225: // calculate aggregated metrics
(no suffixes. Aggregated on everything).
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> nit: s/calculate/Calculate

Done.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:229:
aggregated_stats[content_type].Add(it.second);
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> We shouldn't do the addition if ss == 0 && exp == 0, right? Otherwise we add
it
> to itself.

We add to the separate map. We need to make this addition. Making it in place is
tricky as we need to be very careful with the order of additions.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:233: // for the metric Foo we report
the following slices:
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> nit: Capital F

Done.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:722:
++content_specific_stats->frame_counts.delta_frames;
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> nit: Use {} for if/else

Done.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:764: if (!max || *max < sample) {
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> nit: maybe more readable with if (!max || sample > *max)

Done.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:788: }
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> nit: return max.value_or(-1);

Done.

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy_unittest.cc:909: }
On 2017/08/28 16:25:02, sprang_webrtc wrote:
> Can you verify the actual values for any of these metrics?

Done.

[sprang_webrtc, 2017-08-29 08:04:38]

lgtm

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2986893002/diff/260001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:229:
aggregated_stats[content_type].Add(it.second);
On 2017/08/29 07:56:27, ilnik wrote:
> On 2017/08/28 16:25:02, sprang_webrtc wrote:
> > We shouldn't do the addition if ss == 0 && exp == 0, right? Otherwise we add
> it
> > to itself.
> 
> We add to the separate map. We need to make this addition. Making it in place
is
> tricky as we need to be very careful with the order of additions.

Ah, you're right. I misread.

[ilnik, 2017-08-29 08:13:19]

The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-29 08:13:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-29 08:41:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-29 08:41:25]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-08-29 09:25:45]

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.cc (right):

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:39: const VideoContentType
VideoContentType::UNSPECIFIED = VideoContentType({0});
if for some reason we can't use enum, can we use constexpr here? (global
construction will cause the cl to be reverted)

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:21: static const VideoContentType
UNSPECIFIED;
this isn't an enum, so should be kUnspecified.

actually, why isn't it an enum? (looks like it was before)

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:44: uint8_t content_type;
make this private?  (why a struct and not class?)

[ilnik, 2017-08-29 10:26:15]

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.cc (right):

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:39: const VideoContentType
VideoContentType::UNSPECIFIED = VideoContentType({0});
On 2017/08/29 09:25:45, tommi wrote:
> if for some reason we can't use enum, can we use constexpr here? (global
> construction will cause the cl to be reverted)

Done. However, it's not constexpr in the .h file, is it fine? It can't be set
there as constexpr because it will need initializer there, yet at that place the
class is still incomplete and can't be used.

One solution would be to use uint8_t constants, but the problem is that we can't
use them for initializing VideoContentType because we can't define constructor
or this struct will become complex again.

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:21: static const VideoContentType
UNSPECIFIED;
On 2017/08/29 09:25:45, tommi wrote:
> this isn't an enum, so should be kUnspecified.
> 
> actually, why isn't it an enum? (looks like it was before)

It's not enum, because there're 64 different combinations of content type,
simulcast id and experiment id now mixed together. So this is now a class
mimicking an enum. Extra-options agnostic clients could just use it as an enum
class.

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:44: uint8_t content_type;
On 2017/08/29 09:25:45, tommi wrote:
> make this private?  (why a struct and not class?)

Can't make it private and class. It will make this class 'complex', which will
make everything with it be 'complex' too (RTPVideoHeader and bunch of other
header related structs). Then there will be a lot of compile errors like this:
"Chromium/Style: complex class should have out of line constructor". This will
require broad refactoring of a lot of API and core structs just for making one
member private. 

Then I am a not sure if it will not break everything. There's a lot of code
using these header structs as POD, memseting them and so on.

[philipel, 2017-08-29 11:25:49]

https://codereview.webrtc.org/2986893002/diff/300001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2986893002/diff/300001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:85: static std::map<const char*,
Is this thread safe? ParseAlrSettingsFromFieldTrial is called a few times at the
beginning of the call, so I can't really see the benefit of memoizing this.

https://codereview.webrtc.org/2986893002/diff/300001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:90: if (it != computed_results.end()) {
nit: remove {}

[kwiberg-webrtc, 2017-08-29 11:44:27]

kwiberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[kwiberg-webrtc, 2017-08-29 11:44:29]

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:21: static const VideoContentType
UNSPECIFIED;
On 2017/08/29 10:26:14, ilnik wrote:
> On 2017/08/29 09:25:45, tommi wrote:
> > this isn't an enum, so should be kUnspecified.
> > 
> > actually, why isn't it an enum? (looks like it was before)
> 
> It's not enum, because there're 64 different combinations of content type,
> simulcast id and experiment id now mixed together. So this is now a class
> mimicking an enum. Extra-options agnostic clients could just use it as an enum
> class.

Even with many combinations, it should be convenient to make it an enum class,
if you overload | and & and/or provide convenience functions:

  enum class VideoContentType : uint8_t {
    kSomeFlag = 1,
    kSomeOtherFlag = 2,
    kYetAnotherFlag = 4,
  };

  VideoContentType operator|(VideoContentType a,
                             VideoContentType b) {
    return static_cast<VideoContentType>(
      static_cast<uint8_t>(a) | static_cast<uint8_t>(b));
  }

  VideoContentType SetSomeOtherFlag(VideoContentType a) {
    return a | VideoContentType::kSomeOtherFlag;
  }

You get free functions rather than methods, but I'd say that's an OK tradeoff.

[ilnik, 2017-08-29 11:48:16]

philipel, please check also latest changes in generic_encoder.(cc|h) and how
AlrDetector is used there.

https://codereview.webrtc.org/2986893002/diff/300001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2986893002/diff/300001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:85: static std::map<const char*,
On 2017/08/29 11:25:49, philipel wrote:
> Is this thread safe? ParseAlrSettingsFromFieldTrial is called a few times at
the
> beginning of the call, so I can't really see the benefit of memoizing this.

Removed memoization.

https://codereview.webrtc.org/2986893002/diff/300001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:90: if (it != computed_results.end()) {
On 2017/08/29 11:25:49, philipel wrote:
> nit: remove {}

Removed now.

[ilnik, 2017-08-29 12:01:57]

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:21: static const VideoContentType
UNSPECIFIED;
On 2017/08/29 11:44:28, kwiberg-webrtc wrote:
> On 2017/08/29 10:26:14, ilnik wrote:
> > On 2017/08/29 09:25:45, tommi wrote:
> > > this isn't an enum, so should be kUnspecified.
> > > 
> > > actually, why isn't it an enum? (looks like it was before)
> > 
> > It's not enum, because there're 64 different combinations of content type,
> > simulcast id and experiment id now mixed together. So this is now a class
> > mimicking an enum. Extra-options agnostic clients could just use it as an
enum
> > class.
> 
> Even with many combinations, it should be convenient to make it an enum class,
> if you overload | and & and/or provide convenience functions:
> 
>   enum class VideoContentType : uint8_t {
>     kSomeFlag = 1,
>     kSomeOtherFlag = 2,
>     kYetAnotherFlag = 4,
>   };
> 
>   VideoContentType operator|(VideoContentType a,
>                              VideoContentType b) {
>     return static_cast<VideoContentType>(
>       static_cast<uint8_t>(a) | static_cast<uint8_t>(b));
>   }
> 
>   VideoContentType SetSomeOtherFlag(VideoContentType a) {
>     return a | VideoContentType::kSomeOtherFlag;
>   }
> 
> You get free functions rather than methods, but I'd say that's an OK tradeoff.

Can you elaborate more, how do I, for example, combine two 4-bit values in enum?
Do I define some dummy enum value equal to 255 to fix uint8_t as underlying type
and then simply use bit arithmetic to set values as I want? Is it safe to save
non-listed values in the enum?

[philipel, 2017-08-29 12:06:15]

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:13: #include <map>
Not used.

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:193: experiment_groups_[0] =
experiment_settings->group_id + 1;
Add comment about + 1.

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:89: uint8_t experiment_groups_[2];
I think you will only need an uint8_t since the both experiments can't be active
at the same time.

[kwiberg-webrtc, 2017-08-29 12:12:45]

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:21: static const VideoContentType
UNSPECIFIED;
On 2017/08/29 12:01:57, ilnik wrote:
> On 2017/08/29 11:44:28, kwiberg-webrtc wrote:
> > On 2017/08/29 10:26:14, ilnik wrote:
> > > On 2017/08/29 09:25:45, tommi wrote:
> > > > this isn't an enum, so should be kUnspecified.
> > > > 
> > > > actually, why isn't it an enum? (looks like it was before)
> > > 
> > > It's not enum, because there're 64 different combinations of content type,
> > > simulcast id and experiment id now mixed together. So this is now a class
> > > mimicking an enum. Extra-options agnostic clients could just use it as an
> enum
> > > class.
> > 
> > Even with many combinations, it should be convenient to make it an enum
class,
> > if you overload | and & and/or provide convenience functions:
> > 
> >   enum class VideoContentType : uint8_t {
> >     kSomeFlag = 1,
> >     kSomeOtherFlag = 2,
> >     kYetAnotherFlag = 4,
> >   };
> > 
> >   VideoContentType operator|(VideoContentType a,
> >                              VideoContentType b) {
> >     return static_cast<VideoContentType>(
> >       static_cast<uint8_t>(a) | static_cast<uint8_t>(b));
> >   }
> > 
> >   VideoContentType SetSomeOtherFlag(VideoContentType a) {
> >     return a | VideoContentType::kSomeOtherFlag;
> >   }
> > 
> > You get free functions rather than methods, but I'd say that's an OK
tradeoff.
> 
> Can you elaborate more, how do I, for example, combine two 4-bit values in
enum?
> Do I define some dummy enum value equal to 255 to fix uint8_t as underlying
type
> and then simply use bit arithmetic to set values as I want? Is it safe to save
> non-listed values in the enum?

http://en.cppreference.com/w/cpp/language/enum says:

  "Values of integer, floating-point, and other enumeration types can be
converted, such as by static_cast, to any enumeration type. The result is
unspecified if the value, converted to the enumeration's underlying type, is out
of this enumeration's range. If the underlying type is fixed, the range is the
range of the underlying type. If the underlying type is not fixed, the range is
all values possible for the smallest bit field large enough to hold all
enumerators of the target enumeration. Note that the value after such conversion
may not necessarily equal any of the named enumerators defined for the type."

So by fixing the range to uint8_t, as I did in the example in my previous
comment, we ensure that any integer in the range 0-255 is valid.

[ilnik, 2017-08-29 12:41:22]

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/280001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:21: static const VideoContentType
UNSPECIFIED;
On 2017/08/29 12:12:45, kwiberg-webrtc wrote:
> On 2017/08/29 12:01:57, ilnik wrote:
> > On 2017/08/29 11:44:28, kwiberg-webrtc wrote:
> > > On 2017/08/29 10:26:14, ilnik wrote:
> > > > On 2017/08/29 09:25:45, tommi wrote:
> > > > > this isn't an enum, so should be kUnspecified.
> > > > > 
> > > > > actually, why isn't it an enum? (looks like it was before)
> > > > 
> > > > It's not enum, because there're 64 different combinations of content
type,
> > > > simulcast id and experiment id now mixed together. So this is now a
class
> > > > mimicking an enum. Extra-options agnostic clients could just use it as
an
> > enum
> > > > class.
> > > 
> > > Even with many combinations, it should be convenient to make it an enum
> class,
> > > if you overload | and & and/or provide convenience functions:
> > > 
> > >   enum class VideoContentType : uint8_t {
> > >     kSomeFlag = 1,
> > >     kSomeOtherFlag = 2,
> > >     kYetAnotherFlag = 4,
> > >   };
> > > 
> > >   VideoContentType operator|(VideoContentType a,
> > >                              VideoContentType b) {
> > >     return static_cast<VideoContentType>(
> > >       static_cast<uint8_t>(a) | static_cast<uint8_t>(b));
> > >   }
> > > 
> > >   VideoContentType SetSomeOtherFlag(VideoContentType a) {
> > >     return a | VideoContentType::kSomeOtherFlag;
> > >   }
> > > 
> > > You get free functions rather than methods, but I'd say that's an OK
> tradeoff.
> > 
> > Can you elaborate more, how do I, for example, combine two 4-bit values in
> enum?
> > Do I define some dummy enum value equal to 255 to fix uint8_t as underlying
> type
> > and then simply use bit arithmetic to set values as I want? Is it safe to
save
> > non-listed values in the enum?
> 
> http://en.cppreference.com/w/cpp/language/enum says:
> 
>   "Values of integer, floating-point, and other enumeration types can be
> converted, such as by static_cast, to any enumeration type. The result is
> unspecified if the value, converted to the enumeration's underlying type, is
out
> of this enumeration's range. If the underlying type is fixed, the range is the
> range of the underlying type. If the underlying type is not fixed, the range
is
> all values possible for the smallest bit field large enough to hold all
> enumerators of the target enumeration. Note that the value after such
conversion
> may not necessarily equal any of the named enumerators defined for the type."
> 
> So by fixing the range to uint8_t, as I did in the example in my previous
> comment, we ensure that any integer in the range 0-255 is valid.

Thank you! That was misconception on my side. Please take a look at updated
version.

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/pacing/al...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/pacing/al...
webrtc/modules/pacing/alr_detector.cc:13: #include <map>
On 2017/08/29 12:06:15, philipel wrote:
> Not used.

Done.

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:193: experiment_groups_[0] =
experiment_settings->group_id + 1;
On 2017/08/29 12:06:15, philipel wrote:
> Add comment about + 1.

Done.

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/2986893002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:89: uint8_t experiment_groups_[2];
On 2017/08/29 12:06:15, philipel wrote:
> I think you will only need an uint8_t since the both experiments can't be
active
> at the same time.

I think no, because a single VCMReceiverCallback may is used for several
streams. It's safer to take screenshare status directly from the frame in the
callback.

[ilnik, 2017-08-29 12:42:25]

The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-29 12:42:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[philipel, 2017-08-29 13:04:00]

lgtm with one final comment.

https://codereview.webrtc.org/2986893002/diff/340001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2986893002/diff/340001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:332: // We count simulcast
streams from 1 on the wire.
I think you should do the +1 here where it is explained. Also, if 0 means no
experiment then comment about that here as well.

[åsapersson, 2017-08-29 13:07:10]

webrtc/system_wrappers/include/metrics.h: lgtm (but note I am not an owner)

[ilnik, 2017-08-29 13:28:09]

tommi, kwiberg,

could you also take a quick look at the webrtc/system_wrappers/include/metrics.h
It's been approved by asapersson already.

VideoContentType is now simple enum.

[ilnik, 2017-08-29 13:28:23]

https://codereview.webrtc.org/2986893002/diff/340001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2986893002/diff/340001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:332: // We count simulcast
streams from 1 on the wire.
On 2017/08/29 13:04:00, philipel wrote:
> I think you should do the +1 here where it is explained. Also, if 0 means no
> experiment then comment about that here as well.

Clarified comment.

[kwiberg-webrtc, 2017-08-29 20:58:54]

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.cc (right):

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:13: // VideoContentType stored as a
single byte, which is sent other the network.
over

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:36: static const uint8_t
kExperimentBitsSize = 3;
constexpr?

Also, put these in a namespace. An unnamed namespace inside
webrtc::VideoContentTypeHelpers?

Also, define the masks in therm of the other two?

  static constexpr uint8_t kExperimentBitsMask = ((1u << kExperimentBitsSize) -
1) << kExperimentShift;

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:42: // Store in bits 3-5.
2-4

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:52: // Store in bits 1-2.
5-6

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:23: namespace VideoContentTypeHelpers {
"Namespace names are all lower-case." See
https://google.github.io/styleguide/cppguide.html#Namespace_Names

However, it may make more sense to make a struct with static methods instead of
a namespace; you can't open it up later and add more stuff, and there are fewer
dangerous name lookup rules.

[ilnik, 2017-08-30 07:36:07]

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.cc (right):

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:13: // VideoContentType stored as a
single byte, which is sent other the network.
On 2017/08/29 20:58:53, kwiberg-webrtc wrote:
> over

Done.

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:36: static const uint8_t
kExperimentBitsSize = 3;
On 2017/08/29 20:58:53, kwiberg-webrtc wrote:
> constexpr?
> 
> Also, put these in a namespace. An unnamed namespace inside
> webrtc::VideoContentTypeHelpers?
> 
> Also, define the masks in therm of the other two?
> 
>   static constexpr uint8_t kExperimentBitsMask = ((1u << kExperimentBitsSize)
-
> 1) << kExperimentShift;

Done.

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:42: // Store in bits 3-5.
On 2017/08/29 20:58:53, kwiberg-webrtc wrote:
> 2-4

Done.

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.cc:52: // Store in bits 1-2.
On 2017/08/29 20:58:53, kwiberg-webrtc wrote:
> 5-6

Done.

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/360001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:23: namespace VideoContentTypeHelpers {
On 2017/08/29 20:58:53, kwiberg-webrtc wrote:
> "Namespace names are all lower-case." See
> https://google.github.io/styleguide/cppguide.html#Namespace_Names
> 
> However, it may make more sense to make a struct with static methods instead
of
> a namespace; you can't open it up later and add more stuff, and there are
fewer
> dangerous name lookup rules.

Great idea. Done.

[ilnik, 2017-08-30 08:52:51]

The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-30 08:52:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-30 09:25:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-30 09:25:52]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-08-30 09:48:08]

lgtm w/one comment.  We've moved away from using classes as namespaces and
instead simply use namespaces. We've had to clean up many occurrences of the
pattern in the code in the past few years, so unless there's a good reason to
use the class approach here, I'd prefer using a namespace.

Other than that, good to go as far as I'm concerned and thanks kwiberg@ for all
the input.

https://codereview.webrtc.org/2986893002/diff/380001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/380001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:23: struct VideoContentTypeHelpers {
nit: should this be a namespace instead of class+statics?

[ilnik, 2017-08-30 10:00:48]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-08-30 10:00:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
philipel@webrtc.org, asapersson@webrtc.org, tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2986893002/#ps400001 (title:
"use namespace instead of struct")

[ilnik, 2017-08-30 10:00:57]

https://codereview.webrtc.org/2986893002/diff/380001/webrtc/api/video/video_c...
File webrtc/api/video/video_content_type.h (right):

https://codereview.webrtc.org/2986893002/diff/380001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:23: struct VideoContentTypeHelpers {
On 2017/08/30 09:48:08, tommi wrote:
> nit: should this be a namespace instead of class+statics?

Done.

[commit-bot: I haz the power, 2017-08-30 10:01:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-30 10:32:15]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1504087248481400,
"parent_rev": "057e63a0ac981e555a51683fda63572aff9b133c", "commit_rev":
"6d5b4d6fe13951a7f3f6ed3d3b29bc601920e381"}

[commit-bot: I haz the power, 2017-08-30 10:32:22]

Description was changed from

==========
Piggybacking simulcast id and ALR experiment id into video content type
extension.
Use it to slice UMA video receive statis.

BUG=8032
==========

to

==========
Piggybacking simulcast id and ALR experiment id into video content type
extension.
Use it to slice UMA video receive statis.

BUG=8032

Review-Url: https://codereview.webrtc.org/2986893002
Cr-Commit-Position: refs/heads/master@{#19598}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6d5b4d6fe13951a7f3f6ed3d3...
==========

[commit-bot: I haz the power, 2017-08-30 10:32:23]

Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/external/webrtc/+/6d5b4d6fe13951a7f3f6ed3d3...