Add QP sum stats for received streams.

Add QP sum stats for received streams.

This is not implemented yet in any of the decoders.

BUG=webrtc:6541
Review-Url: https://codereview.webrtc.org/2649133005
Cr-Commit-Position: refs/heads/master@{#16475}
Committed: https://chromium.googlesource.com/external/webrtc/+/ff0e72fd165facac27f0313aa178648782e63bc4

[sakal, 2017-01-24 13:05:26]

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

[commit-bot: I haz the power, 2017-01-24 13:05:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[sakal, 2017-01-24 13:27:01]

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

[commit-bot: I haz the power, 2017-01-24 13:27:10]

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-01-24 14:31:50]

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

[commit-bot: I haz the power, 2017-01-24 14:31:51]

Dry run: This issue passed the CQ dry run.

[sakal, 2017-01-24 14:51:07]

Description was changed from

==========
Add QP sum stats for received streams.

BUG=webrtc:6541
==========

to

==========
Add QP sum stats for received streams.

BUG=webrtc:6541
==========

[sakal, 2017-01-24 14:51:08]

sakal@webrtc.org changed reviewers:
+ hbos@webrtc.org, hta@webrtc.org, perkj@webrtc.org, sprang@webrtc.org

[sakal, 2017-01-24 14:53:19]

Description was changed from

==========
Add QP sum stats for received streams.

BUG=webrtc:6541
==========

to

==========
Add QP sum stats for received streams.

This is not implemented yet in any of the decoders.

BUG=webrtc:6541
==========

[sakal, 2017-01-24 15:17:53]

PTAL

[hbos, 2017-01-25 11:31:13]

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.h:47: int qp);
nit: Can all uses of virtual be changed to override?

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding_defines.h:66: virtual int32_t
FrameToRender(VideoFrame& videoFrame, int qp) = 0;  // NOLINT
Can you use rtc::Optional<uint64_t> instead?

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/test/test_util.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/test/test_util.h:61: int qp);  // NOLINT
Can you change virtual to override? If so do it everywhere in this file.

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:407: }
Either all frames or no frames should have qp values. If only some of them have
qp values then qp sum will not align with the framesDecoded values and that is a
bug. Should we DCHECK that?

++stats_.frames_decoded;
if (qp) {
  if (!stats_.qp_sum) {
    RTC_DCHECK_EQ(stats_.frames_decoded, 1);
    stats_.qp_sum = rtc::Optional<uint64_t>(0);
  }
  *stats_.qp_sum += *qp;
} else {
  RTC_DCHECK(!stats_.qp_sum);
}

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:48: void OnDecodedFrame(int qp);
rtc::Optional<uint64_t>

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (left):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:387: 
Why is this no longer called?

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/video_stream...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/video_stream...
webrtc/video/video_stream_decoder.h:63: int32_t FrameToRender(VideoFrame&
video_frame, int qp) override;  // NOLINT
rtc::Optional<uint64_t>

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video_decoder.h
File webrtc/video_decoder.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video_decoder.h#ne...
webrtc/video_decoder.h:46: int qp) = 0;
I think there should just be one Decoded function with rtc::Optional<uint64_t>
qp. Is decode_time_ms also optional? (If it is but changing to optional is a
pain it can remain int64_t.)

You can add:
virtual int32_t Decoded(VideoFrame& decoded_image,
                        rtc::Optional<int64_t> decode_time_ms,
                        rtc::Optional<uint64_t> qp) {
  Decoded(decoded_image, decode_time_ms ? *decode_time_ms : -1);
}

And a TODO to remove default implementations / other Decoded versions when
upstream projects have implemented it.

[sakal, 2017-01-25 12:39:34]

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.h:47: int qp);
On 2017/01/25 11:31:12, hbos wrote:
> nit: Can all uses of virtual be changed to override?

Done.

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding_defines.h:66: virtual int32_t
FrameToRender(VideoFrame& videoFrame, int qp) = 0;  // NOLINT
On 2017/01/25 11:31:12, hbos wrote:
> Can you use rtc::Optional<uint64_t> instead?

This is something I thought about. I am not sure if we should use rtc::Optional
as a function argument? For non basic types, it is recommended to use const
pointer instead. rtc::Optional probably has worse performance but I doubt it is
anything noticeable since this is only called once per frame. Any comments?

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/test/test_util.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/test/test_util.h:61: int qp);  // NOLINT
On 2017/01/25 11:31:12, hbos wrote:
> Can you change virtual to override? If so do it everywhere in this file.

