Implement timing frames.

Implement timing frames.

Timing information is gathered in EncodedImage,
starting at encoders. Then it's sent using RTP header extension. In the
end, it's gathered at the GenericDecoder. Actual reporting and tests
will be in the next CLs.

BUG=webrtc:7594
Review-Url: https://codereview.webrtc.org/2911193002
Cr-Commit-Position: refs/heads/master@{#18659}
Committed: https://chromium.googlesource.com/external/webrtc/+/04f4d126f84dbbb915f52d91807cabdabf08d483

[ilnik, 2017-05-30 10:33:45]

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

[commit-bot: I haz the power, 2017-05-30 10:33:49]

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-05-30 10:36:13]

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

[commit-bot: I haz the power, 2017-05-30 10:36:14]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25982)
  linux_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm64_dbg/builds/...)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/25003)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/26044)

[ilnik, 2017-05-30 11:22:38]

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

[commit-bot: I haz the power, 2017-05-30 11:22:39]

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-05-30 11:25:09]

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

[commit-bot: I haz the power, 2017-05-30 11:25:10]

Dry run: Try jobs failed on following builders:
  linux_memcheck on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/8332)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/25006)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/21853)

[ilnik, 2017-05-30 11:54:33]

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

[commit-bot: I haz the power, 2017-05-30 11:54:38]

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-05-30 12:12:51]

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

[commit-bot: I haz the power, 2017-05-30 12:12:52]

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

[ilnik, 2017-05-30 13:41:38]

ilnik@webrtc.org changed reviewers:
+ asapersson@webrtc.org, sprang@webrtc.org

[sprang_webrtc, 2017-05-31 11:12:55]

First pass.
A general comment about clocks: you seem to use both rtc::TimeMillis() and
webrtc::Clock::TimeInMilliseconds(), and then diff timestamps. While that
technically works, since nowadays they use the same backing clock
implementation, it's probably a good idea to stick to one of them. Not sure, but
think we want to phase out the Clock class, so probably go for the former.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:34: }
I'm not sure this is the right place for this? Can it be placed near the call
site for function instead?

Also, you could probably simplify it to something like
return static_cast<uint16_t>(std::min(time_ms - base_ms,
std::numeric_limits<uint16_t>::max()));

And add a comment as to its purpose.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:592: // Timing frames configuration.
I think you can expand this comment; it's not obvious to me what these values
will do.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:594: size_t minFrameSizeToForceTimingFrameBytes;
Maybe we can make these a named struct, since they seem to be passed around as a
pair. Something like TimingFrameTriggerThresholds or so, then you could simple
name the members delay_ms and min_frame_size_bytes?
Up to you.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:594: size_t minFrameSizeToForceTimingFrameBytes;
Thinking some more on this, not sure bytes is the right type, as a frame being
"large" is a function of the current bitrate and framerate.
Maybe it can be a factor of the ideal frame size instead?

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:765: // a corresponding bool flag.
So we now have three of these bool + value. Time to do that refactoring? :)

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:260: 
So the empty 8bit block at the end is not a part of this block, right? Or is it
just reserved and unused?

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:271:
ByteReader<uint16_t, 2>::ReadBigEndian(data.data());
You don't need the second template parameter here. It's only used if the number
of bytes to read is less than the size of the type being read, typically reading
24bit values into a 32bit int type.

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:499: }
Wouldn't it be possible to wrap |ptr| in an ArrayView and pass that and the
header->extension.videoTiming pointer to the static parse method in extension
header class from this cl? 

Would at least avoid some code duplication...

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:171:
last_timing_frame_time_ms_(0),
Probably a good idea to use -1 as uninitialized value (and explicitly check for
that down below). It's not uncommon for unit test to have a fake clock starting
at time 0, leading to unintuitive behavior.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:413: 
I'm not sure you need to do this on a per codec level.
Couldn't you implement this in VCMEncodedFrameCallback::OnEncodedImage()
instead, and signal the start time to that instance from
VCMGenericEncoder::Encode()?

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:104: size_t
min_frame_size_to_force_timing_frame_bytes_;
These two can be const?

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_decoder.cc:91:
_clock->CurrentNtpInMilliseconds() - _clock->TimeInMilliseconds();
This will be racy, in the sense that the offset may vary a bit from frame to
frame. I have a vague memory if this offset being cached somewhere...

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:46:
kTimingFramesDelayMs = 200,
These should have "default" somewhere in the name.
Also, do we have to use enum? If so, probably define the type. Otherwise make
them plain static const/constexpr

[ilnik, 2017-05-31 15:17:45]

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:34: }
On 2017/05/31 11:12:54, sprang_webrtc wrote:
> I'm not sure this is the right place for this? Can it be placed near the call
> site for function instead?
> 
> Also, you could probably simplify it to something like
> return static_cast<uint16_t>(std::min(time_ms - base_ms,
> std::numeric_limits<uint16_t>::max()));
> 
> And add a comment as to its purpose.

It's called by "modules/rtp_rtcp/source/rtp_packet_to_send.cc" and
"video/payload_router.cc".

What do you suppose would be a better place for it? It's only used to fill this
data structure, so I am not sure, where to put it.

I have added the clarifying comment.

I tried to do it with std::min at first, but it requires additional cast to make
both arguments of min() the same type. Overall, it's longer and harder to read,
I like this version more.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:592: // Timing frames configuration.
On 2017/05/31 11:12:54, sprang_webrtc wrote:
> I think you can expand this comment; it's not obvious to me what these values
> will do.

Done.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:594: size_t minFrameSizeToForceTimingFrameBytes;
On 2017/05/31 11:12:54, sprang_webrtc wrote:
> Thinking some more on this, not sure bytes is the right type, as a frame being
> "large" is a function of the current bitrate and framerate.
> Maybe it can be a factor of the ideal frame size instead?

Nice idea. Done.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:594: size_t minFrameSizeToForceTimingFrameBytes;
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> Maybe we can make these a named struct, since they seem to be passed around as
a
> pair. Something like TimingFrameTriggerThresholds or so, then you could simple
> name the members delay_ms and min_frame_size_bytes?
> Up to you. 

Done.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:765: // a corresponding bool flag.
On 2017/05/31 11:12:54, sprang_webrtc wrote:
> So we now have three of these bool + value. Time to do that refactoring? :)

Maybe, there are some complications about that. Will it be safe to memset()
rtc::optional (RTP header is memset all other the place)/ Either way, it's a job
for a separate CL.

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:260: 
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> So the empty 8bit block at the end is not a part of this block, right? Or is
it
> just reserved and unused?

Not part of the extension.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:271:
ByteReader<uint16_t, 2>::ReadBigEndian(data.data());
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> You don't need the second template parameter here. It's only used if the
number
> of bytes to read is less than the size of the type being read, typically
reading
> 24bit values into a 32bit int type.

Done.

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:499: }
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> Wouldn't it be possible to wrap |ptr| in an ArrayView and pass that and the
> header->extension.videoTiming pointer to the static parse method in extension
> header class from this cl? 
> 
> Would at least avoid some code duplication...

Done.

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:171:
last_timing_frame_time_ms_(0),
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> Probably a good idea to use -1 as uninitialized value (and explicitly check
for
> that down below). It's not uncommon for unit test to have a fake clock
starting
> at time 0, leading to unintuitive behavior. 

Done.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:413: 
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> I'm not sure you need to do this on a per codec level.
> Couldn't you implement this in VCMEncodedFrameCallback::OnEncodedImage()
> instead, and signal the start time to that instance from
> VCMGenericEncoder::Encode()?

Firstly, it will be very troublesome to update VCMEncodedFrameCallback
interface, as it is used in downstream projects.Secondly, I am not sure, how it
will interact with simulcast encoders, if we have several of them in Simulcast
Adapter for example. Is there a separate instance of VCMEncodedFrameCallback for
each simulcast encoder? We also want to have all encoders to mark the same
frames based on time, so decision on marking a frame as timing is still needed
to be done in each encoder in the same way in parallel.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:104: size_t
min_frame_size_to_force_timing_frame_bytes_;
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> These two can be const?

Unfortunately, no. They are initialized in runtime by InitEncode. Passing
initialization values to constructor will complicate code a lot.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_decoder.cc:91:
_clock->CurrentNtpInMilliseconds() - _clock->TimeInMilliseconds();
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> This will be racy, in the sense that the offset may vary a bit from frame to
> frame. I have a vague memory if this offset being cached somewhere...

I think nothing can be done here, as we are using estimated ntp time to adjust
sender timestamps either way, some degree if imprecision is unavoidable.

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:46:
kTimingFramesDelayMs = 200,
On 2017/05/31 11:12:55, sprang_webrtc wrote:
> These should have "default" somewhere in the name.
> Also, do we have to use enum? If so, probably define the type. Otherwise make
> them plain static const/constexpr

Done.

[ilnik, 2017-06-01 08:03:33]

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

[commit-bot: I haz the power, 2017-06-01 08:04: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-06-01 08:23:44]

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

[commit-bot: I haz the power, 2017-06-01 08:23:45]

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

[ilnik, 2017-06-01 12:27:04]

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

