Add content type information to encoded images and corresponding rtp extension header

Add content type information to Encoded Images and add corresponding RTP extension header.
Use it to separate UMA e2e delay metric between screenshare from video.
Content type extension is set based on encoder settings and processed and decoders.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested.

BUG=webrtc:7420
Review-Url: https://codereview.webrtc.org/2772033002
Cr-Commit-Position: refs/heads/master@{#17640}
Committed: https://chromium.googlesource.com/external/webrtc/+/64e739aeae5629cbbebf2a19e1d3e6b452bb6d0b

[ilnik, 2017-03-24 12:22:59]

ilnik@webrtc.org changed reviewers:
+ sprang@webrtc.org, stefan@webrtc.org, tommi@webrtc.org

[ilnik, 2017-03-24 12:23:00]

Content type is now propagated from receiver to sender. Added new field to
VideoFrame and EncodedImage. Added new RTCP header extension. Majority of
changes are trivial and consist of an additional parameter in the constructor.

[ilnik, 2017-03-27 07:42:53]

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

[commit-bot: I haz the power, 2017-03-27 07:43:02]

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-03-27 07:46:52]

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

[commit-bot: I haz the power, 2017-03-27 07:46:52]

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

[ilnik, 2017-03-27 09:36:11]

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

[commit-bot: I haz the power, 2017-03-27 09:36:18]

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-03-27 09:46:24]

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

[commit-bot: I haz the power, 2017-03-27 09:46:25]

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

[ilnik, 2017-03-27 10:00:32]

Description was changed from

==========
Add content type information to video frames and rtp extension header.
Use it to separate UMA e2e delay metric between screenshare from video.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested

BUG=none
==========

to

==========
Add content type information to video frames and rtp extension header.
Use it to separate UMA e2e delay metric between screenshare from video.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested

BUG=none
==========

[ilnik, 2017-03-27 10:00:33]

ilnik@webrtc.org changed reviewers:
- sprang@webrtc.org, stefan@webrtc.org, tommi@webrtc.org

[ilnik, 2017-03-27 10:43:42]

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

[commit-bot: I haz the power, 2017-03-27 10:43:45]

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-03-27 11:04:19]

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

[commit-bot: I haz the power, 2017-03-27 11:04:20]

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

[ilnik, 2017-03-27 15:20:37]

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

[commit-bot: I haz the power, 2017-03-27 15:20:43]

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

[ilnik, 2017-03-27 15:21:06]

ilnik@webrtc.org changed reviewers:
+ sprang@webrtc.org, stefan@webrtc.org, tommi@webrtc.org

[commit-bot: I haz the power, 2017-03-27 15:59:29]

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

[commit-bot: I haz the power, 2017-03-27 15:59:30]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-03-28 11:14:23]

First pass, looks good
Maybe I missed something, but do we always send this extension now? I didn't
spot where we configure if it should be enabled after negotiation.

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

https://codereview.webrtc.org/2772033002/diff/140001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:19: };
Not sure this is the right place for this. Also I think there are already enum
in place you can use, such as VideoCodecMode in common_type.h

Otherwise, prefer this be an enum class, and drop the kVideoContent_ prefix.

https://codereview.webrtc.org/2772033002/diff/140001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2772033002/diff/140001/webrtc/common_types.h#ne...
webrtc/common_types.h:722: VideoContentTypeId videoContentType;
Maybe an rtc::Optional<VideoContentTypeId> would be in place here?
If so, maybe even refactor the ones above.

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

https://codereview.webrtc.org/2772033002/diff/140001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:77: e2e_delay_max_ms_screenshare_(-1),
Maybe we should report all stats and reset state if we detect a content type
switch, similar to how we do it for the send side?

[ilnik, 2017-03-28 14:47:02]

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

https://codereview.webrtc.org/2772033002/diff/140001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:19: };
On 2017/03/28 11:14:23, språng wrote:
> Not sure this is the right place for this. Also I think there are already enum
> in place you can use, such as VideoCodecMode in common_type.h
> 
> Otherwise, prefer this be an enum class, and drop the kVideoContent_ prefix.

Done with class and prefix. However, it's the correct place to do so. Similarly
to video rotation extension. Also, it needs to be a separate enum because it's a
different thing. We can have more than one codec mode for single content type
and vice versa

https://codereview.webrtc.org/2772033002/diff/140001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2772033002/diff/140001/webrtc/common_types.h#ne...
webrtc/common_types.h:722: VideoContentTypeId videoContentType;
On 2017/03/28 11:14:23, språng wrote:
> Maybe an rtc::Optional<VideoContentTypeId> would be in place here?
> If so, maybe even refactor the ones above.

This CL is already too long. I'll leave it as todo for now.

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

https://codereview.webrtc.org/2772033002/diff/140001/webrtc/video/receive_sta...
webrtc/video/receive_statistics_proxy.cc:77: e2e_delay_max_ms_screenshare_(-1),
On 2017/03/28 11:14:23, språng wrote:
> Maybe we should report all stats and reset state if we detect a content type
> switch, similar to how we do it for the send side?

Problem here is that due to packet loss/reordering we will get some mixed
statistics in the vicinity of switch. We might make this change in the future if
we need some more metrics, it should be a separate CL.

[ilnik, 2017-03-28 14:59:15]

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

[commit-bot: I haz the power, 2017-03-28 14:59:27]

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-03-28 15:01:36]

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