Done for this method.

I don't want to touch other classes in order to not clutter this already pretty
hard to review CL that touches a lot of files.

I also removed the NOLINTs because they didn't seem necessary and were possibly
added because of the missing override?

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:407: }
On 2017/01/25 11:31:12, hbos wrote:
> Either all frames or no frames should have qp values. If only some of them
have
> qp values then qp sum will not align with the framesDecoded values and that is
a
> bug. Should we DCHECK that?
> 
> ++stats_.frames_decoded;
> if (qp) {
>   if (!stats_.qp_sum) {
>     RTC_DCHECK_EQ(stats_.frames_decoded, 1);
>     stats_.qp_sum = rtc::Optional<uint64_t>(0);
>   }
>   *stats_.qp_sum += *qp;
> } else {
>   RTC_DCHECK(!stats_.qp_sum);
> }

Done.

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (left):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:387: 
On 2017/01/25 11:31:13, hbos wrote:
> Why is this no longer called?

It is called from VideoStreamDecoder::FrameToRender now.

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video_decoder.h
File webrtc/video_decoder.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/video_decoder.h#ne...
webrtc/video_decoder.h:46: int qp) = 0;
On 2017/01/25 11:31:13, hbos wrote:
> I think there should just be one Decoded function with rtc::Optional<uint64_t>
> qp. Is decode_time_ms also optional? (If it is but changing to optional is a
> pain it can remain int64_t.)
> 
> You can add:
> virtual int32_t Decoded(VideoFrame& decoded_image,
>                         rtc::Optional<int64_t> decode_time_ms,
>                         rtc::Optional<uint64_t> qp) {
>   Decoded(decoded_image, decode_time_ms ? *decode_time_ms : -1);
> }
> 
> And a TODO to remove default implementations / other Decoded versions when
> upstream projects have implemented it.

I removed the other DecodedWithQp method and renamed the other to Decoded. See
other comment for rtc::Optional discussion.

[sakal, 2017-01-25 12:39:40]

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

[commit-bot: I haz the power, 2017-01-25 12:39:51]

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-01-25 13:33:04]

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