[commit-bot: I haz the power, 2017-06-01 12:27:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[ilnik, 2017-06-01 12:30:17]

Description was changed from

==========
Implement timing frames.

Timing information is gathered in EncodedImage,
starting at encoders. Then it's sent using RTP header extension. In the
end, it's gathered at the GenericDecoder. Actual reporting and tests
will be in the next CLs.

BUG=webrtc:7594
==========

to

==========
Implement timing frames.

Timing information is gathered in EncodedImage,
starting at encoders. Then it's sent using RTP header extension. In the
end, it's gathered at the GenericDecoder. Actual reporting and tests
will be in the next CLs.

BUG=webrtc:7594
==========

[ilnik, 2017-06-01 12:30:17]

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

[ilnik, 2017-06-01 12:33:37]

+nisse

Your recent CL caused some problems here (
https://codereview.webrtc.org/2763023002 )

Please review my fixes to vie_encoder.cc and frame_generator_capturer.cc

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:33: RTC_DCHECK_GE(time_ms, base_ms);
Is this a good idea to crash here in debug? Should I add check for negative
delta for production case, where there no DCHECKS?

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:163:
frame->set_timestamp_us(clock_->TimeInMicroseconds());
For consistency.

[commit-bot: I haz the power, 2017-06-01 12:48:34]

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

[commit-bot: I haz the power, 2017-06-01 12:48:35]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-06-02 12:04:56]

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:163:
frame->set_timestamp_us(clock_->TimeInMicroseconds());
On 2017/06/01 12:33:37, ilnik wrote:
> For consistency.

I'd rather move in the opposite direction. The frame timestamp_us should always
be in rtc::TimeMicros timescale (see comments in api/video/video_frame.h).

Not sure what's the best way to get ntp time (on the line below) without using
webrtc::Clock. timeutils.h declares a function TimeUTCMicros, which has a
compile-time constant offset to ntp time.

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:572: if (incoming_frame.timestamp_us() >
current_time_us)
I don't quite understand the problem here, and I'd really prefer to not
reintroduce overwriting of the timestamps in this function. 

If frames are forwarded, i.e., generated by the decoder, which selects some
future "render time", and then reencoded, can't the responsibility for clipping
the timestamps should be moved somewhere before this function? Maybe you can use
the TimestampAligner class (same logic which is used to process camera
timestamps).

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:580: capture_ntp_time_ms =
video_frame.render_time_ms() + delta_ntp_internal_ms_;
BTW, this delta_ntp_internal_ms may be the caching Erik mentioned in another
comment.

I don't quite like that either. On linux, the offset between rtc::TimeMicros and
utc time is constant over normal frequency adjustments by the ntp daemon (but
will obviously change if the system time is explicitly reset). But in general,
rtc::TimeMicros might have a frequency drift compared to the system wall clock.

[nisse-webrtc, 2017-06-02 12:10:48]

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:34: }
On 2017/05/31 15:17:44, ilnik wrote:
> On 2017/05/31 11:12:54, sprang_webrtc wrote:
> > I'm not sure this is the right place for this? Can it be placed near the
call
> > site for function instead?
> > 
> > Also, you could probably simplify it to something like
> > return static_cast<uint16_t>(std::min(time_ms - base_ms,
> > std::numeric_limits<uint16_t>::max()));
> > 
> > And add a comment as to its purpose.
> 
> It's called by "modules/rtp_rtcp/source/rtp_packet_to_send.cc" and
> "video/payload_router.cc".
> 
> What do you suppose would be a better place for it? It's only used to fill
this
> data structure, so I am not sure, where to put it.
> 
> I have added the clarifying comment.
> 
> I tried to do it with std::min at first, but it requires additional cast to
make
> both arguments of min() the same type. Overall, it's longer and harder to
read,
> I like this version more.

There's also rtc::SafeMin and rtc::SafeMax (added by kwiberg a few weeks ago).

[kwiberg-webrtc, 2017-06-02 12:37:12]

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:34: }
On 2017/06/02 12:10:47, nisse-webrtc wrote:
> On 2017/05/31 15:17:44, ilnik wrote:
> > On 2017/05/31 11:12:54, sprang_webrtc wrote:
> > > I'm not sure this is the right place for this? Can it be placed near the
> call
> > > site for function instead?
> > > 
> > > Also, you could probably simplify it to something like
> > > return static_cast<uint16_t>(std::min(time_ms - base_ms,
> > > std::numeric_limits<uint16_t>::max()));
> > > 
> > > And add a comment as to its purpose.
> > 
> > It's called by "modules/rtp_rtcp/source/rtp_packet_to_send.cc" and
> > "video/payload_router.cc".
> > 
> > What do you suppose would be a better place for it? It's only used to fill
> this
> > data structure, so I am not sure, where to put it.
> > 
> > I have added the clarifying comment.
> > 
> > I tried to do it with std::min at first, but it requires additional cast to
> make
> > both arguments of min() the same type. Overall, it's longer and harder to
> read,
> > I like this version more.
> 
> There's also rtc::SafeMin and rtc::SafeMax (added by kwiberg a few weeks ago).

Yes---those will accept any two integer types, and either automatically choose a
suitable return type or statically verify that the return type you ask for is
suitable. See the docs in base/safe_minmax.h.

In this case, you should be able to replace lines 30-33 with

  return rtc::SafeMin<uint16_t>(time_ms - base_ms,
                                std::numeric_limits<uint16_t>::max());

The compiler will complain if it can't prove that the result will always fit in
an uint16_t.

