RTCMediaStreamTrackStats.frameWidth/Height,framesPerSecond,framesDecoded

RTCMediaStreamTrackStats.frameWidth/Height,framesPerSecond,framesDecoded

- frameWidth/Height previously corresponded to the input width/height,
  now it corresponds to the size sent/received.
- framesPerSecond added, which is the framerate sent/outputed.
- framesDecoded added.

BUG=webrtc:6757, chromium:659137, chromium:627816

[hbos, 2016-12-29 11:38:59]

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

[commit-bot: I haz the power, 2016-12-29 11:39:03]

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, 2016-12-29 11:43:09]

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

[commit-bot: I haz the power, 2016-12-29 11:43:10]

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

[hbos, 2016-12-29 12:44:14]

Description was changed from

==========
RTCMediaStreamTrackStats frameWidth/Height, framesPerSecond, framesDecoded

BUG=
==========

to

==========
RTCMediaStreamTrackStats.frameWidth/Height,framesPerSecond,framesDecoded

- frameWidth/Height previously corresponded to the input width/height,
  now it corresponds to the size sent/received.
- framesPerSecond added, which is the framerate sent/outputed.
- framesDecoded added.

BUG=webrtc:6757, chromium:659137, chromium:627816
==========

[hbos, 2016-12-29 12:45:40]

hbos@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, hta@webrtc.org

[hbos, 2016-12-29 12:45:41]

Please take a look, hta and deadbeef.

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (left):

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:467: video_track_source_stats.input_height);
These sizes correspond to googFrame[Width/Height]Input, probably not as
interesting as googFrame[Width/Height]Sent
(sender_info->send_frame_[width/height])?

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:454: sender_info->framerate_sent);
Alternatively, VideoSenderInfo has framerate_input too.

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:467: receiver_info->framerate_output);
Alternatively, VideoReceiverInfo also has framerate_rcvd, framerate_decoded,
framerate_render_input and framerate_render_output.

[hbos, 2016-12-29 14:24:48]

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (left):

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:467: video_track_source_stats.input_height);
On 2016/12/29 12:45:41, hbos wrote:
> These sizes correspond to googFrame[Width/Height]Input, probably not as
> interesting as googFrame[Width/Height]Sent
> (sender_info->send_frame_[width/height])?

I also found out that this was not available in the rtcstats_integrationtest.cc
for remoteSource = true which made it not possible to distinguish between remote
video and audio by looking at the frameWidth.

[hbos, 2016-12-29 15:29:28]

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:468: video_track_stats->frames_decoded =
receiver_info->frames_decoded;
Uhm, this is the same as RTCInboundRTPStreamStats.framesDecoded added in
https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec....

That can't be right, duplicate stats?

[hta-webrtc, 2017-01-02 16:06:42]

On third thought, I think this needs another think about "decoded frames" before
we merge it.

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:468: video_track_stats->frames_decoded =
receiver_info->frames_decoded;
On 2016/12/29 15:29:28, hbos wrote:
> Uhm, this is the same as RTCInboundRTPStreamStats.framesDecoded added in
>
https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec....
> 
> That can't be right, duplicate stats?

Add a TODO to count the frames delivered to the track instead. I think this
needs some redesign, since in the case where we switch what SSRC we take input
from (simulcast), we need to keep the count correct over the switch, which
requires saving the number at the moment of the switch for both the track and
for the SSRC we switch to, which requires having memory for that in the
statscollector.

This needs deadbeef or video team input: When we have incoming simulcast, do we
switch SSRCs, or does the switch mux everything into a single SSRC? If separate
SSRCs, do we even decode the ones that aren't being shown?

[Taylor Brandstetter, 2017-01-04 23:25:24]

I think some questions still need to be answered. We need to be clear about
which rate "framesPerSecond" is intended to represent, for both local and remote
tracks. Harald, can you take a look at my comments below?

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:454: sender_info->framerate_sent);
On 2016/12/29 12:45:41, hbos wrote:
> Alternatively, VideoSenderInfo has framerate_input too.

I'm not completely sure that framerate_sent (or framerate_input) are correct.
Suppose the source framerate is 60fps, the target encode framerate is 30fps, and
the actual number of frames encoded during the last second is 29 (because one
had to be dropped). Should frames_per_second be 60 (framerate_input), 29
(framerate_sent), or 30 (which might be this?
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...).

We probably should loop in someone more familiar with the video pipeline.

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:467: receiver_info->framerate_output);
On 2016/12/29 12:45:41, hbos wrote:
> Alternatively, VideoReceiverInfo also has framerate_rcvd, framerate_decoded,
> framerate_render_input and framerate_render_output.

I'm also not sure that this is completely right. For codecs that store the
framerate in the bitstream (such as H.264), should this be the framerate from
the bitstream, or the framerate received?