[commit-bot: I haz the power, 2017-01-25 13:33:06]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-01-25 15:25:41]

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:403: if (qp != -1) {
Maybe we should dcheck on the off chance qp < -1 ?
Is 0 a valid value btw?

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:410: RTC_DCHECK(!stats_.qp_sum);
Any chance we could hit this if we change codec on the fly, such as falling back
from hw to sw encoder or vice versa?

[sakal, 2017-01-25 15:34:45]

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:403: if (qp != -1) {
On 2017/01/25 15:25:41, språng wrote:
> Maybe we should dcheck on the off chance qp < -1 ?
> Is 0 a valid value btw?

Sure, I can add that once we decide whether to use a integer or rtc::Optional. 0
is a valid value for all codecs.

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:410: RTC_DCHECK(!stats_.qp_sum);
On 2017/01/25 15:25:41, språng wrote:
> Any chance we could hit this if we change codec on the fly, such as falling
back
> from hw to sw encoder or vice versa?

Yes, I think that is possible if one supports providing QP but another doesn't.
This is of course non-ideal. Maybe in these cases we should just remove qp_sum
stat? Any comments on this?

[sprang_webrtc, 2017-01-25 15:59:15]

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding_defines.h:66: virtual int32_t
FrameToRender(VideoFrame& videoFrame, int qp) = 0;  // NOLINT
On 2017/01/25 12:39:34, sakal wrote:
> On 2017/01/25 11:31:12, hbos wrote:
> > Can you use rtc::Optional<uint64_t> instead?
> 
> This is something I thought about. I am not sure if we should use
rtc::Optional
> as a function argument? For non basic types, it is recommended to use const
> pointer instead. rtc::Optional probably has worse performance but I doubt it
is
> anything noticeable since this is only called once per frame. Any comments?

Agree that optional would be slightly more readable and that the overhead
shouldn't be terrible.
Imho this whole interface should be moved out of vcm and refactored.

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:410: RTC_DCHECK(!stats_.qp_sum);
On 2017/01/25 15:34:45, sakal wrote:
> On 2017/01/25 15:25:41, språng wrote:
> > Any chance we could hit this if we change codec on the fly, such as falling
> back
> > from hw to sw encoder or vice versa?
> 
> Yes, I think that is possible if one supports providing QP but another
doesn't.
> This is of course non-ideal. Maybe in these cases we should just remove qp_sum
> stat? Any comments on this?

Yeah, logging a warning and resetting the stats sounds like the right thing to
do then.

[sakal, 2017-01-26 09:33:31]

I switched to rtc::Optional<uint8_t> for qp. It is the smallest suitable type
since for VP9 QP values range between 0 - 255. For VP8 they are between 0 - 127.
For H264 they are between 0 - 51.

I used int32_t for decode_time_ms because that is what is used by
StopDecodeTimer it is passed to. Of course it should never be negative. Maybe I
should change it to uint32_t, any comments?

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:403: if (qp != -1) {
On 2017/01/25 15:34:45, sakal wrote:
> On 2017/01/25 15:25:41, språng wrote:
> > Maybe we should dcheck on the off chance qp < -1 ?
> > Is 0 a valid value btw?
> 
> Sure, I can add that once we decide whether to use a integer or rtc::Optional.
0
> is a valid value for all codecs.

Done.

https://codereview.webrtc.org/2649133005/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:410: RTC_DCHECK(!stats_.qp_sum);
On 2017/01/25 15:59:14, språng wrote:
> On 2017/01/25 15:34:45, sakal wrote:
> > On 2017/01/25 15:25:41, språng wrote:
> > > Any chance we could hit this if we change codec on the fly, such as
falling
> > back
> > > from hw to sw encoder or vice versa?
> > 
> > Yes, I think that is possible if one supports providing QP but another
> doesn't.
> > This is of course non-ideal. Maybe in these cases we should just remove
qp_sum
> > stat? Any comments on this?
> 
> Yeah, logging a warning and resetting the stats sounds like the right thing to
> do then.

Done.

[hta-webrtc, 2017-01-26 12:54:52]

lgtm

https://codereview.webrtc.org/2649133005/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2649133005/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:3348: EXPECT_EQ(stats.qp_sum,
info.receivers[0].qp_sum);
Will this comparision be correct? If stats.qp_sum is an rtc::Optional, checking
it will return a flag on whether it's present or not, right?

So it's checking whether both are present, not whether both have the same value.

[sakal, 2017-01-26 13:31:32]

On 2017/01/26 12:54:52, hta-webrtc wrote:
> lgtm
> 
>
https://codereview.webrtc.org/2649133005/diff/80001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2649133005/diff/80001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvideoengine2_unittest.cc:3348:
EXPECT_EQ(stats.qp_sum,
> info.receivers[0].qp_sum);
> Will this comparision be correct? If stats.qp_sum is an rtc::Optional,
checking
> it will return a flag on whether it's present or not, right?
> 
> So it's checking whether both are present, not whether both have the same
value.

I think EXPECT_EQ does an equality check on the object using operator==. So, it
will hit this overloaded method:
https://cs.chromium.org/chromium/src/third_party/webrtc/base/optional.h?q=rtc...

I don't think conversion to bool happens here?

[hbos, 2017-01-26 15:04:45]

I have a question about moving the OnDecodedFrame call to the FrameToRender.

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/test/test_util.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/test/test_util.h:61: int qp);  // NOLINT
On 2017/01/25 12:39:34, sakal wrote:
> On 2017/01/25 11:31:12, hbos wrote:
> > Can you change virtual to override? If so do it everywhere in this file.
> 
> Done for this method.
> 
> I don't want to touch other classes in order to not clutter this already
pretty
> hard to review CL that touches a lot of files.
> 
> I also removed the NOLINTs because they didn't seem necessary and were
possibly
> added because of the missing override?

There might be waterfall bots that complain if a file mixes using virtual in
override cases and using override. That happened to me a couple of times and I
had to revert, not sure if that is still the case.

I'm OK with virtual or override in all places in the file. Or this way if it
doesn't break waterfall.

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/mock/mock_video_codec_interface.h
(right):

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/mock/mock_video_codec_interface.h:63:
rtc::Optional<uint8_t> qp));
Is NOLINT necessary?

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/video/video_strea...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/video/video_strea...
webrtc/video/video_stream_decoder.cc:87:
receive_stats_callback_->OnDecodedFrame(qp);
Does this really give the same behavior as before?

Could a frame to be decoded and then never rendered (CPU/GPU adaptation)? Or
rendered multiple times (multiple sinks)?
Or maybe that is handled further down.

[sakal, 2017-01-26 15:46:52]

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/test/test_util.h (right):