(I'll announce these soon, but I'd like to land their buddy SafeClamp first, so
that I don't have to write more than one PSA.)

For the specific case of clamping to the limits of a built-in integer type,
however, there's also rtc::saturated_cast:

  return rtc::saturated_cast<uint16_t>(time_ms - base_ms);

That's probably what you should use here.

[ilnik, 2017-06-02 13:02:22]

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:163:
frame->set_timestamp_us(clock_->TimeInMicroseconds());
On 2017/06/02 12:04:55, nisse-webrtc wrote:
> On 2017/06/01 12:33:37, ilnik wrote:
> > For consistency.
> 
> I'd rather move in the opposite direction. The frame timestamp_us should
always
> be in rtc::TimeMicros timescale (see comments in api/video/video_frame.h).
> 
> Not sure what's the best way to get ntp time (on the line below) without using
> webrtc::Clock. timeutils.h declares a function TimeUTCMicros, which has a
> compile-time constant offset to ntp time.

I believe we should use clock_ instead of wallclock time if available. What if
we are inside some kind of test? Also, in case of realtime clock_, at least on
linux, it boils down to rtc:TimeMicros().

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:572: if (incoming_frame.timestamp_us() >
current_time_us)
On 2017/06/02 12:04:55, nisse-webrtc wrote:
> I don't quite understand the problem here, and I'd really prefer to not
> reintroduce overwriting of the timestamps in this function. 
> 
> If frames are forwarded, i.e., generated by the decoder, which selects some
> future "render time", and then reencoded, can't the responsibility for
clipping
> the timestamps should be moved somewhere before this function? Maybe you can
use
> the TimestampAligner class (same logic which is used to process camera
> timestamps).
> 

Problem is that timestamps are sent over network as deltas from capture_time
(because we can't and don't want to send 40 bytes of timestamps). I am using
uint16_t for that (which gives us ability to send correctly timestamps upto 65
seconds in the future). So capture time after encode start time produces
negative delta, which we can't even send.

Removal of rewriting timestamps simply breaks everything.

Also, introducing additional layer between producer and consumer of frames just
to rewrite timestamps seems too much (especially because it will have to be done
in client code). Also, it will produce very hard to investigate errors if
someone forgets about that additional layer.

I agree, it may make sense to move all the timestamps processing code here
(L566-606) somewhere else, but it's completely separate refactoring to that I am
trying to do.

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:580: capture_ntp_time_ms =
video_frame.render_time_ms() + delta_ntp_internal_ms_;
On 2017/06/02 12:04:56, nisse-webrtc wrote:
> BTW, this delta_ntp_internal_ms may be the caching Erik mentioned in another
> comment.
> 
> I don't quite like that either. On linux, the offset between rtc::TimeMicros
and
> utc time is constant over normal frequency adjustments by the ntp daemon (but
> will obviously change if the system time is explicitly reset). But in general,
> rtc::TimeMicros might have a frequency drift compared to the system wall
clock.

Acknowledged.

[ilnik, 2017-06-02 13:54:42]

On 2017/06/02 12:37:12, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
> File webrtc/api/video/video_timing.h (right):
> 
>
https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
> webrtc/api/video/video_timing.h:34: }
> On 2017/06/02 12:10:47, nisse-webrtc wrote:
> > On 2017/05/31 15:17:44, ilnik wrote:
> > > On 2017/05/31 11:12:54, sprang_webrtc wrote:
> > > > I'm not sure this is the right place for this? Can it be placed near the
> > call
> > > > site for function instead?
> > > > 
> > > > Also, you could probably simplify it to something like
> > > > return static_cast<uint16_t>(std::min(time_ms - base_ms,
> > > > std::numeric_limits<uint16_t>::max()));
> > > > 
> > > > And add a comment as to its purpose.
> > > 
> > > It's called by "modules/rtp_rtcp/source/rtp_packet_to_send.cc" and
> > > "video/payload_router.cc".
> > > 
> > > What do you suppose would be a better place for it? It's only used to fill
> > this
> > > data structure, so I am not sure, where to put it.
> > > 
> > > I have added the clarifying comment.
> > > 
> > > I tried to do it with std::min at first, but it requires additional cast
to
> > make
> > > both arguments of min() the same type. Overall, it's longer and harder to
> > read,
> > > I like this version more.
> > 
> > There's also rtc::SafeMin and rtc::SafeMax (added by kwiberg a few weeks
ago).
> 
> Yes---those will accept any two integer types, and either automatically choose
a
> suitable return type or statically verify that the return type you ask for is
> suitable. See the docs in base/safe_minmax.h.
> 
> In this case, you should be able to replace lines 30-33 with
> 
>   return rtc::SafeMin<uint16_t>(time_ms - base_ms,
>                                 std::numeric_limits<uint16_t>::max());
> 
> The compiler will complain if it can't prove that the result will always fit
in
> an uint16_t.
> 
> (I'll announce these soon, but I'd like to land their buddy SafeClamp first,
so
> that I don't have to write more than one PSA.)
> 
> For the specific case of clamping to the limits of a built-in integer type,
> however, there's also rtc::saturated_cast:
> 
>   return rtc::saturated_cast<uint16_t>(time_ms - base_ms);
> 
> That's probably what you should use here.

Thanks for the pointers. saturated_cast is exactly that I was searching for.

[ilnik, 2017-06-02 14:00:46]

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:34: }
On 2017/06/02 12:37:12, kwiberg-webrtc wrote:
> On 2017/06/02 12:10:47, nisse-webrtc wrote:
> > On 2017/05/31 15:17:44, ilnik wrote:
> > > On 2017/05/31 11:12:54, sprang_webrtc wrote:
> > > > I'm not sure this is the right place for this? Can it be placed near the
> > call
> > > > site for function instead?
> > > > 
> > > > Also, you could probably simplify it to something like
> > > > return static_cast<uint16_t>(std::min(time_ms - base_ms,
> > > > std::numeric_limits<uint16_t>::max()));
> > > > 
> > > > And add a comment as to its purpose.
> > > 
> > > It's called by "modules/rtp_rtcp/source/rtp_packet_to_send.cc" and
> > > "video/payload_router.cc".
> > > 
> > > What do you suppose would be a better place for it? It's only used to fill
> > this
> > > data structure, so I am not sure, where to put it.
> > > 
> > > I have added the clarifying comment.
> > > 
> > > I tried to do it with std::min at first, but it requires additional cast
to
> > make
> > > both arguments of min() the same type. Overall, it's longer and harder to
> > read,
> > > I like this version more.
> > 
> > There's also rtc::SafeMin and rtc::SafeMax (added by kwiberg a few weeks
ago).
> 
> Yes---those will accept any two integer types, and either automatically choose
a
> suitable return type or statically verify that the return type you ask for is
> suitable. See the docs in base/safe_minmax.h.
> 
> In this case, you should be able to replace lines 30-33 with
> 
>   return rtc::SafeMin<uint16_t>(time_ms - base_ms,
>                                 std::numeric_limits<uint16_t>::max());
> 
> The compiler will complain if it can't prove that the result will always fit
in
> an uint16_t.
> 
> (I'll announce these soon, but I'd like to land their buddy SafeClamp first,
so
> that I don't have to write more than one PSA.)
> 
> For the specific case of clamping to the limits of a built-in integer type,
> however, there's also rtc::saturated_cast:
> 
>   return rtc::saturated_cast<uint16_t>(time_ms - base_ms);
> 
> That's probably what you should use here.

Done.

[sprang_webrtc, 2017-06-05 14:39:20]

I still think we should try to move the logic out of the individual encoder
implementations, if at all possible. Both to reduce code complexity/duplication,
but also to be able to capture HW encoders and any other external codecs.

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:413: 
On 2017/05/31 15:17:45, ilnik wrote:
> On 2017/05/31 11:12:55, sprang_webrtc wrote:
> > I'm not sure you need to do this on a per codec level.
> > Couldn't you implement this in VCMEncodedFrameCallback::OnEncodedImage()
> > instead, and signal the start time to that instance from
> > VCMGenericEncoder::Encode()?
> 
> Firstly, it will be very troublesome to update VCMEncodedFrameCallback
> interface, as it is used in downstream projects.

You can always start by having a simple method declared inline. Just something
to set a state that any frame with capture time X should be marked as timing
frame.

> Secondly, I am not sure, how it
> will interact with simulcast encoders, if we have several of them in Simulcast
> Adapter for example. Is there a separate instance of VCMEncodedFrameCallback
for
> each simulcast encoder?

No, these is just a single callback be called several times. This is true
regardless of usage of simulcast encoder adapter or libvpx with built-in
simulcast support.

> We also want to have all encoders to mark the same
> frames based on time, so decision on marking a frame as timing is still needed
> to be done in each encoder in the same way in parallel.

Not sure I follow what you mean here. If you mean outlier detection, isn't it
enough to do that on a per stream basis?

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_decoder.cc:91:
_clock->CurrentNtpInMilliseconds() - _clock->TimeInMilliseconds();
On 2017/05/31 15:17:45, ilnik wrote:
> On 2017/05/31 11:12:55, sprang_webrtc wrote:
> > This will be racy, in the sense that the offset may vary a bit from frame to
> > frame. I have a vague memory if this offset being cached somewhere...
> 
> I think nothing can be done here, as we are using estimated ntp time to adjust
> sender timestamps either way, some degree if imprecision is unavoidable.

Some imprecision is unavoidable, agreed, but we can keep that imprecision
constant. Just cache this offset as a const member of the class for instance.
Otherwise we're introducing extra jitter (and calling timing functions
unnecessarily often).

[ilnik, 2017-06-05 14:44:13]

On 2017/06/05 14:39:20, sprang_webrtc wrote:
> I still think we should try to move the logic out of the individual encoder
> implementations, if at all possible. Both to reduce code
complexity/duplication,
> but also to be able to capture HW encoders and any other external codecs.

The problem here is that we need to invent some way to pass average frame size
and encode start/finish timestamps. I will try to do it as a refactoring next.

[sprang_webrtc, 2017-06-05 14:52:25]

On 2017/06/05 14:44:13, ilnik wrote:
> On 2017/06/05 14:39:20, sprang_webrtc wrote:
> > I still think we should try to move the logic out of the individual encoder
> > implementations, if at all possible. Both to reduce code
> complexity/duplication,
> > but also to be able to capture HW encoders and any other external codecs.
> 
> The problem here is that we need to invent some way to pass average frame size
> and encode start/finish timestamps. I will try to do it as a refactoring next.

Thanks!
Note that VCMGenericEncoder owns a |vcm_encoded_frame_callback_| ptr, so it can
trivially signal time of encode start.
The EncodedImage and CodecSpecificInfo parameters of
VCMEncodedFrameCallback::OnEncodedImage can be used to find resolution and size
plus simulcast id for each frame.
Looks to me like it should be doable...

[ilnik, 2017-06-07 14:25:03]

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

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:413: 
On 2017/06/05 14:39:20, sprang_webrtc wrote:
> On 2017/05/31 15:17:45, ilnik wrote:
> > On 2017/05/31 11:12:55, sprang_webrtc wrote:
> > > I'm not sure you need to do this on a per codec level.
> > > Couldn't you implement this in VCMEncodedFrameCallback::OnEncodedImage()
> > > instead, and signal the start time to that instance from
> > > VCMGenericEncoder::Encode()?
> > 
> > Firstly, it will be very troublesome to update VCMEncodedFrameCallback
> > interface, as it is used in downstream projects.
> 
> You can always start by having a simple method declared inline. Just something
> to set a state that any frame with capture time X should be marked as timing
> frame.
> 
> > Secondly, I am not sure, how it
> > will interact with simulcast encoders, if we have several of them in
Simulcast
> > Adapter for example. Is there a separate instance of VCMEncodedFrameCallback
> for
> > each simulcast encoder?
> 
> No, these is just a single callback be called several times. This is true
> regardless of usage of simulcast encoder adapter or libvpx with built-in
> simulcast support.
> 
> > We also want to have all encoders to mark the same
> > frames based on time, so decision on marking a frame as timing is still
needed
> > to be done in each encoder in the same way in parallel.
> 
> Not sure I follow what you mean here. If you mean outlier detection, isn't it
> enough to do that on a per stream basis?

Done.

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_decoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_decoder.cc:91:
_clock->CurrentNtpInMilliseconds() - _clock->TimeInMilliseconds();
On 2017/06/05 14:39:20, sprang_webrtc wrote:
> On 2017/05/31 15:17:45, ilnik wrote:
> > On 2017/05/31 11:12:55, sprang_webrtc wrote:
> > > This will be racy, in the sense that the offset may vary a bit from frame
to
> > > frame. I have a vague memory if this offset being cached somewhere...
> > 
> > I think nothing can be done here, as we are using estimated ntp time to
adjust
> > sender timestamps either way, some degree if imprecision is unavoidable.
> 
> Some imprecision is unavoidable, agreed, but we can keep that imprecision
> constant. Just cache this offset as a const member of the class for instance.
> Otherwise we're introducing extra jitter (and calling timing functions
> unnecessarily often).

