Delete VideoFrame::set_render_time_ms.

Delete VideoFrame::set_render_time_ms.

Also mark the render_time_ms getter function and the ntp timestamp
as deprecated.

BUG=webrtc:6977
Review-Url: https://codereview.webrtc.org/2633493002
Cr-Commit-Position: refs/heads/master@{#16354}
Committed: https://chromium.googlesource.com/external/webrtc/+/1c0dea8675436aac4e2177f6e0d7eaf3738cbddb

[nisse-webrtc, 2017-01-12 15:58:15]

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

[commit-bot: I haz the power, 2017-01-12 15:58:29]

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-12 16:25:58]

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

[commit-bot: I haz the power, 2017-01-12 16:25:59]

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

[nisse-webrtc, 2017-01-13 09:18:26]

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

[commit-bot: I haz the power, 2017-01-13 09:18:36]

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-01-13 09:20:15]

Description was changed from

==========
Delete VideoFrame::set_render_time_ms.

BUG=None
==========

to

==========
Delete VideoFrame::set_render_time_ms.

Also mark the render_time_ms getter function and the ntp timestamp
as deprecated.

BUG=webrtc:6977
==========

[nisse-webrtc, 2017-01-13 09:33:24]

nisse@webrtc.org changed reviewers:
+ perkj@webrtc.org, sprang@webrtc.org

[nisse-webrtc, 2017-01-13 09:33:24]

PTAL. I'm trying to make a little progress on the timestamp mess. This step
should be fairly safe. set_render_time_ms is just a wrapper for
set_timestamp_us, using a different unit.

https://codereview.webrtc.org/2633493002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2633493002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:466:
incoming_frame.set_timestamp_us(current_time_us);
I plan to delete this in a followup. This line (both before and after this cl)
means that we discard the timestamp attached earlier in the pipeline, which is
silly. But fixing it may break video sources which doesn't get it right, so
might need some extra care.

[commit-bot: I haz the power, 2017-01-13 10:16:55]

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

[commit-bot: I haz the power, 2017-01-13 10:16:56]

Dry run: This issue passed the CQ dry run.

[perkj_webrtc, 2017-01-13 10:39:28]

I am not sure you can check in this as is since it will cause a regression?

https://codereview.webrtc.org/2633493002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2633493002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:466:
incoming_frame.set_timestamp_us(current_time_us);
On 2017/01/13 09:33:24, nisse-webrtc wrote:
> I plan to delete this in a followup. This line (both before and after this cl)
> means that we discard the timestamp attached earlier in the pipeline, which is
> silly. But fixing it may break video sources which doesn't get it right, so
> might need some extra care.

doesn't that cause a regression with for example the Logitech camera?

[nisse-webrtc, 2017-01-13 10:51:28]

On 2017/01/13 10:39:28, perkj_webrtc wrote:
> I am not sure you can check in this as is since it will cause a regression?

There's no intended change in behaviour. Note that set_render_time_ms is
currently implemented as 

  set_timestamp_us(render_time_ms * rtc::kNumMicrosecsPerMillisec);

so this cl basically inlines this at the call site.

>
https://codereview.webrtc.org/2633493002/diff/20001/webrtc/video/vie_encoder....
> webrtc/video/vie_encoder.cc:466:
> incoming_frame.set_timestamp_us(current_time_us);
> On 2017/01/13 09:33:24, nisse-webrtc wrote:
> > I plan to delete this in a followup. This line (both before and after this
cl)
> > means that we discard the timestamp attached earlier in the pipeline, which
is
> > silly. But fixing it may break video sources which doesn't get it right, so
> > might need some extra care.
> 
> doesn't that cause a regression with for example the Logitech camera?

It might cause a regression, that's why I'd like to do that part in a separate
cl.

Current behaviour is a bit puzzling. It looks like we create a new VideoFrame
where the ntp_time_ms is set based on the capture time (camera clock, timestamp
aligner, and friends), but timestamp_us (and render_time_ms) are based on
reading the clock here, not the input timestamp.

[perkj_webrtc, 2017-01-13 12:43:12]