https://codereview.webrtc.org/2649133005/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/test/test_util.h:61: int qp);  // NOLINT
On 2017/01/26 15:04:45, hbos wrote:
> On 2017/01/25 12:39:34, sakal wrote:
> > On 2017/01/25 11:31:12, hbos wrote:
> > > Can you change virtual to override? If so do it everywhere in this file.
> > 
> > Done for this method.
> > 
> > I don't want to touch other classes in order to not clutter this already
> pretty
> > hard to review CL that touches a lot of files.
> > 
> > I also removed the NOLINTs because they didn't seem necessary and were
> possibly
> > added because of the missing override?
> 
> There might be waterfall bots that complain if a file mixes using virtual in
> override cases and using override. That happened to me a couple of times and I
> had to revert, not sure if that is still the case.
> 
> I'm OK with virtual or override in all places in the file. Or this way if it
> doesn't break waterfall.

I changed all methods to override and removed a few unneeded ones.

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/mock/mock_video_codec_interface.h
(right):

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/mock/mock_video_codec_interface.h:63:
rtc::Optional<uint8_t> qp));
On 2017/01/26 15:04:45, hbos wrote:
> Is NOLINT necessary?

Yes:
Is this a non-const reference? If so, make const or use a pointer: VideoFrame&
decodedImage  [runtime/references] [2]

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/video/video_strea...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/video/video_strea...
webrtc/video/video_stream_decoder.cc:87:
receive_stats_callback_->OnDecodedFrame(qp);
On 2017/01/26 15:04:45, hbos wrote:
> Does this really give the same behavior as before?
> 
> Could a frame to be decoded and then never rendered (CPU/GPU adaptation)? Or
> rendered multiple times (multiple sinks)?
> Or maybe that is handled further down.

VideoReceiveStream::OnFrame is actually further down than this method. This
method calls IncomingVideoStream::OnFrame which does frame phasing and calls
VideoReceiveStream::OnFrame on another thread.

So calling it here leads to it being called earlier and not being phased but I
don't think that is a problem.

[sakal, 2017-01-26 15:47:01]

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

[commit-bot: I haz the power, 2017-01-26 15:47:09]

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-01-26 15:48:03]

> I used int32_t for decode_time_ms because that is what is used by
> StopDecodeTimer it is passed to. Of course it should never be negative. Maybe
I
> should change it to uint32_t, any comments?

I can't find a single implementation that actually uses the decode_time_ms
parameter. Can we try removing it altogether?

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/video/video_strea...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/video/video_strea...
webrtc/video/video_stream_decoder.cc:87:
receive_stats_callback_->OnDecodedFrame(qp);
On 2017/01/26 15:04:45, hbos wrote:
> Does this really give the same behavior as before?
> 
> Could a frame to be decoded and then never rendered (CPU/GPU adaptation)? Or
> rendered multiple times (multiple sinks)?
> Or maybe that is handled further down.

I think that's all further downstream. incoming_video_stream_ is the first place
I know of that can drop a frame after decoding.

[hbos, 2017-01-26 15:48:57]

lgtm

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/video/video_strea...
File webrtc/video/video_stream_decoder.cc (right):

https://codereview.webrtc.org/2649133005/diff/100001/webrtc/video/video_strea...
webrtc/video/video_stream_decoder.cc:87:
receive_stats_callback_->OnDecodedFrame(qp);
On 2017/01/26 15:48:03, språng wrote:
> On 2017/01/26 15:04:45, hbos wrote:
> > Does this really give the same behavior as before?
> > 
> > Could a frame to be decoded and then never rendered (CPU/GPU adaptation)? Or
> > rendered multiple times (multiple sinks)?
> > Or maybe that is handled further down.
> 
> I think that's all further downstream. incoming_video_stream_ is the first
place
> I know of that can drop a frame after decoding. 

Acknowledged.

[commit-bot: I haz the power, 2017-01-26 16:01:03]

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

[commit-bot: I haz the power, 2017-01-26 16:01:04]

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

[sakal, 2017-01-26 16:23:19]

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

[commit-bot: I haz the power, 2017-01-26 16:23:23]

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-01-26 17:36:56]

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

[commit-bot: I haz the power, 2017-01-26 17:36:57]

Dry run: This issue passed the CQ dry run.

[perkj_webrtc, 2017-01-29 10:24:35]

perkj@webrtc.org changed reviewers:
+ mflodman@webrtc.org
- perkj@webrtc.org