[commit-bot: I haz the power, 2017-03-28 15:01:36]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  linux32_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux32_rel/builds/158)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/20262)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15476)

[ilnik, 2017-03-28 15:24:43]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-03-28 15:24:49]

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-03-28 15:24:50]

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

[commit-bot: I haz the power, 2017-03-28 15:24:51]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-webrtc-committers".
Note that this has nothing to do with OWNERS files.

[ilnik, 2017-03-28 15:25:45]

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

[commit-bot: I haz the power, 2017-03-28 15:25: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-03-28 15:30:39]

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

[commit-bot: I haz the power, 2017-03-28 15:30:40]

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

[ilnik, 2017-03-28 15:34:33]

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

[commit-bot: I haz the power, 2017-03-28 15:34:44]

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-03-28 15:43:33]

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

[commit-bot: I haz the power, 2017-03-28 15:43:34]

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

[ilnik, 2017-03-28 17:36:59]

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

[commit-bot: I haz the power, 2017-03-28 17:37:08]

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-03-28 17:59:08]

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

[commit-bot: I haz the power, 2017-03-28 17:59:09]

Dry run: This issue passed the CQ dry run.

[ilnik, 2017-03-29 11:16:42]

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

[ilnik, 2017-03-29 11:21:32]

Description was changed from

==========
Add content type information to video frames and rtp extension header.
Use it to separate UMA e2e delay metric between screenshare from video.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested

BUG=none
==========

to

==========
Add content type information to video frames and rtp extension header.
Use it to separate UMA e2e delay metric between screenshare from video.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested

BUG=webrtc:7420
==========

[nisse-webrtc, 2017-03-29 12:05:55]

https://codereview.webrtc.org/2772033002/diff/260001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame.h (right):

https://codereview.webrtc.org/2772033002/diff/260001/webrtc/api/video/video_f...
webrtc/api/video/video_frame.h:124: VideoContentType content_type_;
Please, don't bloat the VideoFrame class with this.

I would prefer if this signalling is terminated in VideoSendStream and
VideoReceiveStream, and wire it up to the rtp code from there. And higher up,
don't think about it as a per-frame property, but as state of the stream: is it
currently processing screencast or not.

[ilnik, 2017-03-29 12:43:55]

https://codereview.webrtc.org/2772033002/diff/260001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame.h (right):

https://codereview.webrtc.org/2772033002/diff/260001/webrtc/api/video/video_f...
webrtc/api/video/video_frame.h:124: VideoContentType content_type_;
On 2017/03/29 12:05:55, nisse-webrtc wrote:
> Please, don't bloat the VideoFrame class with this.
> 
> I would prefer if this signalling is terminated in VideoSendStream and
> VideoReceiveStream, and wire it up to the rtp code from there. And higher up,
> don't think about it as a per-frame property, but as state of the stream: is
it
> currently processing screencast or not.

Are you OK with having it in EncodedImage?

[nisse-webrtc, 2017-03-29 12:59:00]

On 2017/03/29 12:43:55, ilnik wrote:

> Are you OK with having it in EncodedImage?

Since EncodedImage (and its half-dozen subclasses) is such a mess anyway,
it doesn't make much difference ;-/

If there's some reasonable way to avoid it, that would be nice, but I don't have
a strong opinion.

In principle, I think EncodedImage should hold all per-frame info which should
go onto the wire.
And it's questionable if content type really makes sense per-frame (the same
could perhaps be said for rotation,
but rotation has stricter requirements that the metadata really is in sync with
the stream of frames).

An alternative might be to have a flag on some of the RTP objects saying if the
extension should be used, 
and what the current content type is.

[ilnik, 2017-03-30 11:47:38]

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

[commit-bot: I haz the power, 2017-03-30 11:47:50]

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-03-30 12:00:59]

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

[commit-bot: I haz the power, 2017-03-30 12:01:00]

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

[ilnik, 2017-03-30 14:09:38]

On 2017/03/29 12:59:00, nisse-webrtc wrote:
> On 2017/03/29 12:43:55, ilnik wrote:
> 
> > Are you OK with having it in EncodedImage?
> 
> Since EncodedImage (and its half-dozen subclasses) is such a mess anyway,
> it doesn't make much difference ;-/
> 
> If there's some reasonable way to avoid it, that would be nice, but I don't
have
> a strong opinion.
> 
> In principle, I think EncodedImage should hold all per-frame info which should
> go onto the wire.
> And it's questionable if content type really makes sense per-frame (the same
> could perhaps be said for rotation,
> but rotation has stricter requirements that the metadata really is in sync
with
> the stream of frames).
> 
> An alternative might be to have a flag on some of the RTP objects saying if
the
> extension should be used, 
> and what the current content type is.

Done. Now VideoFrame is not affected. I cleanedup and removed some files from
the CL. 
As discussed, extra header is sent only on key frames.

[ilnik, 2017-03-30 14:12:05]

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

[commit-bot: I haz the power, 2017-03-30 14:12:11]

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-03-30 14:34:49]

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

[commit-bot: I haz the power, 2017-03-30 14:34:50]

Dry run: This issue passed the CQ dry run.

[ilnik, 2017-04-03 07:46:47]