Oh, I've got that you mean. I didn't find any cached values like that, so It's
now cached in VCMDecodedFrameCallback.

[ilnik, 2017-06-07 14:26:27]

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

[commit-bot: I haz the power, 2017-06-07 14:27: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, 2017-06-07 14:28:45]

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

[commit-bot: I haz the power, 2017-06-07 14:28:47]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26231)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/25181)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17871)

[ilnik, 2017-06-07 14:34:26]

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

[commit-bot: I haz the power, 2017-06-07 14:34: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, 2017-06-07 14:47:40]

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

[commit-bot: I haz the power, 2017-06-07 14:47:41]

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

[ilnik, 2017-06-07 14:57:59]

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

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

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-06-07 15:11:23]

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

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

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

[ilnik, 2017-06-07 15:45:48]

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

[commit-bot: I haz the power, 2017-06-07 15:46:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[ilnik, 2017-06-07 15:47:51]

The CQ bit was unchecked by ilnik@webrtc.org

[ilnik, 2017-06-07 16:06:44]

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

[commit-bot: I haz the power, 2017-06-07 16:06:49]

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-06-07 16:15:49]

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

[commit-bot: I haz the power, 2017-06-07 16:15:51]

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

[ilnik, 2017-06-07 16:36:33]

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

[commit-bot: I haz the power, 2017-06-07 16:36:43]

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-06-07 18:09:17]

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

[commit-bot: I haz the power, 2017-06-07 18:09:18]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-06-08 06:58:36]

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:572: if (incoming_frame.timestamp_us() >
current_time_us)
On 2017/06/02 13:02:21, ilnik wrote:
> On 2017/06/02 12:04:55, nisse-webrtc wrote:
> > I don't quite understand the problem here, and I'd really prefer to not
> > reintroduce overwriting of the timestamps in this function. 
> > 
> > If frames are forwarded, i.e., generated by the decoder, which selects some
> > future "render time", and then reencoded, can't the responsibility for
> clipping
> > the timestamps should be moved somewhere before this function? Maybe you can
> use
> > the TimestampAligner class (same logic which is used to process camera
> > timestamps).
> > 
> 
> Problem is that timestamps are sent over network as deltas from capture_time
> (because we can't and don't want to send 40 bytes of timestamps). I am using
> uint16_t for that (which gives us ability to send correctly timestamps upto 65
> seconds in the future). So capture time after encode start time produces
> negative delta, which we can't even send.
> 
> Removal of rewriting timestamps simply breaks everything.
> 
> Also, introducing additional layer between producer and consumer of frames
just
> to rewrite timestamps seems too much (especially because it will have to be
done
> in client code). Also, it will produce very hard to investigate errors if
> someone forgets about that additional layer.
> 
> I agree, it may make sense to move all the timestamps processing code here
> (L566-606) somewhere else, but it's completely separate refactoring to that I
am
> trying to do.

I think I'm missing some context here, can you help me? This conditional checks
if the frame timestamp is in the future. When will that happen? Which code
produces webrtc::VideoFrame objects with future timestamps?

I'm mainly familiar with the capture pipeline, and there the TimestampAligner is
used which, among other things, ensures timestamps aren't in the future. And I
agree that's a reasonable requirement.

[ilnik, 2017-06-08 07:26:00]

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:572: if (incoming_frame.timestamp_us() >
current_time_us)
On 2017/06/08 06:58:36, nisse-webrtc wrote:
> On 2017/06/02 13:02:21, ilnik wrote:
> > On 2017/06/02 12:04:55, nisse-webrtc wrote:
> > > I don't quite understand the problem here, and I'd really prefer to not
> > > reintroduce overwriting of the timestamps in this function. 
> > > 
> > > If frames are forwarded, i.e., generated by the decoder, which selects
some
> > > future "render time", and then reencoded, can't the responsibility for
> > clipping
> > > the timestamps should be moved somewhere before this function? Maybe you
can
> > use
> > > the TimestampAligner class (same logic which is used to process camera
> > > timestamps).
> > > 
> > 
> > Problem is that timestamps are sent over network as deltas from capture_time
> > (because we can't and don't want to send 40 bytes of timestamps). I am using
> > uint16_t for that (which gives us ability to send correctly timestamps upto
65
> > seconds in the future). So capture time after encode start time produces
> > negative delta, which we can't even send.
> > 
> > Removal of rewriting timestamps simply breaks everything.
> > 
> > Also, introducing additional layer between producer and consumer of frames
> just
> > to rewrite timestamps seems too much (especially because it will have to be
> done
> > in client code). Also, it will produce very hard to investigate errors if
> > someone forgets about that additional layer.
> > 
> > I agree, it may make sense to move all the timestamps processing code here
> > (L566-606) somewhere else, but it's completely separate refactoring to that
I
> am
> > trying to do.
> 
> I think I'm missing some context here, can you help me? This conditional
checks
> if the frame timestamp is in the future. When will that happen? Which code
> produces webrtc::VideoFrame objects with future timestamps?
> 
> I'm mainly familiar with the capture pipeline, and there the TimestampAligner
is
> used which, among other things, ensures timestamps aren't in the future. And I
> agree that's a reasonable requirement.

It happens in PeerConnectionIntegrationTest.CanSendRemoteVideoTrack test. I
don't quite understand all the details, but output of one decoder is feed into
another encoder inside VideoBroadcaster. In exactly that situation timestamp in
the frame (interpreted as capture time in encoders and render time in decoders)
will be set by decoder to projected render time in the future.

[ilnik, 2017-06-08 07:29:19]

Erik, it's ready for another round of review. As you requested, all the logic is
out of encoders and in the generic encoder now.

[nisse-webrtc, 2017-06-08 07:38:36]

I've only looked at the timestamp issues in vie_encoder.cc. Do you want me to
review any other parts of this cl?

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:572: if (incoming_frame.timestamp_us() >
current_time_us)
On 2017/06/08 07:26:00, ilnik wrote:
> On 2017/06/08 06:58:36, nisse-webrtc wrote:
> > On 2017/06/02 13:02:21, ilnik wrote:
> > > On 2017/06/02 12:04:55, nisse-webrtc wrote:
> > > > I don't quite understand the problem here, and I'd really prefer to not
> > > > reintroduce overwriting of the timestamps in this function. 
> > > > 
> > > > If frames are forwarded, i.e., generated by the decoder, which selects
> some
> > > > future "render time", and then reencoded, can't the responsibility for
> > > clipping
> > > > the timestamps should be moved somewhere before this function? Maybe you
> can
> > > use
> > > > the TimestampAligner class (same logic which is used to process camera
> > > > timestamps).
> > > > 
> > > 
> > > Problem is that timestamps are sent over network as deltas from
capture_time
> > > (because we can't and don't want to send 40 bytes of timestamps). I am
using
> > > uint16_t for that (which gives us ability to send correctly timestamps
upto
> 65
> > > seconds in the future). So capture time after encode start time produces
> > > negative delta, which we can't even send.
> > > 
> > > Removal of rewriting timestamps simply breaks everything.
> > > 
> > > Also, introducing additional layer between producer and consumer of frames
> > just
> > > to rewrite timestamps seems too much (especially because it will have to
be
> > done
> > > in client code). Also, it will produce very hard to investigate errors if
> > > someone forgets about that additional layer.
> > > 
> > > I agree, it may make sense to move all the timestamps processing code here
> > > (L566-606) somewhere else, but it's completely separate refactoring to
that
> I
> > am
> > > trying to do.
> > 
> > I think I'm missing some context here, can you help me? This conditional
> checks
> > if the frame timestamp is in the future. When will that happen? Which code
> > produces webrtc::VideoFrame objects with future timestamps?
> > 
> > I'm mainly familiar with the capture pipeline, and there the
TimestampAligner
> is
> > used which, among other things, ensures timestamps aren't in the future. And
I
> > agree that's a reasonable requirement.
> 
> It happens in PeerConnectionIntegrationTest.CanSendRemoteVideoTrack test. I
> don't quite understand all the details, but output of one decoder is feed into
> another encoder inside VideoBroadcaster. In exactly that situation timestamp
in
> the frame (interpreted as capture time in encoders and render time in
decoders)
> will be set by decoder to projected render time in the future.

Ok, I see. Could you clarify the comment a bit? I think the reference to
"statistics" is confusing, if we have a general requirement downstream on the
encode and rtp send path that the timestamp must not be in the future.

I think the proper way of doing this would be to have the decode pipeline attach
its best guesstimate for original capture time to the frame (which ought to
always be in the past), and apply an appropriate playout delay closer to
rendering. Do you think that's doable? I understand it's going to be a much
larger refactoring.