[perkj_webrtc, 2017-01-29 10:25:01]

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

[perkj_webrtc, 2017-01-29 10:25:02]

Replaced my self with mflodman.

[perkj_webrtc, 2017-01-29 10:40:46]

perkj@webrtc.org changed reviewers:
- perkj@webrtc.org

[sakal, 2017-01-30 12:33:25]

On 2017/01/26 15:48:03, språng wrote:
> > I used int32_t for decode_time_ms because that is what is used by
> > StopDecodeTimer it is passed to. Of course it should never be negative.
Maybe
> I
> > should change it to uint32_t, any comments?
> 
> I can't find a single implementation that actually uses the decode_time_ms
> parameter. Can we try removing it altogether?

At least MediaCodecVideoDecoder on Android is using this:
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a...

[sprang_webrtc, 2017-01-30 13:35:26]

On 2017/01/30 12:33:25, sakal wrote:
> On 2017/01/26 15:48:03, språng wrote:
> > > I used int32_t for decode_time_ms because that is what is used by
> > > StopDecodeTimer it is passed to. Of course it should never be negative.
> Maybe
> > I
> > > should change it to uint32_t, any comments?
> > 
> > I can't find a single implementation that actually uses the decode_time_ms
> > parameter. Can we try removing it altogether?
> 
> At least MediaCodecVideoDecoder on Android is using this:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a...

Huh, must have missed that.

lgtm

[hbos, 2017-02-02 12:36:05]

Ping!

When this lands I'll add qpSum for the inbound case to the new getStats API as
well.

[sprang_webrtc, 2017-02-02 12:51:41]

sprang@webrtc.org changed reviewers:
+ magjed@webrtc.org
- mflodman@webrtc.org

[sprang_webrtc, 2017-02-02 12:51:43]

mflodman will be OOO, so replacing with magjed which should be enough for owners
coverage. ptal

[magjed_webrtc, 2017-02-02 15:54:07]

lgtm

[sakal, 2017-02-03 08:18:58]

sakal@webrtc.org changed reviewers:
+ stefan@webrtc.org

[sakal, 2017-02-03 08:19:41]

Stefan could take a look at the interface changes:
webrtc/video_decoder.h
webrtc/video_receive_stream.h

[sakal, 2017-02-07 09:41:29]

ping

[stefan-webrtc, 2017-02-07 09:56:52]

lg, but one comment that may be worth addressing.

https://codereview.webrtc.org/2649133005/diff/140001/webrtc/video_decoder.h
File webrtc/video_decoder.h (right):

https://codereview.webrtc.org/2649133005/diff/140001/webrtc/video_decoder.h#n...
webrtc/video_decoder.h:44: virtual int32_t Decoded(VideoFrame& decodedImage,
Would it be a good idea to update the return type to int (or perhaps even bool?)
while we're at it?

[sakal, 2017-02-07 12:08:30]

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

[commit-bot: I haz the power, 2017-02-07 12:08:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[sakal, 2017-02-07 12:28:18]

PTAL

https://codereview.webrtc.org/2649133005/diff/140001/webrtc/video_decoder.h
File webrtc/video_decoder.h (right):

https://codereview.webrtc.org/2649133005/diff/140001/webrtc/video_decoder.h#n...
webrtc/video_decoder.h:44: virtual int32_t Decoded(VideoFrame& decodedImage,
On 2017/02/07 09:56:51, stefan-webrtc wrote:
> Would it be a good idea to update the return type to int (or perhaps even
bool?)
> while we're at it?

I removed the return value completely since we always returned OK anyway.

[stefan-webrtc, 2017-02-07 13:16:01]

lgtm

[commit-bot: I haz the power, 2017-02-07 13:34:11]

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

[commit-bot: I haz the power, 2017-02-07 13:34:12]

Dry run: This issue passed the CQ dry run.

[sakal, 2017-02-07 14:03:18]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2017-02-07 14:03:19]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org,
hbos@webrtc.org, magjed@webrtc.org, sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2649133005/#ps160001 (title:
"Remove return value from the new Decoded method since we always returned OK.")

[commit-bot: I haz the power, 2017-02-07 14:03:28]

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-02-07 14:06:28]

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

[commit-bot: I haz the power, 2017-02-07 14:06:30]

Failed to apply patch for webrtc/modules/video_coding/generic_decoder.cc:
While running git apply --index -p1;
  error: patch failed: webrtc/modules/video_coding/generic_decoder.cc:63
  error: webrtc/modules/video_coding/generic_decoder.cc: patch does not apply

