Add histogram for percentage of sent frames that are limited in resolution due to quality.

Add histogram for percentage of sent frames that are limited in resolution due to quality:
- "WebRTC.Video.QualityLimitedResolutionInPercent"

and if a frame is downscaled, the average number of times the frame is downscaled:
- "WebRTC.Video.QualityLimitedResolutionDownscales"

BUG=

Committed: https://crrev.com/4306fc70d778887d8a2ea71b6f4bc6a12d1d9447
Cr-Commit-Position: refs/heads/master@{#10319}

[åsapersson, 2015-09-11 10:58:41]

asapersson@webrtc.org changed reviewers:
+ pbos@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2015-09-16 07:36:58]

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:1019:
quality_scaler_.downscale_shift() > 0;
Would it be more useful to log the number of times we have scaled down?

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:67:
quality_limited_sent_frame_counter_.Percent(kMinRequiredSamples);
I think "sent" is implied here and that you can leave it out.

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.h:105: struct BoolSampleCounter {
I'd prefer this to be a class I think. Same with SampleCounter above.

[stefan-webrtc, 2015-09-16 07:37:35]

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:1019:
quality_scaler_.downscale_shift() > 0;
On 2015/09/16 07:36:57, stefan-webrtc (holmer) wrote:
> Would it be more useful to log the number of times we have scaled down?

To clarify, I think it would be useful to know as it would help us find
regressions where we adapt down too many times.

[åsapersson, 2015-09-17 14:24:45]

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:1019:
quality_scaler_.downscale_shift() > 0;
On 2015/09/16 07:37:35, stefan-webrtc (holmer) wrote:
> On 2015/09/16 07:36:57, stefan-webrtc (holmer) wrote:
> > Would it be more useful to log the number of times we have scaled down?
> 
> To clarify, I think it would be useful to know as it would help us find
> regressions where we adapt down too many times.

Added the average number of times a frame is scaled down for scaled down frames,
ptal.

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:67:
quality_limited_sent_frame_counter_.Percent(kMinRequiredSamples);
On 2015/09/16 07:36:57, stefan-webrtc (holmer) wrote:
> I think "sent" is implied here and that you can leave it out.

Done.

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1325153009/diff/40001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.h:105: struct BoolSampleCounter {
On 2015/09/16 07:36:57, stefan-webrtc (holmer) wrote:
> I'd prefer this to be a class I think. Same with SampleCounter above.

Done in another cl (bw limited stats cl).

[åsapersson, 2015-09-18 10:59:04]

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

[stefan-webrtc, 2015-09-23 15:08:08]

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:1013:
encoded_images_[encoder_idx].adapt_reason_.quality_available =
quality_scaling_available?

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:72: int downscales =
quality_downscales_counter_.Avg(kMinRequiredSamples);
What will this average represent? The average number of downscales per second
over a call? That may be a bit confusing since the framerate will affect that
number. Maybe we should log the average number of downscales per frame?

[åsapersson, 2015-09-24 08:16:17]

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:1013:
encoded_images_[encoder_idx].adapt_reason_.quality_available =
On 2015/09/23 15:08:07, stefan-webrtc (holmer) wrote:
> quality_scaling_available?

In other cl, adapt_reason.bw_available, keep to be similar?

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:72: int downscales =
quality_downscales_counter_.Avg(kMinRequiredSamples);
On 2015/09/23 15:08:07, stefan-webrtc (holmer) wrote:
> What will this average represent? The average number of downscales per second
> over a call? That may be a bit confusing since the framerate will affect that
> number. Maybe we should log the average number of downscales per frame?

If a frame is downscaled, |quality_downscales_counter_| is updated with the
number of downscales for that frame. The average is recorded: average number of
downscales for scaled-downed frames.

[stefan-webrtc, 2015-09-28 13:44:29]

lgtm

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:1013:
encoded_images_[encoder_idx].adapt_reason_.quality_available =
On 2015/09/24 08:16:17, åsapersson wrote:
> On 2015/09/23 15:08:07, stefan-webrtc (holmer) wrote:
> > quality_scaling_available?
> 
> In other cl, adapt_reason.bw_available, keep to be similar?

I guess it works since itäs part of adapt_reason_

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:72: int downscales =
quality_downscales_counter_.Avg(kMinRequiredSamples);
On 2015/09/24 08:16:17, åsapersson wrote:
> On 2015/09/23 15:08:07, stefan-webrtc (holmer) wrote:
> > What will this average represent? The average number of downscales per
second
> > over a call? That may be a bit confusing since the framerate will affect
that
> > number. Maybe we should log the average number of downscales per frame?
> 
> If a frame is downscaled, |quality_downscales_counter_| is updated with the
> number of downscales for that frame. The average is recorded: average number
of
> downscales for scaled-downed frames. 

Ok! I was mostly wondering if it was an average over a time interval or over a
certain number of frames. Seems like it is the latter, which is good.

[pbos-webrtc, 2015-09-28 14:30:18]

lgtm

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:177: if (encoded_image._length > 0) {
!= kSkipFrame please

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.h:107: void Add(bool sample);
Should these counter classes be shared? I don't really mind if
send_statistics_proxy.h includes receive_statistics_proxy.h or vice versa. WDYT?

[mflodman, 2015-10-02 09:21:51]

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.h:107: void Add(bool sample);
On 2015/09/28 14:30:18, pbos-webrtc wrote:
> Should these counter classes be shared? I don't really mind if
> send_statistics_proxy.h includes receive_statistics_proxy.h or vice versa.
WDYT?

Or move it to a shared header.

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video_frame.h#newc...
webrtc/video_frame.h:189: bool quality_available;  // True if
|quality_resolution_downscales| is
I don't fully understand this comment. Is it true if we might downscale or if it
has been downscaled?

[åsapersson, 2015-10-06 11:26:39]

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.h:107: void Add(bool sample);
On 2015/10/02 09:21:51, mflodman wrote:
> On 2015/09/28 14:30:18, pbos-webrtc wrote:
> > Should these counter classes be shared? I don't really mind if
> > send_statistics_proxy.h includes receive_statistics_proxy.h or vice versa.
> WDYT?
> 
> Or move it to a shared header.

Ok this part has been submitted in another cl. Can I fix this in a follow up cl?

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video_frame.h#newc...
webrtc/video_frame.h:189: bool quality_available;  // True if
|quality_resolution_downscales| is
On 2015/10/02 09:21:51, mflodman wrote:
> I don't fully understand this comment. Is it true if we might downscale or if
it
> has been downscaled?

It is true if we might downscale (the quality scaler is enabled and the number
of downscales (|quality_resolution_downscales|) is provided).

|quality_resolution_downscales| should be ignored if |quality_available| is
false.

[mflodman, 2015-10-07 11:18:59]

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.h:107: void Add(bool sample);
On 2015/10/06 11:26:39, åsapersson wrote:
> On 2015/10/02 09:21:51, mflodman wrote:
> > On 2015/09/28 14:30:18, pbos-webrtc wrote:
> > > Should these counter classes be shared? I don't really mind if
> > > send_statistics_proxy.h includes receive_statistics_proxy.h or vice versa.
> > WDYT?
> > 
> > Or move it to a shared header.
> 
> Ok this part has been submitted in another cl. Can I fix this in a follow up
cl?

Absolutely.

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video_frame.h#newc...
webrtc/video_frame.h:189: bool quality_available;  // True if
|quality_resolution_downscales| is
On 2015/10/06 11:26:39, åsapersson wrote:
> On 2015/10/02 09:21:51, mflodman wrote:
> > I don't fully understand this comment. Is it true if we might downscale or
if
> it
> > has been downscaled?
> 
> It is true if we might downscale (the quality scaler is enabled and the number
> of downscales (|quality_resolution_downscales|) is provided).
> 
> |quality_resolution_downscales| should be ignored if |quality_available| is
> false.

Does it make sense to skip the bool and do something like:
quality_resolution_downscales = -1 -> Not available.
quality_resolution_downscales = 0 -> Available but hasn't happened.
quality_resolution_downscales = x -> number of downscales

Or do you prefer keeping the bool?

[åsapersson, 2015-10-09 12:17:07]

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/1325153009/diff/60001/webrtc/video_frame.h#newc...
webrtc/video_frame.h:189: bool quality_available;  // True if
|quality_resolution_downscales| is
On 2015/10/07 11:18:58, mflodman wrote:
> On 2015/10/06 11:26:39, åsapersson wrote:
> > On 2015/10/02 09:21:51, mflodman wrote:
> > > I don't fully understand this comment. Is it true if we might downscale or
> if
> > it
> > > has been downscaled?
> > 
> > It is true if we might downscale (the quality scaler is enabled and the
number
> > of downscales (|quality_resolution_downscales|) is provided).
> > 
> > |quality_resolution_downscales| should be ignored if |quality_available| is
> > false.
> 
> Does it make sense to skip the bool and do something like:
> quality_resolution_downscales = -1 -> Not available.
> quality_resolution_downscales = 0 -> Available but hasn't happened.
> quality_resolution_downscales = x -> number of downscales
> 
> Or do you prefer keeping the bool?

Done. Removed bool.

[mflodman, 2015-10-16 11:45:51]

LGTM

[åsapersson, 2015-10-19 06:47:55]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2015-10-19 06:47:55]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1325153009/#ps140001 (title:
"removed bool")

[commit-bot: I haz the power, 2015-10-19 06:48:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1325153009/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1325153009/140001

[commit-bot: I haz the power, 2015-10-19 07:35:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-19 07:35:40]

Patchset 8 (id:??) landed as
https://crrev.com/4306fc70d778887d8a2ea71b6f4bc6a12d1d9447
Cr-Commit-Position: refs/heads/master@{#10319}