On 2017/03/30 14:09:38, ilnik wrote:
> On 2017/03/29 12:59:00, nisse-webrtc wrote:
> > On 2017/03/29 12:43:55, ilnik wrote:
> > 
> > > Are you OK with having it in EncodedImage?
> > 
> > Since EncodedImage (and its half-dozen subclasses) is such a mess anyway,
> > it doesn't make much difference ;-/
> > 
> > If there's some reasonable way to avoid it, that would be nice, but I don't
> have
> > a strong opinion.
> > 
> > In principle, I think EncodedImage should hold all per-frame info which
should
> > go onto the wire.
> > And it's questionable if content type really makes sense per-frame (the same
> > could perhaps be said for rotation,
> > but rotation has stricter requirements that the metadata really is in sync
> with
> > the stream of frames).
> > 
> > An alternative might be to have a flag on some of the RTP objects saying if
> the
> > extension should be used, 
> > and what the current content type is.
> 
> Done. Now VideoFrame is not affected. I cleanedup and removed some files from
> the CL. 
> As discussed, extra header is sent only on key frames.

Bump.

[nisse-webrtc, 2017-04-03 12:07:18]

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

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:17: kDefault = 0,
If these are the numerical values that are intended to go onto the wire, I think
that deserves a comment. Or maybe the on-the-wire values should be defined
elsewhere.

I think it would be good to think a bit more about what types make sense and
what they mean. "kDefault" doesn't make much sense to me. I think something like
kUnknown, kCameraFeed, kScreenshare would make more sense. And "unknown" could
be represented with extension absent.

And then, what content type should we have for a screenshare capturing a live
camera?

Or if we intend this to be used only for stats, we don't need to specify any
semantics, the sending endpoint could just set distinct numerical values for
whatever types of content they like to separate in the stats.

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/common_types.h#ne...
webrtc/common_types.h:721: // a corresponding bool flag.
Can you do that in this cl? Should be an easy cleanup.

Or in the rotation case, maybe it would work to just set rotation to 0 when not
present?

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:104: // TODO(ilnik):
Think of a better URI.
I don't know how we allocate id:s like this. Stefan?

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:66: VideoContentType
content_type) = 0;
The implementation of this call uses this info for stats only?

It might make sense to collect content type and the qp value into a struct. Not
sure what qp is used for here, if that's also for stats only?

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.h:88:
VideoCodecMode mode_;
What's VideoCodecMode?

[danilchap, 2017-04-03 12:19:54]

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:105: static constexpr
const char* kUri = "urn:webrtc:video-content-type";
http://www.webrtc.org/experiments/rtp-hdrext/video-content-type
might be better uri for an experemental extension.
You can also feel the page above with some details about the extension.

[ilnik, 2017-04-03 14:03:43]

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

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:17: kDefault = 0,
On 2017/04/03 12:07:18, nisse-webrtc wrote:
> If these are the numerical values that are intended to go onto the wire, I
think
> that deserves a comment. Or maybe the on-the-wire values should be defined
> elsewhere.
> 
> I think it would be good to think a bit more about what types make sense and
> what they mean. "kDefault" doesn't make much sense to me. I think something
like
> kUnknown, kCameraFeed, kScreenshare would make more sense. And "unknown" could
> be represented with extension absent.
> 
> And then, what content type should we have for a screenshare capturing a live
> camera?
> 
> Or if we intend this to be used only for stats, we don't need to specify any
> semantics, the sending endpoint could just set distinct numerical values for
> whatever types of content they like to separate in the stats.

These are intended only for stats. Some kind of values are chosen because they
have to be fixed explicitly at end-points. kDefault makes sense, as unspecified
content type. It's set like that if no extension is present. It means: not
separated in a different prefix in UMA. Maybe kUnspecified may be better?

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/common_types.h#ne...
webrtc/common_types.h:721: // a corresponding bool flag.
On 2017/04/03 12:07:18, nisse-webrtc wrote:
> Can you do that in this cl? Should be an easy cleanup.
> 
> Or in the rotation case, maybe it would work to just set rotation to 0 when
not
> present?

The rotation is set up as a 0 if not present while parsing that header.

I will do that in another CL. I don't want to bload this one anymore.

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:105: static constexpr
const char* kUri = "urn:webrtc:video-content-type";
On 2017/04/03 12:19:54, danilchap wrote:
> http://www.webrtc.org/experiments/rtp-hdrext/video-content-type
> might be better uri for an experemental extension.
> You can also feel the page above with some details about the extension.

Changed the kUri. But how can I fill that page with information?

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:66: VideoContentType
content_type) = 0;
On 2017/04/03 12:07:18, nisse-webrtc wrote:
> The implementation of this call uses this info for stats only?
> 
> It might make sense to collect content type and the qp value into a struct.
Not
> sure what qp is used for here, if that's also for stats only?

Yes, stats only. Two parameters do not warrant a separate structure, in my
opinion.

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.h:88:
VideoCodecMode mode_;
On 2017/04/03 12:07:18, nisse-webrtc wrote:
> What's VideoCodecMode?

Currently it's screenshare or realtime. Set in encoder config. That's the thing
we want to communicate to the receiver. It's easily available in webrtc
encoders, but ignored in the SDK.

