Add stats for average QP per frame for VP8 (for received video streams).

Add stats for average QP per frame for VP8 (for received video streams):

"WebRTC.Video.Decoded.VP8.Qp"

BUG=chromium:512752
Committed: https://crrev.com/86b016027d2d27c62fedd108354a2b1274118ae3
Cr-Commit-Position: refs/heads/master@{#10349}

[åsapersson, 2015-09-18 15:35:51]

Patchset #3 (id:40001) has been deleted

[åsapersson, 2015-09-18 15:35:59]

Patchset #2 (id:20001) has been deleted

[åsapersson, 2015-09-18 15:36:09]

Patchset #2 (id:60001) has been deleted

[åsapersson, 2015-09-21 12:31:38]

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

[pbos-webrtc, 2015-09-24 16:16:27]

https://codereview.webrtc.org/1340623002/diff/180001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/include/qp_wrapper.h (right):

https://codereview.webrtc.org/1340623002/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/include/qp_wrapper.h:18: int GetQp(
This doesn't work generally since H264 parsing is stateful. We need to maintain
a "QP parser" object for the incoming stream. Can you put this into a QpParser
class that we reset whenever incoming codec type changes?

We'll also need bool ::GetQp(&qp, stream, length), since parsing will be able to
fail (or return -1, but I prefer it failing). Are you cool with making it bool
as return type?

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

https://codereview.webrtc.org/1340623002/diff/180001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:52:
RTC_HISTOGRAM_COUNTS_200("WebRTC.Video.Decoded.Vp8.Qp", qp);
What does 200 here mean? :)

https://codereview.webrtc.org/1340623002/diff/180001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:173: if (codec_specific_info ==
nullptr) {
|| encoded_image.qp_ == -1), early return instead

[åsapersson, 2015-09-25 14:11:12]

https://codereview.webrtc.org/1340623002/diff/180001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/include/qp_wrapper.h (right):

https://codereview.webrtc.org/1340623002/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/include/qp_wrapper.h:18: int GetQp(
On 2015/09/24 16:16:27, pbos-webrtc wrote:
> This doesn't work generally since H264 parsing is stateful. We need to
maintain
> a "QP parser" object for the incoming stream. Can you put this into a QpParser
> class that we reset whenever incoming codec type changes?
> 
> We'll also need bool ::GetQp(&qp, stream, length), since parsing will be able
to
> fail (or return -1, but I prefer it failing). Are you cool with making it bool
> as return type?

Created QpParser class to work for H264.
Changed return type to bool.

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

https://codereview.webrtc.org/1340623002/diff/180001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:52:
RTC_HISTOGRAM_COUNTS_200("WebRTC.Video.Decoded.Vp8.Qp", qp);
On 2015/09/24 16:16:27, pbos-webrtc wrote:
> What does 200 here mean? :)

Max sample value (min:1, max:200, num_bucket:50).

https://codereview.webrtc.org/1340623002/diff/180001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:173: if (codec_specific_info ==
nullptr) {
On 2015/09/24 16:16:27, pbos-webrtc wrote:
> || encoded_image.qp_ == -1), early return instead

Done.

[stefan-webrtc, 2015-09-28 15:26:51]

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

https://codereview.webrtc.org/1340623002/diff/280001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:178: rtc::CritScope lock(&crit_);
Does this have to be locked? I can't see that we are using it anywhere else
while taking this lock.

[pbos-webrtc, 2015-09-28 15:37:24]

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

https://codereview.webrtc.org/1340623002/diff/280001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:178: rtc::CritScope lock(&crit_);
On 2015/09/28 15:26:51, stefan-webrtc (holmer) wrote:
> Does this have to be locked? I can't see that we are using it anywhere else
> while taking this lock.

Having GUARDED_BY permits us having this warn if we start using it from multiple
threads, but I don't know if we need this to be that future proof. For thread
safety the lock is fine to skip now so long as these Encoded() calls are not
concurrent (can they be?).

[åsapersson, 2015-10-13 09:18:00]

Patchset #13 (id:300001) has been deleted

[åsapersson, 2015-10-13 09:23:42]

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

https://codereview.webrtc.org/1340623002/diff/280001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:178: rtc::CritScope lock(&crit_);
On 2015/09/28 15:37:24, pbos-webrtc wrote:
> On 2015/09/28 15:26:51, stefan-webrtc (holmer) wrote:
> > Does this have to be locked? I can't see that we are using it anywhere else
> > while taking this lock.
> 
> Having GUARDED_BY permits us having this warn if we start using it from
multiple
> threads, but I don't know if we need this to be that future proof. For thread
> safety the lock is fine to skip now so long as these Encoded() calls are not
> concurrent (can they be?).

Removed lock and added comment.

[pbos-webrtc, 2015-10-15 11:56:02]