[ilnik, 2017-06-08 08:01:43]

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:572: if (incoming_frame.timestamp_us() >
current_time_us)
On 2017/06/08 07:38:36, nisse-webrtc wrote:
> On 2017/06/08 07:26:00, ilnik wrote:
> > On 2017/06/08 06:58:36, nisse-webrtc wrote:
> > > On 2017/06/02 13:02:21, ilnik wrote:
> > > > On 2017/06/02 12:04:55, nisse-webrtc wrote:
> > > > > I don't quite understand the problem here, and I'd really prefer to
not
> > > > > reintroduce overwriting of the timestamps in this function. 
> > > > > 
> > > > > If frames are forwarded, i.e., generated by the decoder, which selects
> > some
> > > > > future "render time", and then reencoded, can't the responsibility for
> > > > clipping
> > > > > the timestamps should be moved somewhere before this function? Maybe
you
> > can
> > > > use
> > > > > the TimestampAligner class (same logic which is used to process camera
> > > > > timestamps).
> > > > > 
> > > > 
> > > > Problem is that timestamps are sent over network as deltas from
> capture_time
> > > > (because we can't and don't want to send 40 bytes of timestamps). I am
> using
> > > > uint16_t for that (which gives us ability to send correctly timestamps
> upto
> > 65
> > > > seconds in the future). So capture time after encode start time produces
> > > > negative delta, which we can't even send.
> > > > 
> > > > Removal of rewriting timestamps simply breaks everything.
> > > > 
> > > > Also, introducing additional layer between producer and consumer of
frames
> > > just
> > > > to rewrite timestamps seems too much (especially because it will have to
> be
> > > done
> > > > in client code). Also, it will produce very hard to investigate errors
if
> > > > someone forgets about that additional layer.
> > > > 
> > > > I agree, it may make sense to move all the timestamps processing code
here
> > > > (L566-606) somewhere else, but it's completely separate refactoring to
> that
> > I
> > > am
> > > > trying to do.
> > > 
> > > I think I'm missing some context here, can you help me? This conditional
> > checks
> > > if the frame timestamp is in the future. When will that happen? Which code
> > > produces webrtc::VideoFrame objects with future timestamps?
> > > 
> > > I'm mainly familiar with the capture pipeline, and there the
> TimestampAligner
> > is
> > > used which, among other things, ensures timestamps aren't in the future.
And
> I
> > > agree that's a reasonable requirement.
> > 
> > It happens in PeerConnectionIntegrationTest.CanSendRemoteVideoTrack test. I
> > don't quite understand all the details, but output of one decoder is feed
into
> > another encoder inside VideoBroadcaster. In exactly that situation timestamp
> in
> > the frame (interpreted as capture time in encoders and render time in
> decoders)
> > will be set by decoder to projected render time in the future.
> 
> Ok, I see. Could you clarify the comment a bit? I think the reference to
> "statistics" is confusing, if we have a general requirement downstream on the
> encode and rtp send path that the timestamp must not be in the future.
> 
> I think the proper way of doing this would be to have the decode pipeline
attach
> its best guesstimate for original capture time to the frame (which ought to
> always be in the past), and apply an appropriate playout delay closer to
> rendering. Do you think that's doable? I understand it's going to be a much
> larger refactoring.

I clarified the comment. 

About the broad refactoring, I don't see much benefits here. The only
problematic case is when encoder is fed on decoder output. But amount of
refactoring is quite huge. Also, it's still possible in that case to wire things
mistakenly and still fed frames with incorrect timestamps to the decoder. So,
safety check here is still required.

[ilnik, 2017-06-08 08:10:25]

On 2017/06/08 07:38:36, nisse-webrtc wrote:
> I've only looked at the timestamp issues in vie_encoder.cc. Do you want me to
> review any other parts of this cl?

Only if you want to. I added you because I've partly reverted your recent
commit. Seems like you are not the sole owner of some parts of this CL.
Ofcourse, I would be thankful if you could find the time to review it.

[ilnik, 2017-06-09 09:36:14]

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

[ilnik, 2017-06-09 09:36:57]

+stefan,

Please review modules/ changes.

[sprang_webrtc, 2017-06-09 11:02:01]

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/test/frame_genera...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2911193002/diff/180001/webrtc/test/frame_genera...
webrtc/test/frame_generator_capturer.cc:163:
frame->set_timestamp_us(clock_->TimeInMicroseconds());
On 2017/06/02 12:04:55, nisse-webrtc wrote:
> On 2017/06/01 12:33:37, ilnik wrote:
> > For consistency.
> 
> I'd rather move in the opposite direction. The frame timestamp_us should
always
> be in rtc::TimeMicros timescale (see comments in api/video/video_frame.h).
> 
> Not sure what's the best way to get ntp time (on the line below) without using
> webrtc::Clock. timeutils.h declares a function TimeUTCMicros, which has a
> compile-time constant offset to ntp time.

Agree that we should move towards rtc::TimeMicros rather than Clock, but then we
should do so for the whole file at once, just so we're not using a fake clock
for some of the calls but not others... This CL is already very large, I think
that refactoring can "wait" for a follow-up.

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:233:
encoded_image.capture_time_ms_);
nit: maybe you could store
timing_frames_info_[simulcast_svc_idx].encode_start_time_ms as temporary
pointer, so you don't have to write that full string quite so often

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:258: framerate_;
Could you clarify this by adding explicit parenthesis or breaking it up into two
steps? Also probably want an explicit static_cast for that float, if float is
even necessary?

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:80:
VideoCodec::TimingFrameTriggerThresholds timing_frames_thresholds_;
Looks like there are potential raciness here, as rate setters etc are called by
the vie encoder queue (right?) but OnEncodedImage() is potentially called
asynchronously by an encoder-internal thread.
Add locks and annotate as appropriate.

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:81: };
This logic definitely warrants units tests as well. Pretty bad that don't
currently have a generic_encoder_unittests.cc, could you add one? No need to add
tests for the existing logic in this cl though.

[ilnik, 2017-06-09 12:06:06]

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:233:
encoded_image.capture_time_ms_);
On 2017/06/09 11:02:01, sprang_webrtc wrote:
> nit: maybe you could store
> timing_frames_info_[simulcast_svc_idx].encode_start_time_ms as temporary
> pointer, so you don't have to write that full string quite so often

Done.

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:258: framerate_;
On 2017/06/09 11:02:01, sprang_webrtc wrote:
> Could you clarify this by adding explicit parenthesis or breaking it up into
two
> steps? Also probably want an explicit static_cast for that float, if float is
> even necessary?

Split in two steps now. No float is needed now.

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:80:
VideoCodec::TimingFrameTriggerThresholds timing_frames_thresholds_;
On 2017/06/09 11:02:01, sprang_webrtc wrote:
> Looks like there are potential raciness here, as rate setters etc are called
by
> the vie encoder queue (right?) but OnEncodedImage() is potentially called
> asynchronously by an encoder-internal thread.
> Add locks and annotate as appropriate.

I think, there's no raciness here, as most methods of vcm_encoded_image are
called from methods of GenericEncoder directly, like OnFrameRateChanged,
OnEncodeStarted and OnTargetBitrateChanged. Only OnEncodedImage() is called from
encoder_->Encode(), which is called by GenericEncoder::Encode().
GenericEncoder::Encode(), SetEncoderParameters(), and RequestFrame() are all
protected by RTC_DCHECK_RUNS_SERIALIZED(&race_checker_) inside GenericEncoder.

I agree, that it must be documented, so I added a comment.

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:81: };
On 2017/06/09 11:02:01, sprang_webrtc wrote:
> This logic definitely warrants units tests as well. Pretty bad that don't
> currently have a generic_encoder_unittests.cc, could you add one? No need to
add
> tests for the existing logic in this cl though.

I am writing a second CL with tests in parallel. Generic encoder utittest will
be added there, Is that OK?

[sprang_webrtc, 2017-06-12 07:10:10]

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:80:
VideoCodec::TimingFrameTriggerThresholds timing_frames_thresholds_;
On 2017/06/09 12:06:05, ilnik wrote:
> On 2017/06/09 11:02:01, sprang_webrtc wrote:
> > Looks like there are potential raciness here, as rate setters etc are called
> by
> > the vie encoder queue (right?) but OnEncodedImage() is potentially called
> > asynchronously by an encoder-internal thread.
> > Add locks and annotate as appropriate.
> 
> I think, there's no raciness here, as most methods of vcm_encoded_image are
> called from methods of GenericEncoder directly, like OnFrameRateChanged,
> OnEncodeStarted and OnTargetBitrateChanged. Only OnEncodedImage() is called
from
> encoder_->Encode(), which is called by GenericEncoder::Encode().
> GenericEncoder::Encode(), SetEncoderParameters(), and RequestFrame() are all
> protected by RTC_DCHECK_RUNS_SERIALIZED(&race_checker_) inside GenericEncoder.
> 
> I agree, that it must be documented, so I added a comment.

OnEncodedImage() is called by the encoder, but there is no guarantee that it's
called by the same thread as the one that called Encode(). For the default
software encoders it is currently the case, but for external encoders it need
not be.

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:81: };
On 2017/06/09 12:06:06, ilnik wrote:
> On 2017/06/09 11:02:01, sprang_webrtc wrote:
> > This logic definitely warrants units tests as well. Pretty bad that don't
> > currently have a generic_encoder_unittests.cc, could you add one? No need to
> add
> > tests for the existing logic in this cl though.
> 
> I am writing a second CL with tests in parallel. Generic encoder utittest will
> be added there, Is that OK?

I'd actually prefer they are in the same one. If it becomes too large it's
better to split the cl by functionality instead. Adding unit tests later usually
just means finding out the problems later.