[ilnik, 2017-04-05 11:24:16]

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

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:17: kDefault = 0,
On 2017/04/03 14:03:42, ilnik wrote:
> On 2017/04/03 12:07:18, nisse-webrtc wrote:
> > If these are the numerical values that are intended to go onto the wire, I
> think
> > that deserves a comment. Or maybe the on-the-wire values should be defined
> > elsewhere.
> > 
> > I think it would be good to think a bit more about what types make sense and
> > what they mean. "kDefault" doesn't make much sense to me. I think something
> like
> > kUnknown, kCameraFeed, kScreenshare would make more sense. And "unknown"
could
> > be represented with extension absent.
> > 
> > And then, what content type should we have for a screenshare capturing a
live
> > camera?
> > 
> > Or if we intend this to be used only for stats, we don't need to specify any
> > semantics, the sending endpoint could just set distinct numerical values for
> > whatever types of content they like to separate in the stats.
> 
> These are intended only for stats. Some kind of values are chosen because they
> have to be fixed explicitly at end-points. kDefault makes sense, as
unspecified
> content type. It's set like that if no extension is present. It means: not
> separated in a different prefix in UMA. Maybe kUnspecified may be better?

Nisse, so, what are you thoughts on that? Should I rename kDefault to
kUnspecified?

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:66: VideoContentType
content_type) = 0;
On 2017/04/03 14:03:43, ilnik wrote:
> On 2017/04/03 12:07:18, nisse-webrtc wrote:
> > The implementation of this call uses this info for stats only?
> > 
> > It might make sense to collect content type and the qp value into a struct.
> Not
> > sure what qp is used for here, if that's also for stats only?
> 
> Yes, stats only. Two parameters do not warrant a separate structure, in my
> opinion.

Nisse, do you strongly disagree with leaving it as a two parameters and insist
on adding a structure here?

[nisse-webrtc, 2017-04-05 11:46:36]

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

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:17: kDefault = 0,
On 2017/04/05 11:24:16, ilnik wrote:
> On 2017/04/03 14:03:42, ilnik wrote:
> > On 2017/04/03 12:07:18, nisse-webrtc wrote:
> > > If these are the numerical values that are intended to go onto the wire, I
> > think
> > > that deserves a comment. Or maybe the on-the-wire values should be defined
> > > elsewhere.
> > > 
> > > I think it would be good to think a bit more about what types make sense
and
> > > what they mean. "kDefault" doesn't make much sense to me. I think
something
> > like
> > > kUnknown, kCameraFeed, kScreenshare would make more sense. And "unknown"
> could
> > > be represented with extension absent.
> > > 
> > > And then, what content type should we have for a screenshare capturing a
> live
> > > camera?
> > > 
> > > Or if we intend this to be used only for stats, we don't need to specify
any
> > > semantics, the sending endpoint could just set distinct numerical values
for
> > > whatever types of content they like to separate in the stats.
> > 
> > These are intended only for stats. Some kind of values are chosen because
they
> > have to be fixed explicitly at end-points. kDefault makes sense, as
> unspecified
> > content type. It's set like that if no extension is present. It means: not
> > separated in a different prefix in UMA. Maybe kUnspecified may be better?
> 
> Nisse, so, what are you thoughts on that? Should I rename kDefault to
> kUnspecified?

On the wire, would it make sense to represent "unspecified" as an absent
extension? Then change from screenshare to non-screenshare would be a key frame
with the extension missing.

For the enum, I prefer kUnspecified or kUnknown over kDefault.

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:66: VideoContentType
content_type) = 0;
On 2017/04/05 11:24:16, ilnik wrote:
> On 2017/04/03 14:03:43, ilnik wrote:
> > On 2017/04/03 12:07:18, nisse-webrtc wrote:
> > > The implementation of this call uses this info for stats only?
> > > 
> > > It might make sense to collect content type and the qp value into a
struct.
> > Not
> > > sure what qp is used for here, if that's also for stats only?
> > 
> > Yes, stats only. Two parameters do not warrant a separate structure, in my
> > opinion.
> 
> Nisse, do you strongly disagree with leaving it as a two parameters and insist
> on adding a structure here?

I don't insist.

[ilnik, 2017-04-05 11:58:02]

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

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:17: kDefault = 0,
On 2017/04/05 11:46:36, nisse-webrtc wrote:
> On 2017/04/05 11:24:16, ilnik wrote:
> > On 2017/04/03 14:03:42, ilnik wrote:
> > > On 2017/04/03 12:07:18, nisse-webrtc wrote:
> > > > If these are the numerical values that are intended to go onto the wire,
I
> > > think
> > > > that deserves a comment. Or maybe the on-the-wire values should be
defined
> > > > elsewhere.
> > > > 
> > > > I think it would be good to think a bit more about what types make sense
> and
> > > > what they mean. "kDefault" doesn't make much sense to me. I think
> something
> > > like
> > > > kUnknown, kCameraFeed, kScreenshare would make more sense. And "unknown"
> > could
> > > > be represented with extension absent.
> > > > 
> > > > And then, what content type should we have for a screenshare capturing a
> > live
> > > > camera?
> > > > 
> > > > Or if we intend this to be used only for stats, we don't need to specify
> any
> > > > semantics, the sending endpoint could just set distinct numerical values
> for
> > > > whatever types of content they like to separate in the stats.
> > > 
> > > These are intended only for stats. Some kind of values are chosen because
> they
> > > have to be fixed explicitly at end-points. kDefault makes sense, as
> > unspecified
> > > content type. It's set like that if no extension is present. It means: not
> > > separated in a different prefix in UMA. Maybe kUnspecified may be better?
> > 
> > Nisse, so, what are you thoughts on that? Should I rename kDefault to
> > kUnspecified?
> 
> On the wire, would it make sense to represent "unspecified" as an absent
> extension? Then change from screenshare to non-screenshare would be a key
frame
> with the extension missing.
> 
> For the enum, I prefer kUnspecified or kUnknown over kDefault.