Patch:       webrtc/modules/video_coding/generic_decoder.cc
Index: webrtc/modules/video_coding/generic_decoder.cc
diff --git a/webrtc/modules/video_coding/generic_decoder.cc
b/webrtc/modules/video_coding/generic_decoder.cc
index
b95571b8f73793718d318eedc82fff249e9a9f17..277702d44b9181cb7729a2af1db3cf4a4236d99f
100644
--- a/webrtc/modules/video_coding/generic_decoder.cc
+++ b/webrtc/modules/video_coding/generic_decoder.cc
@@ -48,6 +48,16 @@ int32_t VCMDecodedFrameCallback::Decoded(VideoFrame&
decodedImage) {
 
 int32_t VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
                                          int64_t decode_time_ms) {
+  Decoded(decodedImage,
+          decode_time_ms >= 0 ? rtc::Optional<int32_t>(decode_time_ms)
+                              : rtc::Optional<int32_t>(),
+          rtc::Optional<uint8_t>());
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+void VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
+                                      rtc::Optional<int32_t> decode_time_ms,
+                                      rtc::Optional<uint8_t> qp) {
   TRACE_EVENT_INSTANT1("webrtc", "VCMDecodedFrameCallback::Decoded",
                        "timestamp", decodedImage.timestamp());
   // TODO(holmer): We should improve this so that we can handle multiple
@@ -63,15 +73,15 @@ int32_t VCMDecodedFrameCallback::Decoded(VideoFrame&
decodedImage,
   if (frameInfo == NULL) {
     LOG(LS_WARNING) << "Too many frames backed up in the decoder, dropping "
                        "this one.";
-    return WEBRTC_VIDEO_CODEC_OK;
+    return;
   }
 
   const int64_t now_ms = _clock->TimeInMilliseconds();
-  if (decode_time_ms < 0) {
+  if (!decode_time_ms) {
     decode_time_ms =
-        static_cast<int32_t>(now_ms - frameInfo->decodeStartTimeMs);
+        rtc::Optional<int32_t>(now_ms - frameInfo->decodeStartTimeMs);
   }
-  _timing->StopDecodeTimer(decodedImage.timestamp(), decode_time_ms, now_ms,
+  _timing->StopDecodeTimer(decodedImage.timestamp(), *decode_time_ms, now_ms,
                            frameInfo->renderTimeMs);
 
   decodedImage.set_render_time_ms(frameInfo->renderTimeMs);
@@ -79,11 +89,10 @@ int32_t VCMDecodedFrameCallback::Decoded(VideoFrame&
decodedImage,
   // TODO(sakal): Investigate why callback is NULL sometimes and replace if
   // statement with a DCHECK.
   if (callback) {
-    callback->FrameToRender(decodedImage);
+    callback->FrameToRender(decodedImage, qp);
   } else {
     LOG(LS_WARNING) << "No callback, dropping frame.";
   }
-  return WEBRTC_VIDEO_CODEC_OK;
 }
 
 int32_t VCMDecodedFrameCallback::ReceivedDecodedReferenceFrame(

[sakal, 2017-02-07 14:20:09]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2017-02-07 14:20:10]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
hta@webrtc.org, sprang@webrtc.org, hbos@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2649133005/#ps180001 (title:
"Rebase.")

[commit-bot: I haz the power, 2017-02-07 14:20:17]

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-02-07 15:15:18]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486477209863700,
"parent_rev": "7de8d64f89b51967ee7fa8ff2a07f716aee72c53", "commit_rev":
"ff0e72fd165facac27f0313aa178648782e63bc4"}

[commit-bot: I haz the power, 2017-02-07 15:15:21]

Description was changed from

==========
Add QP sum stats for received streams.

This is not implemented yet in any of the decoders.

BUG=webrtc:6541
==========

to

==========
Add QP sum stats for received streams.

This is not implemented yet in any of the decoders.

BUG=webrtc:6541

Review-Url: https://codereview.webrtc.org/2649133005
Cr-Commit-Position: refs/heads/master@{#16475}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ff0e72fd165facac27f0313aa...
==========

[commit-bot: I haz the power, 2017-02-07 15:15:23]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/ff0e72fd165facac27f0313aa...

[skvlad, 2017-02-07 18:46:01]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.webrtc.org/2680893002/ by skvlad@webrtc.org.

The reason for reverting is: Breaks downstream build..