[Cast] In Audio/VideoSender, drop frames when too-long a duration is in-flight.

[Cast] In Audio/VideoSender, drop frames when too-long a duration is in-flight.

BUG=404813, 405622
Committed: https://crrev.com/bbb875132105e0feeee0c0a56251ab83775a4fdb
Cr-Commit-Position: refs/heads/master@{#292673}

[miu, 2014-08-26 02:56:58]

miu@chromium.org changed reviewers:
+ hclam@chromium.org, hubbe@chromium.org

[miu, 2014-08-26 02:56:58]

PTAL.

[hubbe, 2014-08-26 17:49:56]

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/frame_send...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/frame_send...
media/cast/sender/frame_sender.h:100: int64 max_unacked_rtp_delta_;
Why not TimeDelta?

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/rtp_timest...
File media/cast/sender/rtp_timestamp_helper.h (right):

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/rtp_timest...
media/cast/sender/rtp_timestamp_helper.h:25: bool EstimateRtpTimestamp(const
base::TimeTicks& t,
Why is it an estimate?
Does it return "approximately" the right timestamp?

[miu, 2014-08-26 19:57:19]

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/frame_send...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/frame_send...
media/cast/sender/frame_sender.h:100: int64 max_unacked_rtp_delta_;
On 2014/08/26 17:49:56, hubbe wrote:
> Why not TimeDelta?

It more convenient to have in the time calculations being done in
Audio/VideoSender::ShouldDropNextFrame().  Slightly less math since both
AudioSender and VideoSender already store the prior frame RTP timestamps in the
frame_id_to_rtp_timestamp_ map.

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/rtp_timest...
File media/cast/sender/rtp_timestamp_helper.h (right):

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/rtp_timest...
media/cast/sender/rtp_timestamp_helper.h:25: bool EstimateRtpTimestamp(const
base::TimeTicks& t,
On 2014/08/26 17:49:56, hubbe wrote:
> Why is it an estimate?
> Does it return "approximately" the right timestamp?

Yes.  It's an estimate because, ultimately, it's the encoders that decide what
the RTP timestamps for each frame are going to be.  For example, in the case of
audio, the record_time will never be precise to a 10ms boundary; and so the
translation will be to an |rtp_timestamp| value that is near, but never exact to
one emitted downstream by the encoder.  In other words, it's an approximation.

[hubbe, 2014-08-26 20:54:21]

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/frame_send...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/frame_send...
media/cast/sender/frame_sender.h:100: int64 max_unacked_rtp_delta_;
On 2014/08/26 19:57:19, miu wrote:
> On 2014/08/26 17:49:56, hubbe wrote:
> > Why not TimeDelta?
> 
> It more convenient to have in the time calculations being done in
> Audio/VideoSender::ShouldDropNextFrame().  Slightly less math since both
> AudioSender and VideoSender already store the prior frame RTP timestamps in
the
> frame_id_to_rtp_timestamp_ map.

This is an odd situation. Usually you're the one who wants to use
timetick/timedelta for all time calculations....  I'm not convinced that the
convenience is worth it, it seems the code will be clearer without modular math.

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/rtp_timest...
File media/cast/sender/rtp_timestamp_helper.h (right):

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/rtp_timest...
media/cast/sender/rtp_timestamp_helper.h:25: bool EstimateRtpTimestamp(const
base::TimeTicks& t,
On 2014/08/26 19:57:19, miu wrote:
> On 2014/08/26 17:49:56, hubbe wrote:
> > Why is it an estimate?
> > Does it return "approximately" the right timestamp?
> 
> Yes.  It's an estimate because, ultimately, it's the encoders that decide what
> the RTP timestamps for each frame are going to be.  For example, in the case
of
> audio, the record_time will never be precise to a 10ms boundary; and so the
> translation will be to an |rtp_timestamp| value that is near, but never exact
to
> one emitted downstream by the encoder.  In other words, it's an approximation.

If we do this with timeticks/deltas instead, do we even need this estimate? If
the answer is yes, please update the comment to provide the same information as
your reply above.

[miu, 2014-08-26 23:27:16]

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/frame_send...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/frame_send...
media/cast/sender/frame_sender.h:100: int64 max_unacked_rtp_delta_;
On 2014/08/26 20:54:21, hubbe wrote:
> On 2014/08/26 19:57:19, miu wrote:
> > On 2014/08/26 17:49:56, hubbe wrote:
> > > Why not TimeDelta?
> > 
> > It more convenient to have in the time calculations being done in
> > Audio/VideoSender::ShouldDropNextFrame().  Slightly less math since both
> > AudioSender and VideoSender already store the prior frame RTP timestamps in
> the
> > frame_id_to_rtp_timestamp_ map.
> 
> This is an odd situation. Usually you're the one who wants to use
> timetick/timedelta for all time calculations....  I'm not convinced that the
> convenience is worth it, it seems the code will be clearer without modular
math.

Done.  I rewrote it the other way.  The ShouldDropNextFrame() code was about the
same in math, but the max_unacked_rtp_delta_ field could be removed and just use
target_playout_delay_.  So, I'll admit your way is cleaner.  ;-)

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/rtp_timest...
File media/cast/sender/rtp_timestamp_helper.h (right):

https://codereview.chromium.org/502333002/diff/1/media/cast/sender/rtp_timest...
media/cast/sender/rtp_timestamp_helper.h:25: bool EstimateRtpTimestamp(const
base::TimeTicks& t,
On 2014/08/26 20:54:21, hubbe wrote:
> On 2014/08/26 19:57:19, miu wrote:
> > On 2014/08/26 17:49:56, hubbe wrote:
> > > Why is it an estimate?
> > > Does it return "approximately" the right timestamp?
> > 
> > Yes.  It's an estimate because, ultimately, it's the encoders that decide
what
> > the RTP timestamps for each frame are going to be.  For example, in the case
> of
> > audio, the record_time will never be precise to a 10ms boundary; and so the
> > translation will be to an |rtp_timestamp| value that is near, but never
exact
> to
> > one emitted downstream by the encoder.  In other words, it's an
approximation.
> 
> If we do this with timeticks/deltas instead, do we even need this estimate? If
> the answer is yes, please update the comment to provide the same information
as
> your reply above.

Done.  This method is also used when generating SR's, so I explained the
"estimate" via code comments.

[miu, 2014-08-26 23:29:37]

BTW--This change has a (wonderful?) side effect: It starts dropping frames if
there are too many piled up in the encoder.  This may increase frame drop
numbers on low-end hardware, but I would expect to see less freezing/hiccups.

[miu, 2014-08-27 19:31:31]

On 2014/08/26 23:29:37, miu wrote:
> BTW--This change has a (wonderful?) side effect: It starts dropping frames if
> there are too many piled up in the encoder.  This may increase frame drop
> numbers on low-end hardware, but I would expect to see less freezing/hiccups.

Actually, nm.  This is only true while at least one frame is in-flight over the
network.  Also, the tab capture pipeline has a fixed pool size, preventing too
many frames from piling-up in the encoder anyway.

[hubbe, 2014-08-28 17:34:46]

https://codereview.chromium.org/502333002/diff/20001/media/cast/sender/audio_...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/502333002/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_sender.cc:116: encoded_frame->rtp_timestamp;
why don't we save the reference time here instead?

[miu, 2014-08-28 18:57:58]

https://codereview.chromium.org/502333002/diff/20001/media/cast/sender/audio_...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/502333002/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_sender.cc:116: encoded_frame->rtp_timestamp;
On 2014/08/28 17:34:45, hubbe wrote:
> why don't we save the reference time here instead?

We never save the reference time (see line 126 below).  I didn't want to
introduce a second [redundant] frame_id_to_capture_time_ map.

[hubbe, 2014-08-28 19:03:10]

https://codereview.chromium.org/502333002/diff/20001/media/cast/sender/audio_...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/502333002/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_sender.cc:116: encoded_frame->rtp_timestamp;
On 2014/08/28 18:57:58, miu wrote:
> On 2014/08/28 17:34:45, hubbe wrote:
> > why don't we save the reference time here instead?
> 
> We never save the reference time (see line 126 below).  I didn't want to
> introduce a second [redundant] frame_id_to_capture_time_ map.

I think another map would be better than translating timestamps to rtp and back.

Also, why are we setting frame_id_to_rtp_timestamp_ in two places?
Wouldn't it be sufficeint to just set it on line 110 or something?

[miu, 2014-08-28 20:37:16]

https://codereview.chromium.org/502333002/diff/20001/media/cast/sender/audio_...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/502333002/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_sender.cc:116: encoded_frame->rtp_timestamp;
On 2014/08/28 19:03:10, hubbe wrote:
> On 2014/08/28 18:57:58, miu wrote:
> > On 2014/08/28 17:34:45, hubbe wrote:
> > > why don't we save the reference time here instead?
> 
> I think another map would be better than translating timestamps to rtp and
back.

Done.  This led me to move a few "common members" into FrameSender, and then I
was able to trivially get rid of RtpTimestampHelper.  Moar LOC deleted!  :)

> Also, why are we setting frame_id_to_rtp_timestamp_ in two places?
> Wouldn't it be sufficeint to just set it on line 110 or something?

I removed this.  I think, in some earlier code patch, I was afraid of the
possibility of this element getting accessed the first time
ShouldDropNextFrame() was called.

[hubbe, 2014-08-29 03:23:46]

lgtm

[miu, 2014-08-29 05:29:53]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-08-29 05:30:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miu@chromium.org/502333002/40001

[commit-bot: I haz the power, 2014-08-29 05:41:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-29 05:42:55]

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

[commit-bot: I haz the power, 2014-08-29 05:42:56]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[miu, 2014-08-29 17:16:45]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-08-29 17:17:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miu@chromium.org/502333002/60001

[miu, 2014-08-29 17:44:10]

The CQ bit was unchecked by miu@chromium.org

[miu, 2014-08-29 17:44:35]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-08-29 17:45:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miu@chromium.org/502333002/80001

[commit-bot: I haz the power, 2014-08-29 18:46:22]

Committed patchset #5 (id:80001) as 6b036dd95ee51378550e1b3c3cc37a9b6d9a8886

[commit-bot: I haz the power, 2014-09-10 03:10:14]

Patchset 5 (id:??) landed as
https://crrev.com/bbb875132105e0feeee0c0a56251ab83775a4fdb
Cr-Commit-Position: refs/heads/master@{#292673}