It makes sense to not send the extension for unspecified content_type. I will
change that shortly. I will also rename the first value in the enum. However, we
still need this value in the enum, because something have to be stored in the
encoded_image. Having there a separate flag would be a bad design. Also, for the
future, there is 1-byte value sent in the extension. That's in case we will add
additional content types in the future. And also I don't think it will work with
0 value bytes in the extension.

[ilnik, 2017-04-05 12:02:55]

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

[commit-bot: I haz the power, 2017-04-05 12:03:06]

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-04-05 12:14:56]

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

[commit-bot: I haz the power, 2017-04-05 12:14:57]

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

[danilchap, 2017-04-05 12:16:52]

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

https://codereview.webrtc.org/2772033002/diff/400001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:17: kDefault = 0,
On 2017/04/05 11:58:02, ilnik wrote:
> On 2017/04/05 11:46:36, nisse-webrtc wrote:
> > On 2017/04/05 11:24:16, ilnik wrote:
> > > On 2017/04/03 14:03:42, ilnik wrote:
> > > > On 2017/04/03 12:07:18, nisse-webrtc wrote:
> > > > > If these are the numerical values that are intended to go onto the
wire,
> I
> > > > think
> > > > > that deserves a comment. Or maybe the on-the-wire values should be
> defined
> > > > > elsewhere.
> > > > > 
> > > > > I think it would be good to think a bit more about what types make
sense
> > and
> > > > > what they mean. "kDefault" doesn't make much sense to me. I think
> > something
> > > > like
> > > > > kUnknown, kCameraFeed, kScreenshare would make more sense. And
"unknown"
> > > could
> > > > > be represented with extension absent.
> > > > > 
> > > > > And then, what content type should we have for a screenshare capturing
a
> > > live
> > > > > camera?
> > > > > 
> > > > > Or if we intend this to be used only for stats, we don't need to
specify
> > any
> > > > > semantics, the sending endpoint could just set distinct numerical
values
> > for
> > > > > whatever types of content they like to separate in the stats.
> > > > 
> > > > These are intended only for stats. Some kind of values are chosen
because
> > they
> > > > have to be fixed explicitly at end-points. kDefault makes sense, as
> > > unspecified
> > > > content type. It's set like that if no extension is present. It means:
not
> > > > separated in a different prefix in UMA. Maybe kUnspecified may be
better?
> > > 
> > > Nisse, so, what are you thoughts on that? Should I rename kDefault to
> > > kUnspecified?
> > 
> > On the wire, would it make sense to represent "unspecified" as an absent
> > extension? Then change from screenshare to non-screenshare would be a key
> frame
> > with the extension missing.
> > 
> > For the enum, I prefer kUnspecified or kUnknown over kDefault.
> 
> It makes sense to not send the extension for unspecified content_type. I will
> change that shortly. I will also rename the first value in the enum. However,
we
> still need this value in the enum, because something have to be stored in the
> encoded_image. Having there a separate flag would be a bad design. Also, for
the
> future, there is 1-byte value sent in the extension. That's in case we will
add
> additional content types in the future. And also I don't think it will work
with
> 0 value bytes in the extension.

Yes, it is not possible to send extension of size 0.

[nisse-webrtc, 2017-04-05 12:35:14]

On 2017/04/05 11:58:02, ilnik wrote:
> It makes sense to not send the extension for unspecified content_type. I will
> change that shortly. I will also rename the first value in the enum. However,
we
> still need this value in the enum, because something have to be stored in the
> encoded_image. Having there a separate flag would be a bad design.

Using rtc::Optional would make some sense, but perhaps overkill in this case.

In the specification of the extension, I think it makes sense to allow the
kUnspecified 
value on the wire, and specify that it has the same meaning as the extension
being absent.

[ilnik, 2017-04-05 12:48:45]

On 2017/04/05 12:35:14, nisse-webrtc wrote:
> On 2017/04/05 11:58:02, ilnik wrote:
> > It makes sense to not send the extension for unspecified content_type. I
will
> > change that shortly. I will also rename the first value in the enum.
However,
> we
> > still need this value in the enum, because something have to be stored in
the
> > encoded_image. Having there a separate flag would be a bad design.
> 
> Using rtc::Optional would make some sense, but perhaps overkill in this case.
> 
> In the specification of the extension, I think it makes sense to allow the
> kUnspecified 
> value on the wire, and specify that it has the same meaning as the extension
> being absent.

I will specify that.

[ilnik, 2017-04-06 07:43:44]

On 2017/04/05 12:48:45, ilnik wrote:
> On 2017/04/05 12:35:14, nisse-webrtc wrote:
> > On 2017/04/05 11:58:02, ilnik wrote:
> > > It makes sense to not send the extension for unspecified content_type. I
> will
> > > change that shortly. I will also rename the first value in the enum.
> However,
> > we
> > > still need this value in the enum, because something have to be stored in
> the
> > > encoded_image. Having there a separate flag would be a bad design.
> > 
> > Using rtc::Optional would make some sense, but perhaps overkill in this
case.
> > 
> > In the specification of the extension, I think it makes sense to allow the
> > kUnspecified 
> > value on the wire, and specify that it has the same meaning as the extension
> > being absent.
> 
> I will specify that.

Extension is now described at
http://webrtc.org/experiments/rtp-hdrext/content-type

