Add cricket::VideoFrame::frame_id() and set it to RTP timestamp.

Add cricket::VideoFrame::transport_frame_id() and set it to RTP timestamp.

Passing transport_frame_id() to VideoSink will allow to identify incoming video
frames, which will make it possible to correlate video frames on the
sender and on the receiver.

BUG=chromium:621691
R=mflodman@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/19ee1e6eb102b2282a9993d6c373c7185ad9ba34
Cr-Commit-Position: refs/heads/master@{#13596}

[Sergey Ulanov, 2016-06-21 21:38:29]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-06-21 21:41:03]

sergeyu@chromium.org changed reviewers:
+ mflodman@webrtc.org, nisse@webrtc.org

[nisse-webrtc, 2016-06-22 07:46:37]

Hi,

I don't object to adding a frame id, but I'd like to know why it is needed. I'd
like to keep the VideoFrame class lean.

I think there's code elsewhere that uses the timestamp as an id, which sounds a
bit kludgy, but it would be good to identify frames in the same way throughout
the code. So if an id is added, those use cases should be kept in mind.

Are there wraparound issues when using a 32-bit id?

What will break in case id:s are reused in unexpected ways? If they're copied
from received rtp messages, they obviously can't be trusted. Using a sequence
number incremented locally might be more robust.

For frame timestamps, my plan is to try to get away with a One True Timestamp
per frame (rather then threee like in webrtc::VideoFrame), which will be
microseconds in rtc:TimeMicros timescale. See
https://bugs.chromium.org/p/webrtc/issues/detail?id=5740

The filter in https://codereview.webrtc.org/2017443003/ could perhaps be reused
for translating rtp timestamps to local time (after dealing with wraparound).
But unless someone else wants to adopt the timestamping refactoring work, there
won't be much progress during the summer.

https://codereview.webrtc.org/2088953002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (left):

https://codereview.webrtc.org/2088953002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2435: const WebRtcVideoFrame
render_frame(
Why drop const?

[Sergey Ulanov, 2016-06-22 21:22:30]

On 2016/06/22 07:46:37, nisse-webrtc (ooo August 15) wrote:
> Hi,
> 
> I don't object to adding a frame id, but I'd like to know why it is needed.
I'd
> like to keep the VideoFrame class lean.

I've sent you the relevant design doc. Basically I'd like to be able to measure
latency for each frame and to do that we need to be able to identify frames
somehow on both ends of connection.

> 
> I think there's code elsewhere that uses the timestamp as an id, which sounds
a
> bit kludgy, but it would be good to identify frames in the same way throughout
> the code. So if an id is added, those use cases should be kept in mind.
> 
> Are there wraparound issues when using a 32-bit id?

RTP code it supposed to handle wraparound timestamp. Code that uses frame_id()
will need to handle the case when frame_id() is reused.


> What will break in case id:s are reused in unexpected ways? If they're copied
> from received rtp messages, they obviously can't be trusted. Using a sequence
> number incremented locally might be more robust.

Not sure what you mean when you say it can't be trusted. Generating sequence
number on the receiver defeats the purpose. We need some way to uniquely
identify frames on both sender and the receiver and the ID has to be the same on
both ends. RTP timestamp is the natural way to do that, but let me know if you
have other suggestions.

> 
> For frame timestamps, my plan is to try to get away with a One True Timestamp
> per frame (rather then threee like in webrtc::VideoFrame), which will be
> microseconds in rtc:TimeMicros timescale. See
> https://bugs.chromium.org/p/webrtc/issues/detail?id=5740

I suggest to either use rtc::TimeDelta or add something similar to chromiums
base::TimeTicks. That helps to avoid issues with units conversion and potential
bugs around it.

> 
> The filter in https://codereview.webrtc.org/2017443003/ could perhaps be
reused
> for translating rtp timestamps to local time (after dealing with wraparound).
> But unless someone else wants to adopt the timestamping refactoring work,
there
> won't be much progress during the summer.

https://codereview.webrtc.org/2088953002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (left):

https://codereview.webrtc.org/2088953002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2435: const WebRtcVideoFrame
render_frame(
On 2016/06/22 07:46:36, nisse-webrtc (ooo August 15) wrote:
> Why drop const?

I just don't think it's useful there, but please let me know if there is a
reason it needs to be there.

[Sergey Ulanov, 2016-07-26 23:38:17]

sergeyu@chromium.org changed reviewers:
+ stefan@webrtc.org

[Sergey Ulanov, 2016-07-26 23:38:18]

+stefan@webrtc.org
Stefan, can you please take a look while Magnus and Niels are OOO?

[stefan-webrtc, 2016-07-28 08:54:49]

https://codereview.webrtc.org/2088953002/diff/20001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2088953002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:38: virtual uint32_t frame_id() const = 0;
Maybe add "transport" to the name to make it more clear that this is something
that only exists after the frame has been sent (or received) over the network
since it's set by the transport layer.

[Sergey Ulanov, 2016-07-29 00:10:22]

https://codereview.webrtc.org/2088953002/diff/20001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2088953002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:38: virtual uint32_t frame_id() const = 0;
On 2016/07/28 08:54:49, stefan-webrtc (holmer) wrote:
> Maybe add "transport" to the name to make it more clear that this is something
> that only exists after the frame has been sent (or received) over the network
> since it's set by the transport layer.

Done.

[Sergey Ulanov, 2016-07-29 00:10:44]

Description was changed from

==========
Add cricket::VideoFrame::frame_id() and set it to RTP timestamp.

Passing frame_id() to VideoSink will allow to identify incoming video
frames, which will make it possible to correlate video frames on the
sender and on the receiver.

BUG=chromium:621691
==========

to

==========
Add cricket::VideoFrame::transport_frame_id() and set it to RTP timestamp.

Passing transport_frame_id() to VideoSink will allow to identify incoming video
frames, which will make it possible to correlate video frames on the
sender and on the receiver.

BUG=chromium:621691
==========

[stefan-webrtc, 2016-07-29 07:14:49]

lgtm

[Sergey Ulanov, 2016-07-29 16:44:55]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-07-29 16:45:07]

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-07-29 17:03:31]

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