https://codereview.webrtc.org/1340623002/diff/320001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:790: if
(webrtc::vp8::GetQp(payload, payload_size, &qp))
I think we can RTC_CHECK this one, the stream we produce should be trusted.

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/video_receiver.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/video_receiver.cc:306: if
(qp_parser_.GetQp(frame->Buffer(), frame->Length(),
Log a warning if this fails imo.

[stefan-webrtc, 2015-10-15 12:49:35]

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/include/qp_parser.h (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/include/qp_parser.h:27: VideoCodecType
codec_type,
Can this take a VCMEncodedFrame instead maybe?

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/vp8_header_parser.cc:19: const size_t
kCommonPayloadHdrLength = 3;
PayloadHdr -> PayloadHeader

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:325: int32_t VideoReceiveStream::Encoded(
I find it a bit strange that we use an encode callback on the receiver. Should
we at least move the callback class to somewhere different than video_encoder.h?
Or maybe just create a different callback for this purpose?

[åsapersson, 2015-10-16 09:03:14]

Patchset #14 (id:340001) has been deleted

[åsapersson, 2015-10-16 09:28:07]

Patchset #14 (id:360001) has been deleted

[åsapersson, 2015-10-16 09:33:59]

https://codereview.webrtc.org/1340623002/diff/320001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:790: if
(webrtc::vp8::GetQp(payload, payload_size, &qp))
On 2015/10/15 11:56:01, pbos-webrtc wrote:
> I think we can RTC_CHECK this one, the stream we produce should be trusted.

Added a RTC_DCHECK (call can proceed even if this fails).

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/video_receiver.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/video_receiver.cc:306: if
(qp_parser_.GetQp(frame->Buffer(), frame->Length(),
On 2015/10/15 11:56:02, pbos-webrtc wrote:
> Log a warning if this fails imo.

Added log warnings in vp8_header_parser.cc.

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/include/qp_parser.h (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/include/qp_parser.h:27: VideoCodecType
codec_type,
On 2015/10/15 12:49:35, stefan-webrtc (holmer) wrote:
> Can this take a VCMEncodedFrame instead maybe?

Done.

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/vp8_header_parser.cc:19: const size_t
kCommonPayloadHdrLength = 3;
On 2015/10/15 12:49:35, stefan-webrtc (holmer) wrote:
> PayloadHdr -> PayloadHeader

Done.

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:325: int32_t VideoReceiveStream::Encoded(
On 2015/10/15 12:49:35, stefan-webrtc (holmer) wrote:
> I find it a bit strange that we use an encode callback on the receiver. Should
> we at least move the callback class to somewhere different than
video_encoder.h?
> Or maybe just create a different callback for this purpose?

Do this in a separate CL? Added comment.

[stefan-webrtc, 2015-10-16 09:40:10]

lgtm

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:325: int32_t VideoReceiveStream::Encoded(
On 2015/10/16 09:33:59, åsapersson wrote:
> On 2015/10/15 12:49:35, stefan-webrtc (holmer) wrote:
> > I find it a bit strange that we use an encode callback on the receiver.
Should
> > we at least move the callback class to somewhere different than
> video_encoder.h?
> > Or maybe just create a different callback for this purpose?
> 
> Do this in a separate CL? Added comment.

Sounds good.

[pbos-webrtc, 2015-10-16 09:50:56]

lgtm

https://codereview.webrtc.org/1340623002/diff/320001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/1340623002/diff/320001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:790: if
(webrtc::vp8::GetQp(payload, payload_size, &qp))
On 2015/10/16 09:33:59, åsapersson wrote:
> On 2015/10/15 11:56:01, pbos-webrtc wrote:
> > I think we can RTC_CHECK this one, the stream we produce should be trusted.
> 
> Added a RTC_DCHECK (call can proceed even if this fails).

I think we want a CHECK to run out in the field, it'd be good to know if any
encoders start giving broken bitstreams.

[åsapersson, 2015-10-16 10:55:45]

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

[åsapersson, 2015-10-16 10:59:23]

Magnus, can you take a look at webrtc/video_frame.h?

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

video_frame.h LGTM

[åsapersson, 2015-10-20 09:30:37]

Patchset #15 (id:400001) has been deleted

[åsapersson, 2015-10-20 11:05:20]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2015-10-20 11:05:21]

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

[commit-bot: I haz the power, 2015-10-20 11:05:31]

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

[commit-bot: I haz the power, 2015-10-20 11:12:41]

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

[commit-bot: I haz the power, 2015-10-20 11:12:42]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1314)

[åsapersson, 2015-10-20 11:17:24]

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

[åsapersson, 2015-10-20 11:23:33]

Adding magjed for
talk/app/webrtc/java/jni/androidmediaencoder_jni.h OWNERS

[magjed_webrtc, 2015-10-20 13:17:21]

https://codereview.webrtc.org/1340623002/diff/420001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/1340623002/diff/420001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:802: if (found)
You shouldn't handle DCHECK failures. At least this is the case according to the
Chromium style guide:
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

[åsapersson, 2015-10-20 13:52:36]

https://codereview.webrtc.org/1340623002/diff/420001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/1340623002/diff/420001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:802: if (found)
On 2015/10/20 13:17:20, magjed_webrtc wrote:
> You shouldn't handle DCHECK failures. At least this is the case according to
the
> Chromium style guide:
>
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

Ok. Removed check.

[magjed_webrtc, 2015-10-20 15:40:55]

androidmediaencoder_jni lgtm

[åsapersson, 2015-10-21 06:51:24]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2015-10-21 06:51:25]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
stefan@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1340623002/#ps440001 (title:
"remove check")

[commit-bot: I haz the power, 2015-10-21 06:51:31]

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

[commit-bot: I haz the power, 2015-10-21 06:55:31]

Committed patchset #16 (id:440001)

[commit-bot: I haz the power, 2015-10-21 06:55:37]

Patchset 16 (id:??) landed as
https://crrev.com/86b016027d2d27c62fedd108354a2b1274118ae3
Cr-Commit-Position: refs/heads/master@{#10349}