[ilnik, 2017-04-06 09:01:10]

On 2017/04/06 07:43:44, ilnik wrote:
> On 2017/04/05 12:48:45, ilnik wrote:
> > On 2017/04/05 12:35:14, nisse-webrtc wrote:
> > > On 2017/04/05 11:58:02, ilnik wrote:
> > > > It makes sense to not send the extension for unspecified content_type. I
> > will
> > > > change that shortly. I will also rename the first value in the enum.
> > However,
> > > we
> > > > still need this value in the enum, because something have to be stored
in
> > the
> > > > encoded_image. Having there a separate flag would be a bad design.
> > > 
> > > Using rtc::Optional would make some sense, but perhaps overkill in this
> case.
> > > 
> > > In the specification of the extension, I think it makes sense to allow the
> > > kUnspecified 
> > > value on the wire, and specify that it has the same meaning as the
extension
> > > being absent.
> > 
> > I will specify that.
> 
> Extension is now described at
> http://webrtc.org/experiments/rtp-hdrext/content-type

I think all concerns in the previous comments are resolved, except for
refactoring a VideoContentTypeExtension. I will do it in the separate CL. Please
make another pass over this CL.

[nisse-webrtc, 2017-04-06 09:23:22]

lgtm.

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:214: *content_type =
static_cast<VideoContentType>(data[0]);
Maybe return false if the value on the wire is invalid?

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:466:
static_cast<VideoContentType>(ptr[0]);
Here too, might want to reject invalid values.

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/video_cap...
File webrtc/modules/video_capture/video_capture_impl.cc (right):

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:207: } else {
In coming cl's, try to not include unrelated formatting improvements like this,
it makes it harder to review the relevant changes.

Both the -p option to git add and git reset, and emacs' vc support, make it
reasonably easy to add or revert selected parts of a change.

[ilnik, 2017-04-06 10:04:49]

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:214: *content_type =
static_cast<VideoContentType>(data[0]);
On 2017/04/06 09:23:22, nisse-webrtc wrote:
> Maybe return false if the value on the wire is invalid?

Done.

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:466:
static_cast<VideoContentType>(ptr[0]);
On 2017/04/06 09:23:22, nisse-webrtc wrote:
> Here too, might want to reject invalid values.

Done.

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/video_cap...
File webrtc/modules/video_capture/video_capture_impl.cc (right):

https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:207: } else {
On 2017/04/06 09:23:22, nisse-webrtc wrote:
> In coming cl's, try to not include unrelated formatting improvements like
this,
> it makes it harder to review the relevant changes.
> 
> Both the -p option to git add and git reset, and emacs' vc support, make it
> reasonably easy to add or revert selected parts of a change.

Yes, sorry about that. There were some meaningful changes here then content type
were added to VideoFrame, and it caused a lot of formatting warnings. Now
meaningful changes are gone, but formatting remains.

[ilnik, 2017-04-06 10:20:20]

On 2017/04/06 10:04:49, ilnik wrote:
>
https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
> File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):
> 
>
https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
> webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:214: *content_type =
> static_cast<VideoContentType>(data[0]);
> On 2017/04/06 09:23:22, nisse-webrtc wrote:
> > Maybe return false if the value on the wire is invalid?
> 
> Done.
> 
>
https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
> File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):
> 
>
https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/rtp_rtcp/...
> webrtc/modules/rtp_rtcp/source/rtp_utility.cc:466:
> static_cast<VideoContentType>(ptr[0]);
> On 2017/04/06 09:23:22, nisse-webrtc wrote:
> > Here too, might want to reject invalid values.
> 
> Done.
> 
>
https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/video_cap...
> File webrtc/modules/video_capture/video_capture_impl.cc (right):
> 
>
https://codereview.webrtc.org/2772033002/diff/480001/webrtc/modules/video_cap...
> webrtc/modules/video_capture/video_capture_impl.cc:207: } else {
> On 2017/04/06 09:23:22, nisse-webrtc wrote:
> > In coming cl's, try to not include unrelated formatting improvements like
> this,
> > it makes it harder to review the relevant changes.
> > 
> > Both the -p option to git add and git reset, and emacs' vc support, make it
> > reasonably easy to add or revert selected parts of a change.
> 
> Yes, sorry about that. There were some meaningful changes here then content
type
> were added to VideoFrame, and it caused a lot of formatting warnings. Now
> meaningful changes are gone, but formatting remains.

Nisse, please confirm that you like how I addressed your latest comments.

[ilnik, 2017-04-06 10:39:08]

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

[commit-bot: I haz the power, 2017-04-06 10:39:15]

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

[nisse-webrtc, 2017-04-06 10:55:14]

Still lgtm.

[commit-bot: I haz the power, 2017-04-06 11:07:06]

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

[commit-bot: I haz the power, 2017-04-06 11:07:08]

Dry run: This issue passed the CQ dry run.

[ilnik, 2017-04-06 11:14:31]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-04-06 11:14:37]

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-04-06 11:19:45]

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

[commit-bot: I haz the power, 2017-04-06 11:19:47]

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

[ilnik, 2017-04-06 11:24:03]

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

[ilnik, 2017-04-06 11:28:43]

On 2017/04/06 11:19:47, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15826)

Magnus, please have a quick look at this CL. it is already reviewed by Niels,
but I need a LGTM for root directory.

[ilnik, 2017-04-10 09:34:47]