On 2017/01/13 10:51:28, nisse-webrtc wrote:
> On 2017/01/13 10:39:28, perkj_webrtc wrote:
> > I am not sure you can check in this as is since it will cause a regression?
> 
> There's no intended change in behaviour. Note that set_render_time_ms is
> currently implemented as 
> 
>   set_timestamp_us(render_time_ms * rtc::kNumMicrosecsPerMillisec);
> 
> so this cl basically inlines this at the call site.
> 
> >
>
https://codereview.webrtc.org/2633493002/diff/20001/webrtc/video/vie_encoder....
> > webrtc/video/vie_encoder.cc:466:
> > incoming_frame.set_timestamp_us(current_time_us);
> > On 2017/01/13 09:33:24, nisse-webrtc wrote:
> > > I plan to delete this in a followup. This line (both before and after this
> cl)
> > > means that we discard the timestamp attached earlier in the pipeline,
which
> is
> > > silly. But fixing it may break video sources which doesn't get it right,
so
> > > might need some extra care.
> > 
> > doesn't that cause a regression with for example the Logitech camera?
> 
> It might cause a regression, that's why I'd like to do that part in a separate
> cl.
> 
> Current behaviour is a bit puzzling. It looks like we create a new VideoFrame
> where the ntp_time_ms is set based on the capture time (camera clock,
timestamp
> aligner, and friends), but timestamp_us (and render_time_ms) are based on
> reading the clock here, not the input timestamp.

ok, lgtm then.

[sprang_webrtc, 2017-01-13 12:59:36]

lgtm

[nisse-webrtc, 2017-01-13 13:16:14]

The CQ bit was checked by nisse@webrtc.org

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

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-13 13:20:38]

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

[commit-bot: I haz the power, 2017-01-13 13:20:39]

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

[nisse-webrtc, 2017-01-13 13:26:23]

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

[nisse-webrtc, 2017-01-13 13:26:23]

Stefan, OWNER's approval needed for 

    webrtc/common_video/i420_video_frame_unittest.cc
    webrtc/test/frame_generator_unittest.cc

[stefan-webrtc, 2017-01-25 14:25:53]

lgtm, but update the description, I can't see any methods being deprecated.

[nisse-webrtc, 2017-01-25 15:22:25]

On 2017/01/25 14:25:53, stefan-webrtc wrote:
> lgtm, but update the description, I can't see any methods being deprecated.

Ooops, forgot to upload the patchset with deprecation comments. See the patchset
3 changes.
Ok?

[stefan-webrtc, 2017-01-25 15:38:51]

https://codereview.webrtc.org/2633493002/diff/40001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.h (right):

https://codereview.webrtc.org/2633493002/diff/40001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.h:74: // TODO(nisse): Deprecated. Migrate all users
to timestamp_us().
May want to use RTC_DEPRECATED?

[nisse-webrtc, 2017-01-25 15:52:12]

https://codereview.webrtc.org/2633493002/diff/40001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.h (right):

https://codereview.webrtc.org/2633493002/diff/40001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.h:74: // TODO(nisse): Deprecated. Migrate all users
to timestamp_us().
On 2017/01/25 15:38:51, stefan-webrtc wrote:
> May want to use RTC_DEPRECATED?

Due to the widespread use of the -Werror option (telling the compiler to treat
all warnings as fatal errors), I would expect that using RTC_DEPRECATED will
break lots of downstream projects.

[nisse-webrtc, 2017-01-30 09:52:01]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-01-30 09:52:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
perkj@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2633493002/#ps40001 (title:
"Add deprecation comments.")

[commit-bot: I haz the power, 2017-01-30 09:52:11]

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-30 10:43:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485769921708850,
"parent_rev": "a26330ac6f82c3e5f79dc63421ee93e806c9065e", "commit_rev":
"1c0dea8675436aac4e2177f6e0d7eaf3738cbddb"}

[commit-bot: I haz the power, 2017-01-30 10:43:22]

Description was changed from

==========
Delete VideoFrame::set_render_time_ms.

Also mark the render_time_ms getter function and the ntp timestamp
as deprecated.

BUG=webrtc:6977
==========

to

==========
Delete VideoFrame::set_render_time_ms.

Also mark the render_time_ms getter function and the ntp timestamp
as deprecated.

BUG=webrtc:6977

Review-Url: https://codereview.webrtc.org/2633493002
Cr-Commit-Position: refs/heads/master@{#16354}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1c0dea8675436aac4e2177f6e...
==========

[commit-bot: I haz the power, 2017-01-30 10:43:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/1c0dea8675436aac4e2177f6e...