[ilnik, 2017-06-12 09:06:25]

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:80:
VideoCodec::TimingFrameTriggerThresholds timing_frames_thresholds_;
On 2017/06/12 07:10:10, sprang_webrtc wrote:
> On 2017/06/09 12:06:05, ilnik wrote:
> > On 2017/06/09 11:02:01, sprang_webrtc wrote:
> > > Looks like there are potential raciness here, as rate setters etc are
called
> > by
> > > the vie encoder queue (right?) but OnEncodedImage() is potentially called
> > > asynchronously by an encoder-internal thread.
> > > Add locks and annotate as appropriate.
> > 
> > I think, there's no raciness here, as most methods of vcm_encoded_image are
> > called from methods of GenericEncoder directly, like OnFrameRateChanged,
> > OnEncodeStarted and OnTargetBitrateChanged. Only OnEncodedImage() is called
> from
> > encoder_->Encode(), which is called by GenericEncoder::Encode().
> > GenericEncoder::Encode(), SetEncoderParameters(), and RequestFrame() are all
> > protected by RTC_DCHECK_RUNS_SERIALIZED(&race_checker_) inside
GenericEncoder.
> > 
> > I agree, that it must be documented, so I added a comment.
> 
> OnEncodedImage() is called by the encoder, but there is no guarantee that it's
> called by the same thread as the one that called Encode(). For the default
> software encoders it is currently the case, but for external encoders it need
> not be.

Ok, Added protection using critical section.

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:81: };
On 2017/06/12 07:10:10, sprang_webrtc wrote:
> On 2017/06/09 12:06:06, ilnik wrote:
> > On 2017/06/09 11:02:01, sprang_webrtc wrote:
> > > This logic definitely warrants units tests as well. Pretty bad that don't
> > > currently have a generic_encoder_unittests.cc, could you add one? No need
to
> > add
> > > tests for the existing logic in this cl though.
> > 
> > I am writing a second CL with tests in parallel. Generic encoder utittest
will
> > be added there, Is that OK?
> 
> I'd actually prefer they are in the same one. If it becomes too large it's
> better to split the cl by functionality instead. Adding unit tests later
usually
> just means finding out the problems later.

Working on it.

[ilnik, 2017-06-12 09:07:11]

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

[commit-bot: I haz the power, 2017-06-12 09:07: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-06-12 09:09:48]

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

[commit-bot: I haz the power, 2017-06-12 09:09:49]

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/22209)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[ilnik, 2017-06-12 10:08:50]

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.h (right):

https://codereview.webrtc.org/2911193002/diff/320001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.h:81: };
On 2017/06/12 07:10:10, sprang_webrtc wrote:
> On 2017/06/09 12:06:06, ilnik wrote:
> > On 2017/06/09 11:02:01, sprang_webrtc wrote:
> > > This logic definitely warrants units tests as well. Pretty bad that don't
> > > currently have a generic_encoder_unittests.cc, could you add one? No need
to
> > add
> > > tests for the existing logic in this cl though.
> > 
> > I am writing a second CL with tests in parallel. Generic encoder utittest
will
> > be added there, Is that OK?
> 
> I'd actually prefer they are in the same one. If it becomes too large it's
> better to split the cl by functionality instead. Adding unit tests later
usually
> just means finding out the problems later.

Added generic_encoder_unittests and also another test to
video_send_stream_tests.

[ilnik, 2017-06-12 10:09:30]

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

[commit-bot: I haz the power, 2017-06-12 10:09:38]

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-06-12 10:11:29]

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

[commit-bot: I haz the power, 2017-06-12 10:11:30]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26357)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/25316)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/25364)

[ilnik, 2017-06-12 10:16:35]

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

[commit-bot: I haz the power, 2017-06-12 10:16:38]

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-06-12 10:23:48]

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

[commit-bot: I haz the power, 2017-06-12 10:23:50]

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

[ilnik, 2017-06-12 10:30:06]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-06-12 10:30:10]

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-06-12 10:30:12]

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

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

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-06-12 10:30:34]

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

[commit-bot: I haz the power, 2017-06-12 10:30:39]

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-06-12 11:11:41]

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

[commit-bot: I haz the power, 2017-06-12 11:11:44]

Dry run: This issue passed the CQ dry run.

[åsapersson, 2017-06-12 14:33:42]

Some comments for webrtc/modules/rtp_rtcp.

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

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:248: // 5 timestamps in
milliseconds counted from capture time stored in rtp header:
6 timestamps

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:249: // ecode
start/finish, packetization complete, pacer exit and reserved for
encode

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:274:
timing->encode_finish_ms_delta = ByteReader<uint16_t, 2>::ReadBigEndian(
ByteReader<uint16_t> here and below.

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

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:138: static size_t
ValueSize(VideoTiming) { return kValueSizeBytes; }
const VideoTiming&

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:139: static size_t
ValueSize(uint16_t delta_ms, uint8_t idx) {
time_delta_ms

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:139: static size_t
ValueSize(uint16_t delta_ms, uint8_t idx) {
maybe move to line 143 above Write

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right):

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:111:
rtp_header->type.Video.video_timing.is_timing_frame = false;
maybe move to line 94 as others

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:743: if
(packet->HasExtension<VideoTimingExtension>())
packet_to_send?

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:744:
packet->set_pacer_exit_time_ms(clock_->TimeInMilliseconds());
maybe use now_ms

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:745: 
Maybe add a unit test, e.g. in rtp_sender_unittest.cc.

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

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:397: // Put packetization
finish timestamp into extension;
nit end with.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:398: if (last &&
is_timing_frame) {
Can HasExtension be used instead of is_timing_frame?

[ilnik, 2017-06-13 08:43:14]

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

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:248: // 5 timestamps in
milliseconds counted from capture time stored in rtp header:
On 2017/06/12 14:33:41, åsapersson wrote:
> 6 timestamps

Done.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:249: // ecode
start/finish, packetization complete, pacer exit and reserved for
On 2017/06/12 14:33:41, åsapersson wrote:
> encode

Done.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:274:
timing->encode_finish_ms_delta = ByteReader<uint16_t, 2>::ReadBigEndian(
On 2017/06/12 14:33:41, åsapersson wrote:
> ByteReader<uint16_t> here and below.

Done.

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

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:138: static size_t
ValueSize(VideoTiming) { return kValueSizeBytes; }
On 2017/06/12 14:33:41, åsapersson wrote:
> const VideoTiming&

Done.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:139: static size_t
ValueSize(uint16_t delta_ms, uint8_t idx) {
On 2017/06/12 14:33:41, åsapersson wrote:
> maybe move to line 143 above Write

Done.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right):

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:111:
rtp_header->type.Video.video_timing.is_timing_frame = false;
On 2017/06/12 14:33:42, åsapersson wrote:
> maybe move to line 94 as others

Done.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:743: if
(packet->HasExtension<VideoTimingExtension>())
On 2017/06/12 14:33:42, åsapersson wrote:
> packet_to_send?

Done. Thank's for catching that.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:744:
packet->set_pacer_exit_time_ms(clock_->TimeInMilliseconds());
On 2017/06/12 14:33:42, åsapersson wrote:
> maybe use now_ms

Done.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:745: 
On 2017/06/12 14:33:42, åsapersson wrote:
> Maybe add a unit test, e.g. in rtp_sender_unittest.cc.

Done. Please check rtp_sender_unittest.cc

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

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:397: // Put packetization
finish timestamp into extension;
On 2017/06/12 14:33:42, åsapersson wrote:
> nit end with.

Done.

https://codereview.webrtc.org/2911193002/diff/420001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:398: if (last &&
is_timing_frame) {
On 2017/06/12 14:33:42, åsapersson wrote:
> Can HasExtension be used instead of is_timing_frame?

Yes, but they are set together at lines 337-339 exactly for the purpose of not
calling HasExtension on every packet. I think it's more readable to have an
explicit flag.

[sprang_webrtc, 2017-06-13 14:14:15]

Thanks for adding tests, looks good!

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:477:
kAbsoluteSendTimeExtensionId));
why toffset and ast extensions?

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:484: VideoTiming
kVideoTiming = {0u, 0u, 0u, 0u, 0u, 0u, true};
const

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:504:
rtp_header.extension.videoTiming.pacer_exit_ms_delta);
nit: indentation?

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

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:336:
input_frame.video_frame_buffer()->ToI420();
Why? Rebase?

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:238: auto& encode_start_map =
nit: make it a pointer instead of a reference

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:19: namespace
generic_encoder_tests {
Usually only call the namespace test, but up to you

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:42: };
You can probably replace this with a gmock instead.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:47: Dropped,
nit: kCamelCase or ALL_CAPS

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:62:
VCMEncodedFrameCallback callback(&sink, nullptr);
Maybe it would be a good idea to have a TestVCMEncodedFrameCallback base class,
then this method and the callback instance etc could be protected members.
Otherwise put the utilities in an anonymous namespace

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:114: int i, s;
Why not just declare in the for statements?

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:125: ++num_dropped;
nit: Use brackets for if/else

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:354: timing_frame_observed_ = true;
Do you need this variable?

[ilnik, 2017-06-13 14:55:44]

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:477:
kAbsoluteSendTimeExtensionId));
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> why toffset and ast extensions?

Leftover from copypaste and playing around. Removed.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:484: VideoTiming
kVideoTiming = {0u, 0u, 0u, 0u, 0u, 0u, true};
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> const