Tommi, could you please give a rubberstamp lgtm? The issue was reviewed by
Sprang and Nisse.

[nisse-webrtc, 2017-04-10 09:41:23]

Can you clarify cl title and description, now that it's
only encoded frames that get the content type attached?

[ilnik, 2017-04-10 09:43:48]

Description was changed from

==========
Add content type information to video frames and rtp extension header.
Use it to separate UMA e2e delay metric between screenshare from video.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested

BUG=webrtc:7420
==========

to

==========
Add content type information to Encoded Images and add corresponding RTP
extension header.
Use it to separate UMA e2e delay metric between screenshare from video.
Content type extension is set based on encoder settings and processed and
decoders.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested.

BUG=webrtc:7420
==========

[ilnik, 2017-04-10 09:44:09]

On 2017/04/10 09:41:23, nisse-webrtc wrote:
> Can you clarify cl title and description, now that it's
> only encoded frames that get the content type attached?

Done.

[nisse-webrtc, 2017-04-10 09:47:47]

On 2017/04/10 09:44:09, ilnik wrote:
> On 2017/04/10 09:41:23, nisse-webrtc wrote:
> > Can you clarify cl title and description, now that it's
> > only encoded frames that get the content type attached?
> 
> Done.

Thanks. Still lgtm.

[ilnik, 2017-04-10 09:49:32]

On 2017/04/10 09:47:47, nisse-webrtc wrote:
> On 2017/04/10 09:44:09, ilnik wrote:
> > On 2017/04/10 09:41:23, nisse-webrtc wrote:
> > > Can you clarify cl title and description, now that it's
> > > only encoded frames that get the content type attached?
> > 
> > Done.
> 
> Thanks. Still lgtm.

Thanks. Sadly, your approval alone is not enough to land this CL.

[ilnik, 2017-04-10 09:53:46]

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

[commit-bot: I haz the power, 2017-04-10 09:53:52]

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-04-10 10:43:39]

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

[commit-bot: I haz the power, 2017-04-10 10:43:40]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-04-10 10:59:18]

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

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:17: kUnspecified = 0,
This should be UNSPECIFIED, SCREENSHARE, TOTAL_CONTENT_TYPES as per style guide

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/config.h#newcode92
webrtc/config.h:92: static const char* kVideoContentTypeUri;
can this be declared inline as 
static const char kVideoContentTypeUri[] = "...";

Do we actually need the pointer to the string or is the string itself what we
need?

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:237: } else {
no need for |else|

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:244: data[0] =
static_cast<uint8_t>(content_type);
is there a safe cast method that we can use instead of static_cast?

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:106: static constexpr
const char* kUri =
static constexpr const char kUri[] =

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:329:
video_header->content_type != VideoContentType::kUnspecified)
use {}

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:465:
static_cast<uint8_t>(VideoContentType::kTotalContentTypes)) {
safe cast?

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
File webrtc/modules/video_capture/video_capture_impl.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:205: if
(_incomingFrameTimesNanos[0] / rtc::kNumNanosecsPerMicrosec == 0) {
nit: use parenthesis for clarity

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:209: for (int i =
(kFrameRateCountHistorySize - 2); i >= 0; i--) {
--i

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:219: for (num = 1; num <
(kFrameRateCountHistorySize - 1); num++) {
++num

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:370: if (mode_ ==
kScreensharing)
nit (2 lines, one assignment instead of 4 lines, two assignments):
encoded_image_.content_type_ = (mode_ == kScreensharing) ?
    VideoContentType::kScreenshare : VideoContentType::kUnspecified;

You could do the same elsewhere where the same check is being done.

[ilnik, 2017-04-10 12:47:43]

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

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/api/video/video_c...
webrtc/api/video/video_content_type.h:17: kUnspecified = 0,
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> This should be UNSPECIFIED, SCREENSHARE, TOTAL_CONTENT_TYPES as per style
guide

Done.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/config.h#newcode92
webrtc/config.h:92: static const char* kVideoContentTypeUri;
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> can this be declared inline as 
> static const char kVideoContentTypeUri[] = "...";
> 
> Do we actually need the pointer to the string or is the string itself what we
> need?

I think it will be better if we leave it in the same way as in the other
extensions, and refactor them all later.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:237: } else {
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> no need for |else|

Done.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:244: data[0] =
static_cast<uint8_t>(content_type);
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> is there a safe cast method that we can use instead of static_cast?

I don't want to write one, as it will introduce a second set of constants.
However, now enum is based on uint8_t and this conversion is totally safe.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:106: static constexpr
const char* kUri =
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> static constexpr const char kUri[] =

I prefer to leave it as in other extensions and refactor it all later.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:329:
video_header->content_type != VideoContentType::kUnspecified)
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> use {}

Done.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:465:
static_cast<uint8_t>(VideoContentType::kTotalContentTypes)) {
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> safe cast?

It's safe now. VideoContentType underlying type is uint8_t now.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
File webrtc/modules/video_capture/video_capture_impl.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:205: if
(_incomingFrameTimesNanos[0] / rtc::kNumNanosecsPerMicrosec == 0) {
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> nit: use parenthesis for clarity

Acknowledged.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:209: for (int i =
(kFrameRateCountHistorySize - 2); i >= 0; i--) {
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> --i

Done.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:219: for (num = 1; num <
(kFrameRateCountHistorySize - 1); num++) {
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> ++num

Done.

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:370: if (mode_ ==
kScreensharing)
On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> nit (2 lines, one assignment instead of 4 lines, two assignments):
> encoded_image_.content_type_ = (mode_ == kScreensharing) ?
>     VideoContentType::kScreenshare : VideoContentType::kUnspecified;
> 
> You could do the same elsewhere where the same check is being done.

Done.

[ilnik, 2017-04-10 12:56:21]

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

[commit-bot: I haz the power, 2017-04-10 12:56:32]

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-04-10 13:11:10]

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

[commit-bot: I haz the power, 2017-04-10 13:11:12]

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/20637)

[ilnik, 2017-04-10 13:53:04]

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

[commit-bot: I haz the power, 2017-04-10 13:53:16]

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-04-10 14:06:02]

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

[commit-bot: I haz the power, 2017-04-10 14:06:04]

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/20650)

[ilnik, 2017-04-10 14:13:59]

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

[commit-bot: I haz the power, 2017-04-10 14:14:12]

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-04-10 14:22:02]

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

[commit-bot: I haz the power, 2017-04-10 14:22:04]

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

[ilnik, 2017-04-10 14:53:37]

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

[commit-bot: I haz the power, 2017-04-10 14:53:40]

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-04-10 18:15:00]

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