Here's where the former would be for H.264:
https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/avcodec.h?...

It looks like we don't currently do anything with it. So for now, framerate_rcvd
may be the best we can do.

https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:468: video_track_stats->frames_decoded =
receiver_info->frames_decoded;
On 2017/01/02 16:06:42, hta-webrtc wrote:
> On 2016/12/29 15:29:28, hbos wrote:
> > Uhm, this is the same as RTCInboundRTPStreamStats.framesDecoded added in
> >
>
https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec....
> > 
> > That can't be right, duplicate stats?
> 
> Add a TODO to count the frames delivered to the track instead. I think this
> needs some redesign, since in the case where we switch what SSRC we take input
> from (simulcast), we need to keep the count correct over the switch, which
> requires saving the number at the moment of the switch for both the track and
> for the SSRC we switch to, which requires having memory for that in the
> statscollector.
> 
> This needs deadbeef or video team input: When we have incoming simulcast, do
we
> switch SSRCs, or does the switch mux everything into a single SSRC? If
separate
> SSRCs, do we even decode the ones that aren't being shown?

I believe this already does count the frames delivered to the track. When using
simulcast, the entire simulcast group is represented by a single VideoSenderInfo
with multiple SSRCs (again, using the first one for identification). Which means
that we're not currently collecting per-simulcast-layer stats, which (if they
existed) would go in SsrcSenderInfo.

[hbos, 2017-01-18 14:03:50]

On 2017/01/04 23:25:24, Taylor Brandstetter wrote:
> I think some questions still need to be answered. We need to be clear about
> which rate "framesPerSecond" is intended to represent, for both local and
remote
> tracks. Harald, can you take a look at my comments below?
> 
>
https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector.cc
> File webrtc/api/rtcstatscollector.cc (right):
> 
>
https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
> webrtc/api/rtcstatscollector.cc:454: sender_info->framerate_sent);
> On 2016/12/29 12:45:41, hbos wrote:
> > Alternatively, VideoSenderInfo has framerate_input too.
> 
> I'm not completely sure that framerate_sent (or framerate_input) are correct.
> Suppose the source framerate is 60fps, the target encode framerate is 30fps,
and
> the actual number of frames encoded during the last second is 29 (because one
> had to be dropped). Should frames_per_second be 60 (framerate_input), 29
> (framerate_sent), or 30 (which might be this?
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...).
> 
> We probably should loop in someone more familiar with the video pipeline.
> 
>
https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
> webrtc/api/rtcstatscollector.cc:467: receiver_info->framerate_output);
> On 2016/12/29 12:45:41, hbos wrote:
> > Alternatively, VideoReceiverInfo also has framerate_rcvd, framerate_decoded,
> > framerate_render_input and framerate_render_output.
> 
> I'm also not sure that this is completely right. For codecs that store the
> framerate in the bitstream (such as H.264), should this be the framerate from
> the bitstream, or the framerate received?
> 
> Here's where the former would be for H.264:
>
https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/avcodec.h?...
> 
> It looks like we don't currently do anything with it. So for now,
framerate_rcvd
> may be the best we can do.
> 
>
https://codereview.webrtc.org/2602033002/diff/1/webrtc/api/rtcstatscollector....
> webrtc/api/rtcstatscollector.cc:468: video_track_stats->frames_decoded =
> receiver_info->frames_decoded;
> On 2017/01/02 16:06:42, hta-webrtc wrote:
> > On 2016/12/29 15:29:28, hbos wrote:
> > > Uhm, this is the same as RTCInboundRTPStreamStats.framesDecoded added in
> > >
> >
>
https://codereview.webrtc.org/2588373005/diff/20001/webrtc/api/rtcstatscollec....
> > > 
> > > That can't be right, duplicate stats?
> > 
> > Add a TODO to count the frames delivered to the track instead. I think this
> > needs some redesign, since in the case where we switch what SSRC we take
input
> > from (simulcast), we need to keep the count correct over the switch, which
> > requires saving the number at the moment of the switch for both the track
and
> > for the SSRC we switch to, which requires having memory for that in the
> > statscollector.
> > 
> > This needs deadbeef or video team input: When we have incoming simulcast, do
> we
> > switch SSRCs, or does the switch mux everything into a single SSRC? If
> separate
> > SSRCs, do we even decode the ones that aren't being shown?
> 
> I believe this already does count the frames delivered to the track. When
using
> simulcast, the entire simulcast group is represented by a single
VideoSenderInfo
> with multiple SSRCs (again, using the first one for identification). Which
means
> that we're not currently collecting per-simulcast-layer stats, which (if they
> existed) would go in SsrcSenderInfo.

Closing this due to https://codereview.webrtc.org/2602033002 replacing a lot of
this code. Will add the framesPerSecond/framesDecoded stats in tiny separate
follow-up CLs to it.