Done.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:504:
rtp_header.extension.videoTiming.pacer_exit_ms_delta);
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> nit: indentation?

Done.

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

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:336:
input_frame.video_frame_buffer()->ToI420();
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> Why? Rebase?

Yes.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder.cc:238: auto& encode_start_map =
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> nit: make it a pointer instead of a reference

Done.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:19: namespace
generic_encoder_tests {
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> Usually only call the namespace test, but up to you

Done.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:42: };
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> You can probably replace this with a gmock instead.

I think it will be too hard, because it will require determining what exact
values should be passed here beforehand. It's much easier to just generate all
the values and later check them all together.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:47: Dropped,
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> nit: kCamelCase or ALL_CAPS

Done.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:62:
VCMEncodedFrameCallback callback(&sink, nullptr);
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> Maybe it would be a good idea to have a TestVCMEncodedFrameCallback base
class,
> then this method and the callback instance etc could be protected members.
> Otherwise put the utilities in an anonymous namespace

Done.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:114: int i, s;
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> Why not just declare in the for statements?

Done.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:125: ++num_dropped;
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> nit: Use brackets for if/else

Done.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:354: timing_frame_observed_ = true;
On 2017/06/13 14:14:14, sprang_webrtc wrote:
> Do you need this variable?

You are right, it's not needed, because if observation_complete_ is not set,
test will fail.

[åsapersson, 2017-06-14 08:03:51]

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:834:
packet->set_pacer_exit_time_ms(now_ms);
check indentation

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:353: 
remove extra line

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:512: 
Perhaps also add a test which calls rtp_sender_video_->SendVideo and verify
packetization_finish_ms_delta and timing timestamps in RTPVideoHeader.

[nisse-webrtc, 2017-06-14 09:24:43]

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:19: namespace
generic_encoder_tests {
On 2017/06/13 14:55:44, ilnik wrote:
> On 2017/06/13 14:14:14, sprang_webrtc wrote:
> > Usually only call the namespace test, but up to you
> 
> Done.

Since this is in a cc file, why not simply use the anonymous namespace?
That seems common in other test files, and the name is never used as far as I
see.

[ilnik, 2017-06-14 10:17:11]

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:834:
packet->set_pacer_exit_time_ms(now_ms);
On 2017/06/14 08:03:50, åsapersson wrote:
> check indentation

Done.

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:353: 
On 2017/06/14 08:03:50, åsapersson wrote:
> remove extra line

Done.

https://codereview.webrtc.org/2911193002/diff/460001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:512: 
On 2017/06/14 08:03:50, åsapersson wrote:
> Perhaps also add a test which calls rtp_sender_video_->SendVideo and verify
> packetization_finish_ms_delta and timing timestamps in RTPVideoHeader.

Done.

[ilnik, 2017-06-14 10:17:25]

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

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

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-06-14 10:49:07]

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

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

Dry run: This issue passed the CQ dry run.

[ilnik, 2017-06-14 11:10:37]

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:19: namespace
generic_encoder_tests {
On 2017/06/14 09:24:43, nisse-webrtc wrote:
> On 2017/06/13 14:55:44, ilnik wrote:
> > On 2017/06/13 14:14:14, sprang_webrtc wrote:
> > > Usually only call the namespace test, but up to you
> > 
> > Done.
> 
> Since this is in a cc file, why not simply use the anonymous namespace?
> That seems common in other test files, and the name is never used as far as I
> see.

The FrameType enum, even if in anonymous namespace is conflicting with
WebRTC::FrameType. Thus, some nested namespace is needed.

[nisse-webrtc, 2017-06-14 11:16:04]

lgtm for the things I've commented on.

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/generic_encoder_unittest.cc (right):

https://codereview.webrtc.org/2911193002/diff/440001/webrtc/modules/video_cod...
webrtc/modules/video_coding/generic_encoder_unittest.cc:19: namespace
generic_encoder_tests {
On 2017/06/14 11:10:37, ilnik wrote:
> On 2017/06/14 09:24:43, nisse-webrtc wrote:
> > On 2017/06/13 14:55:44, ilnik wrote:
> > > On 2017/06/13 14:14:14, sprang_webrtc wrote:
> > > > Usually only call the namespace test, but up to you
> > > 
> > > Done.
> > 
> > Since this is in a cc file, why not simply use the anonymous namespace?
> > That seems common in other test files, and the name is never used as far as
I
> > see.
> 
> The FrameType enum, even if in anonymous namespace is conflicting with
> WebRTC::FrameType. Thus, some nested namespace is needed.

I see. Keeping the namespace for now seems ok, but I think it would be better to
rename the enum to avoid colliding. Even if the extra namespace is clear enough
to the compiler, it's still a bit confusing for us humans.

[sprang_webrtc, 2017-06-16 09:05:07]

lgtm

[åsapersson, 2017-06-16 10:00:41]

webrtc/modules/rtp_rtcp: lgtm

[ilnik, 2017-06-16 11:23:37]

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

[commit-bot: I haz the power, 2017-06-16 11:23:48]

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

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

[commit-bot: I haz the power, 2017-06-16 12:25:27]

Dry run: This issue passed the CQ dry run.

[ilnik, 2017-06-16 12:46:04]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-06-16 12:46:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-16 12:53:34]

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

[commit-bot: I haz the power, 2017-06-16 12:53:36]

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

[ilnik, 2017-06-16 12:57:41]

Stefan, your approval is needed for:
    webrtc/api/BUILD.gn
    webrtc/api/video/video_timing.h
    webrtc/common_types.cc
    webrtc/common_types.h
    webrtc/common_video/include/video_frame.h
    webrtc/config.cc
    webrtc/config.h
    webrtc/media/engine/webrtcvideoengine.cc
    webrtc/modules/include/module_common_types.h
    webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc
    webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.mm
    webrtc/test/constants.cc
    webrtc/test/constants.h
    webrtc/test/fake_encoder.cc
    webrtc/test/frame_generator_capturer.cc
    webrtc/test/fuzzers/rtp_packet_fuzzer.cc

These changes are trivial. Please have a look at that. If you have time, feel
free to look at everything else.

[holmer, 2017-06-19 07:48:24]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2017-06-19 07:48:27]

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:24: static const uint8_t kEncodeFinishIdx = 1;
Why doesn't this have delta in the name?

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:28: static const uint8_t
kNetwork2TimestampDeltaIdx = 5;
What does 2 mean?

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:39: uint16_t encode_start_ms_delta;
I'd prefer delta before _ms:
encode_start_delta_ms

as ms is the unit.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/common_types.h#ne...
webrtc/common_types.h:596: // |outlier_ratio / 100.0 * bitrate_bps / fps| in
bits.
Can you comment on why we have logic like this? Because we want them not to be
sent too often? Do we want them on large frames for a reason?

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/common_types.h#ne...
webrtc/common_types.h:600: } timingFrameTriggerThresholds;
timing_frame_thresholds

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/common_types.h#ne...
webrtc/common_types.h:778: VideoTiming videoTiming;
No camelCase. I see why you used it, but since we are using non_camel_case below
I think you can do that too.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:151: specifics.codecSpecific.generic.simulcast_idx =
i;
Is this fixing a test bug?

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
File webrtc/video/payload_router.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:136: encoded_image.timing_.encode_start_ms);
Is there a reason why the delta is computed here and not when the
encode_start_ms timestamp is taken? AFAICT you might just as well do it there
already?

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:143:
rtp_video_header.video_timing.network2_timstamp_ms_delta = 0;
Maybe add a default ctor which initializes everything to zero instead?

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:627: // In some cases, e.g. then the frame from
decoder is fed to encoder,
"e.g. when"?

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:628: // the timestamp may be set to the future. As
the encoding pipeline assumes
I don't understand why this would be the case. Can you explain why frames passed
from the decoder to the encoder may have timestamps in the future?

[nisse-webrtc, 2017-06-19 08:08:40]

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:627: // In some cases, e.g. then the frame from
decoder is fed to encoder,
On 2017/06/19 07:48:26, holmer wrote:
> "e.g. when"?

To be *really* picky, it should be ", e.g., when".

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:628: // the timestamp may be set to the future. As
the encoding pipeline assumes
On 2017/06/19 07:48:26, holmer wrote:
> I don't understand why this would be the case. Can you explain why frames
passed
> from the decoder to the encoder may have timestamps in the future?

Because the decoder doesn't even try to attach the original capture time (which
would lie in the past). Instead, it attaches a timestamp for a suitable render
time, which by intention is in the future.

At least that's my understanding. I would prefer if the decoder estimated the
(past) capture time as well as it can, and playout delay for rendering was added
somewhere else, closer to actual rendering. But that's a refactoring for a
different cl.

[ilnik, 2017-06-19 08:41:51]

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:24: static const uint8_t kEncodeFinishIdx = 1;
On 2017/06/19 07:48:26, holmer wrote:
> Why doesn't this have delta in the name?

Overlooked typo. Fixed.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:28: static const uint8_t
kNetwork2TimestampDeltaIdx = 5;
On 2017/06/19 07:48:26, holmer wrote:
> What does 2 mean?