[commit-bot: I haz the power, 2017-04-10 18:15:02]

Dry run: Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build hasn't started
yet, builder probably lacks capacity)

[ilnik, 2017-04-11 07:48:00]

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

[commit-bot: I haz the power, 2017-04-11 07:48:09]

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-04-11 07:52:20]

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

[commit-bot: I haz the power, 2017-04-11 07:52:21]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-04-11 07:59:06]

lgtm

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
File webrtc/modules/video_capture/video_capture_impl.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:205: if
(_incomingFrameTimesNanos[0] / rtc::kNumNanosecsPerMicrosec == 0) {
On 2017/04/10 12:47:42, ilnik wrote:
> On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> > nit: use parenthesis for clarity
> 
> Acknowledged.

I'm not sure if you meant to do this or prefer not to.

https://codereview.webrtc.org/2772033002/diff/580001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2772033002/diff/580001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:465:
static_cast<uint8_t>(VideoContentType::TOTAL_CONTENT_TYPES)) {
Is the cast still needed?

[ilnik, 2017-04-11 08:10:31]

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
File webrtc/modules/video_capture/video_capture_impl.cc (right):

https://codereview.webrtc.org/2772033002/diff/560001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:205: if
(_incomingFrameTimesNanos[0] / rtc::kNumNanosecsPerMicrosec == 0) {
On 2017/04/11 07:59:06, tommi ooo (webrtc) wrote:
> On 2017/04/10 12:47:42, ilnik wrote:
> > On 2017/04/10 10:59:18, tommi ooo (webrtc) wrote:
> > > nit: use parenthesis for clarity
> > 
> > Acknowledged.
> 
> I'm not sure if you meant to do this or prefer not to.

I just don't understand how parenthesis could add more clarity here. Did you
mean to do: (incomingFrameTimesNanos[0] / rtc::kNumNanosecsPerMicrosec) == 0?

https://codereview.webrtc.org/2772033002/diff/580001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2772033002/diff/580001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:465:
static_cast<uint8_t>(VideoContentType::TOTAL_CONTENT_TYPES)) {
On 2017/04/11 07:59:06, tommi ooo (webrtc) wrote:
> Is the cast still needed?

Yes, enum class can't be implicitly casted to an underlying type. However,
static cast is totally safe here.

[ilnik, 2017-04-11 08:42:55]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-04-11 08:42:56]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2772033002/#ps600001 (title:
"Fix typo, leading to failed video catpure test")

[commit-bot: I haz the power, 2017-04-11 08:43:04]

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-04-11 08:46:05]

CQ is committing da patch.

Bot data: {"patchset_id": 600001, "attempt_start_ts": 1491900175908220,
"parent_rev": "93cda2ebde909ad9cc424690f44f59a6fb84b149", "commit_rev":
"64e739aeae5629cbbebf2a19e1d3e6b452bb6d0b"}

[commit-bot: I haz the power, 2017-04-11 08:46:10]

Description was changed from

==========
Add content type information to Encoded Images and add corresponding RTP
extension header.
Use it to separate UMA e2e delay metric between screenshare from video.
Content type extension is set based on encoder settings and processed and
decoders.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested.

BUG=webrtc:7420
==========

to

==========
Add content type information to Encoded Images and add corresponding RTP
extension header.
Use it to separate UMA e2e delay metric between screenshare from video.
Content type extension is set based on encoder settings and processed and
decoders.

Also,
Fix full-stack-tests to calculate RTT correctly, so new metric could be tested.

BUG=webrtc:7420

Review-Url: https://codereview.webrtc.org/2772033002
Cr-Commit-Position: refs/heads/master@{#17640}
Committed:
https://chromium.googlesource.com/external/webrtc/+/64e739aeae5629cbbebf2a19e...
==========

[commit-bot: I haz the power, 2017-04-11 08:46:12]

Committed patchset #31 (id:600001) as
https://chromium.googlesource.com/external/webrtc/+/64e739aeae5629cbbebf2a19e...

[ilnik, 2017-04-11 10:59:21]

A revert of this CL (patchset #31 id:600001) has been created in
https://codereview.webrtc.org/2816463002/ by ilnik@webrtc.org.

The reason for reverting is: Breaks dependent projects..