[commit-bot: I haz the power, 2016-07-29 17:03:32]

Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/11484)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/15370)

[Sergey Ulanov, 2016-08-01 05:51:10]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-08-01 05:51:10]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2088953002/#ps80001 (title:
"compile issues on ios, android, mac")

[commit-bot: I haz the power, 2016-08-01 05:51:22]

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-08-01 05:54:27]

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

[commit-bot: I haz the power, 2016-08-01 05:54:28]

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

[Sergey Ulanov, 2016-08-01 07:11:50]

The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 07:11:52]

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

[Sergey Ulanov, 2016-08-01 07:13:00]

sergeyu@chromium.org changed reviewers:
+ tommi@webrtc.org

[Sergey Ulanov, 2016-08-01 07:13:01]

+tommi@ for owners approval

[commit-bot: I haz the power, 2016-08-01 07:14:41]

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

[commit-bot: I haz the power, 2016-08-01 07:14:42]

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

[mflodman, 2016-08-01 13:01:56]

RS LGTM after Stefan's review.

[Sergey Ulanov, 2016-08-01 17:08:54]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-08-01 17:08:54]

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

[commit-bot: I haz the power, 2016-08-01 17:08:57]

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-08-01 17:11:34]

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

[commit-bot: I haz the power, 2016-08-01 17:11:35]

Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)

[Sergey Ulanov, 2016-08-01 18:24:25]

The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 18:24:35]

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-08-01 20:25:03]

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

[commit-bot: I haz the power, 2016-08-01 20:25:04]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Sergey Ulanov, 2016-08-01 20:36:08]

Description was changed from

==========
Add cricket::VideoFrame::transport_frame_id() and set it to RTP timestamp.

Passing transport_frame_id() to VideoSink will allow to identify incoming video
frames, which will make it possible to correlate video frames on the
sender and on the receiver.

BUG=chromium:621691
==========

to

==========
Add cricket::VideoFrame::transport_frame_id() and set it to RTP timestamp.

Passing transport_frame_id() to VideoSink will allow to identify incoming video
frames, which will make it possible to correlate video frames on the
sender and on the receiver.

BUG=chromium:621691
R=mflodman@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/19ee1e6eb102b2282a9993d6c...
==========

[commit-bot: I haz the power, 2016-08-01 20:36:09]

Description was changed from

==========
Add cricket::VideoFrame::transport_frame_id() and set it to RTP timestamp.

Passing transport_frame_id() to VideoSink will allow to identify incoming video
frames, which will make it possible to correlate video frames on the
sender and on the receiver.

BUG=chromium:621691
R=mflodman@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/19ee1e6eb102b2282a9993d6c...
==========

to

==========
Add cricket::VideoFrame::transport_frame_id() and set it to RTP timestamp.

Passing transport_frame_id() to VideoSink will allow to identify incoming video
frames, which will make it possible to correlate video frames on the
sender and on the receiver.

BUG=chromium:621691
R=mflodman@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/19ee1e6eb102b2282a9993d6c373c7185ad9ba34
Cr-Commit-Position: refs/heads/master@{#13596}
==========

[Sergey Ulanov, 2016-08-01 20:36:10]

Committed patchset #7 (id:140001) manually as
19ee1e6eb102b2282a9993d6c373c7185ad9ba34 (presubmit successful).

[commit-bot: I haz the power, 2016-08-01 20:36:10]

Patchset 7 (id:??) landed as
https://crrev.com/19ee1e6eb102b2282a9993d6c373c7185ad9ba34
Cr-Commit-Position: refs/heads/master@{#13596}