We just have 2 different timestamps reserved for network processors to modify.
Some rtp stream processor might e.g. mark when it received a packet and when it
sent it out. Since we don't care about it inside WebRTC I used generic names
network and network2. Can change it to anything you suggest.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:39: uint16_t encode_start_ms_delta;
On 2017/06/19 07:48:26, holmer wrote:
> I'd prefer delta before _ms:
> encode_start_delta_ms
> 
> as ms is the unit.

Done.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/common_types.h#ne...
webrtc/common_types.h:596: // |outlier_ratio / 100.0 * bitrate_bps / fps| in
bits.
On 2017/06/19 07:48:26, holmer wrote:
> Can you comment on why we have logic like this? Because we want them not to be
> sent too often? Do we want them on large frames for a reason?

Done.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/common_types.h#ne...
webrtc/common_types.h:600: } timingFrameTriggerThresholds;
On 2017/06/19 07:48:26, holmer wrote:
> timing_frame_thresholds

Done.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/common_types.h#ne...
webrtc/common_types.h:778: VideoTiming videoTiming;
On 2017/06/19 07:48:26, holmer wrote:
> No camelCase. I see why you used it, but since we are using non_camel_case
below
> I think you can do that too.

Done.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:151: specifics.codecSpecific.generic.simulcast_idx =
i;
On 2017/06/19 07:48:26, holmer wrote:
> Is this fixing a test bug?

Yes, new logic in this CL requires some kind of simulcast id for all frames, and
this fake encoder was the only one not setting it correctly.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
File webrtc/video/payload_router.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:136: encoded_image.timing_.encode_start_ms);
On 2017/06/19 07:48:26, holmer wrote:
> Is there a reason why the delta is computed here and not when the
> encode_start_ms timestamp is taken? AFAICT you might just as well do it there
> already?

Reason is: we have the same data-structure to store timestamps on sending time
and on receiving time in EncodedImage. Doing otherwise would require to separate
data-structures in EncodedImage for send and receive sides. Thus, we can't store
deltas in EncodedImage, and they are computed then rtp video header is formed.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:143:
rtp_video_header.video_timing.network2_timstamp_ms_delta = 0;
On 2017/06/19 07:48:26, holmer wrote:
> Maybe add a default ctor which initializes everything to zero instead?

Tried that, it causes some weird problems with RTPVideoHeader class:
error: [chromium-style] Complex class/struct needs an explicit out-of-line
constructor.

Adding there constructor seems like a too big of a change, it also creates a
cascade change to WebRtcRTPHeader and so on.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:627: // In some cases, e.g. then the frame from
decoder is fed to encoder,
On 2017/06/19 07:48:26, holmer wrote:
> "e.g. when"?

Done.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:627: // In some cases, e.g. then the frame from
decoder is fed to encoder,
On 2017/06/19 08:08:39, nisse-webrtc wrote:
> On 2017/06/19 07:48:26, holmer wrote:
> > "e.g. when"?
> 
> To be *really* picky, it should be ", e.g., when".

Done.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:628: // the timestamp may be set to the future. As
the encoding pipeline assumes
On 2017/06/19 08:08:39, nisse-webrtc wrote:
> On 2017/06/19 07:48:26, holmer wrote:
> > I don't understand why this would be the case. Can you explain why frames
> passed
> > from the decoder to the encoder may have timestamps in the future?
> 
> Because the decoder doesn't even try to attach the original capture time
(which
> would lie in the past). Instead, it attaches a timestamp for a suitable render
> time, which by intention is in the future.
> 
> At least that's my understanding. I would prefer if the decoder estimated the
> (past) capture time as well as it can, and playout delay for rendering was
added
> somewhere else, closer to actual rendering. But that's a refactoring for a
> different cl.

Yes, that's what happens.

[ilnik, 2017-06-19 09:11:07]

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

[commit-bot: I haz the power, 2017-06-19 09:11: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-06-19 09:32:51]

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

[commit-bot: I haz the power, 2017-06-19 09:32:52]

Dry run: This issue passed the CQ dry run.

[holmer, 2017-06-19 12:17:47]

lgtm, but consider removing network2 if it makes sense.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:28: static const uint8_t
kNetwork2TimestampDeltaIdx = 5;
On 2017/06/19 08:41:51, ilnik wrote:
> On 2017/06/19 07:48:26, holmer wrote:
> > What does 2 mean?
> 
> We just have 2 different timestamps reserved for network processors to modify.
> Some rtp stream processor might e.g. mark when it received a packet and when
it
> sent it out. Since we don't care about it inside WebRTC I used generic names
> network and network2. Can change it to anything you suggest.

Ok, so we don't know if we need two? Maybe just keep what we actually will use
in that case?

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
File webrtc/video/payload_router.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:136: encoded_image.timing_.encode_start_ms);
On 2017/06/19 08:41:51, ilnik wrote:
> On 2017/06/19 07:48:26, holmer wrote:
> > Is there a reason why the delta is computed here and not when the
> > encode_start_ms timestamp is taken? AFAICT you might just as well do it
there
> > already?
> 
> Reason is: we have the same data-structure to store timestamps on sending time
> and on receiving time in EncodedImage. Doing otherwise would require to
separate
> data-structures in EncodedImage for send and receive sides. Thus, we can't
store
> deltas in EncodedImage, and they are computed then rtp video header is formed.

Acknowledged.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:143:
rtp_video_header.video_timing.network2_timstamp_ms_delta = 0;
On 2017/06/19 08:41:51, ilnik wrote:
> On 2017/06/19 07:48:26, holmer wrote:
> > Maybe add a default ctor which initializes everything to zero instead?
> 
> Tried that, it causes some weird problems with RTPVideoHeader class:
> error: [chromium-style] Complex class/struct needs an explicit out-of-line
> constructor.
> 
> Adding there constructor seems like a too big of a change, it also creates a
> cascade change to WebRtcRTPHeader and so on.

Acknowledged.

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:628: // the timestamp may be set to the future. As
the encoding pipeline assumes
On 2017/06/19 08:08:39, nisse-webrtc wrote:
> On 2017/06/19 07:48:26, holmer wrote:
> > I don't understand why this would be the case. Can you explain why frames
> passed
> > from the decoder to the encoder may have timestamps in the future?
> 
> Because the decoder doesn't even try to attach the original capture time
(which
> would lie in the past). Instead, it attaches a timestamp for a suitable render
> time, which by intention is in the future.
> 
> At least that's my understanding. I would prefer if the decoder estimated the
> (past) capture time as well as it can, and playout delay for rendering was
added
> somewhere else, closer to actual rendering. But that's a refactoring for a
> different cl.

Acknowledged, and I agree with your preferences. :)

[ilnik, 2017-06-19 12:51:58]

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/2911193002/diff/500001/webrtc/api/video/video_t...
webrtc/api/video/video_timing.h:28: static const uint8_t
kNetwork2TimestampDeltaIdx = 5;
On 2017/06/19 12:17:47, holmer wrote:
> On 2017/06/19 08:41:51, ilnik wrote:
> > On 2017/06/19 07:48:26, holmer wrote:
> > > What does 2 mean?
> > 
> > We just have 2 different timestamps reserved for network processors to
modify.
> > Some rtp stream processor might e.g. mark when it received a packet and when
> it
> > sent it out. Since we don't care about it inside WebRTC I used generic names
> > network and network2. Can change it to anything you suggest.
> 
> Ok, so we don't know if we need two? Maybe just keep what we actually will use
> in that case?

No, I know that one downstream project wants to have two timestamps.

[ilnik, 2017-06-19 12:52:11]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-06-19 12:52:12]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
asapersson@webrtc.org, sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2911193002/#ps520001 (title:
"Implement Holmer@ comments")

[commit-bot: I haz the power, 2017-06-19 12:52: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, 2017-06-19 13:00:59]

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

[commit-bot: I haz the power, 2017-06-19 13:01:03]

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

[stefan-webrtc, 2017-06-19 14:10:55]

lgtm

[ilnik, 2017-06-19 14:16:10]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-06-19 14:16:15]

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-06-19 14:18:56]

CQ is committing da patch.

Bot data: {"patchset_id": 520001, "attempt_start_ts": 1497881770994300,
"parent_rev": "3b921f08568bd0a540d0be483a005082ebf9d4a7", "commit_rev":
"04f4d126f84dbbb915f52d91807cabdabf08d483"}

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

Description was changed from

==========
Implement timing frames.

Timing information is gathered in EncodedImage,
starting at encoders. Then it's sent using RTP header extension. In the
end, it's gathered at the GenericDecoder. Actual reporting and tests
will be in the next CLs.

BUG=webrtc:7594
==========

to

==========
Implement timing frames.

Timing information is gathered in EncodedImage,
starting at encoders. Then it's sent using RTP header extension. In the
end, it's gathered at the GenericDecoder. Actual reporting and tests
will be in the next CLs.

BUG=webrtc:7594

Review-Url: https://codereview.webrtc.org/2911193002
Cr-Commit-Position: refs/heads/master@{#18659}
Committed:
https://chromium.googlesource.com/external/webrtc/+/04f4d126f84dbbb915f52d918...
==========

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

Committed patchset #27 (id:520001) as
https://chromium.googlesource.com/external/webrtc/+/04f4d126f84dbbb915f52d918...