Implement adaptive target delay extension

Implement adaptive target delay extension

Adds new call on cast_sender: SetTargetPlayoutDelay
Caller is responsible for negotiating with the receiver if the extension is actually supported or not.

BUG=405339
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291453

[Alpha Left Google, 2014-08-20 20:17:06]

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender.h#new...
media/cast/cast_sender.h:91: // supports the "extra_target_delay" rtp extension.
nit: "adaptive_playout_delay"

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender_impl.cc
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender_impl....
media/cast/cast_sender_impl.cc:156:
audio_sender_->SetTargetPlayoutDelay(new_target_playout_delay);
What happens if we:

1. InitializeVideo()
2. SetTargetPlayoutDelay()
3. InitializeAudio()

Then the target delay for audio and video would be different.

https://codereview.chromium.org/493823002/diff/1/media/cast/net/rtp/rtp_defin...
File media/cast/net/rtp/rtp_defines.h (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/net/rtp/rtp_defin...
media/cast/net/rtp/rtp_defines.h:17: static const uint8
kCastRTPExtensionAdaptiveLatency = 1;
nit: kCastRtpExtensionAdaptiveLatency to be consistent with code above.

https://codereview.chromium.org/493823002/diff/1/media/cast/net/rtp/rtp_packe...
File media/cast/net/rtp/rtp_packetizer.cc (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/net/rtp/rtp_packe...
media/cast/net/rtp/rtp_packetizer.cc:76: byte0 |= 1;
I suggest having an additional counter for the number of extensions and then OR
later. It'll be more readable than hardcoding 1 and easy to add new extensions
later.

https://codereview.chromium.org/493823002/diff/1/media/cast/sender/audio_send...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/sender/audio_send...
media/cast/sender/audio_sender.cc:37: kAudioFrameRate * 2.0, // We lie to
increase max outstanding frames
nit: period at the end of sentence.

[hubbe, 2014-08-20 21:42:02]

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender.h
File media/cast/cast_sender.h (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender.h#new...
media/cast/cast_sender.h:91: // supports the "extra_target_delay" rtp extension.
On 2014/08/20 20:17:05, Alpha wrote:
> nit: "adaptive_playout_delay"

Done.

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender_impl.cc
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender_impl....
media/cast/cast_sender_impl.cc:156:
audio_sender_->SetTargetPlayoutDelay(new_target_playout_delay);
On 2014/08/20 20:17:05, Alpha wrote:
> What happens if we:
> 
> 1. InitializeVideo()
> 2. SetTargetPlayoutDelay()
> 3. InitializeAudio()
> 
> Then the target delay for audio and video would be different.

Depends on if (3) uses the same playout delay as (1) or (2).
A sane person would set the playout delay in (3) to the same as (2), which would
do what you want.

https://codereview.chromium.org/493823002/diff/1/media/cast/net/rtp/rtp_defin...
File media/cast/net/rtp/rtp_defines.h (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/net/rtp/rtp_defin...
media/cast/net/rtp/rtp_defines.h:17: static const uint8
kCastRTPExtensionAdaptiveLatency = 1;
On 2014/08/20 20:17:05, Alpha wrote:
> nit: kCastRtpExtensionAdaptiveLatency to be consistent with code above.

Done.

https://codereview.chromium.org/493823002/diff/1/media/cast/net/rtp/rtp_packe...
File media/cast/net/rtp/rtp_packetizer.cc (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/net/rtp/rtp_packe...
media/cast/net/rtp/rtp_packetizer.cc:76: byte0 |= 1;
On 2014/08/20 20:17:06, Alpha wrote:
> I suggest having an additional counter for the number of extensions and then
OR
> later. It'll be more readable than hardcoding 1 and easy to add new extensions
> later.

Total overkill, but ok.

https://codereview.chromium.org/493823002/diff/1/media/cast/sender/audio_send...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/sender/audio_send...
media/cast/sender/audio_sender.cc:37: kAudioFrameRate * 2.0, // We lie to
increase max outstanding frames
On 2014/08/20 20:17:06, Alpha wrote:
> nit: period at the end of sentence.

Done.

[Alpha Left Google, 2014-08-20 21:56:37]

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender_impl.cc
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender_impl....
media/cast/cast_sender_impl.cc:156:
audio_sender_->SetTargetPlayoutDelay(new_target_playout_delay);
On 2014/08/20 21:42:01, hubbe wrote:
> On 2014/08/20 20:17:05, Alpha wrote:
> > What happens if we:
> > 
> > 1. InitializeVideo()
> > 2. SetTargetPlayoutDelay()
> > 3. InitializeAudio()
> > 
> > Then the target delay for audio and video would be different.
> 
> Depends on if (3) uses the same playout delay as (1) or (2).
> A sane person would set the playout delay in (3) to the same as (2), which
would
> do what you want.
> 

When InitializeAudio() or InitializeVideo() is called the parameters are usually
the default values. For example these APIs are driven from the extension and
things can go wrong. I suggest we have a DCHECK or LOG in both InitializeAudio()
and InitializeVideo() to check the target delay matches the other stream.

[Alpha Left Google, 2014-08-20 22:02:01]

https://codereview.chromium.org/493823002/diff/20001/media/cast/cast_sender_i...
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/493823002/diff/20001/media/cast/cast_sender_i...
media/cast/cast_sender_impl.cc:155: if (audio_sender_) {
One more thing. There should be a VLOG for debugging.

[hubbe, 2014-08-20 22:46:07]

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender_impl.cc
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/493823002/diff/1/media/cast/cast_sender_impl....
media/cast/cast_sender_impl.cc:156:
audio_sender_->SetTargetPlayoutDelay(new_target_playout_delay);
On 2014/08/20 21:56:37, Alpha wrote:
> On 2014/08/20 21:42:01, hubbe wrote:
> > On 2014/08/20 20:17:05, Alpha wrote:
> > > What happens if we:
> > > 
> > > 1. InitializeVideo()
> > > 2. SetTargetPlayoutDelay()
> > > 3. InitializeAudio()
> > > 
> > > Then the target delay for audio and video would be different.
> > 
> > Depends on if (3) uses the same playout delay as (1) or (2).
> > A sane person would set the playout delay in (3) to the same as (2), which
> would
> > do what you want.
> > 
> 
> When InitializeAudio() or InitializeVideo() is called the parameters are
usually
> the default values. For example these APIs are driven from the extension and
> things can go wrong. I suggest we have a DCHECK or LOG in both
InitializeAudio()
> and InitializeVideo() to check the target delay matches the other stream.

Done.

https://codereview.chromium.org/493823002/diff/20001/media/cast/cast_sender_i...
File media/cast/cast_sender_impl.cc (right):

https://codereview.chromium.org/493823002/diff/20001/media/cast/cast_sender_i...
media/cast/cast_sender_impl.cc:155: if (audio_sender_) {
On 2014/08/20 22:02:01, Alpha wrote:
> One more thing. There should be a VLOG for debugging.

Done.

[Alpha Left Google, 2014-08-20 22:52:46]

The CQ bit was checked by hclam@chromium.org

[Alpha Left Google, 2014-08-20 22:52:47]

lgtm

[commit-bot: I haz the power, 2014-08-20 22:54:50]

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

[commit-bot: I haz the power, 2014-08-21 01:28:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-21 01:31:23]

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

[commit-bot: I haz the power, 2014-08-21 01:31:24]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[hubbe, 2014-08-21 21:54:45]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-08-21 21:57:22]

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

[commit-bot: I haz the power, 2014-08-21 22:21:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-21 22:24:29]

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

[commit-bot: I haz the power, 2014-08-21 22:24:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hubbe, 2014-08-21 22:27:00]

+ tsepez for security review

[Tom Sepez, 2014-08-21 22:41:03]

Messages LGTM.

https://codereview.chromium.org/493823002/diff/40001/media/cast/net/rtp/rtp_p...
File media/cast/net/rtp/rtp_packetizer.cc (right):

https://codereview.chromium.org/493823002/diff/40001/media/cast/net/rtp/rtp_p...
media/cast/net/rtp/rtp_packetizer.cc:92:
packet->data.push_back(frame.new_playout_delay_ms);
nit: do you want to explicitly mask this rather than just letting it get
truncated in push_back()?

[hubbe, 2014-08-22 16:38:33]

On 2014/08/21 22:41:03, Tom Sepez wrote:
> Messages LGTM.
> 
>
https://codereview.chromium.org/493823002/diff/40001/media/cast/net/rtp/rtp_p...
> File media/cast/net/rtp/rtp_packetizer.cc (right):
> 
>
https://codereview.chromium.org/493823002/diff/40001/media/cast/net/rtp/rtp_p...
> media/cast/net/rtp/rtp_packetizer.cc:92:
> packet->data.push_back(frame.new_playout_delay_ms);
> nit: do you want to explicitly mask this rather than just letting it get
> truncated in push_back()?

Seems redundant to me, but I suppose I could.
I assume the intention is to make it clear to the reader that it's getting
truncated?

[Tom Sepez, 2014-08-22 16:41:26]

> I assume the intention is to make it clear to the reader that it's getting
> truncated?
Exactly.

[hubbe, 2014-08-22 17:00:21]

On 2014/08/22 16:41:26, Tom Sepez wrote:
> > I assume the intention is to make it clear to the reader that it's getting
> > truncated?
> Exactly.

The problem with doing something redundant is that it will often cause the
reader to wonder why.
In my personal opinion, there is enough idiomatic clues around for the reader to
assume that truncation is taking place. If I add " & 0xff", it creates the
possibility in the mind of the reader that packet->data is something other than
a vector of uint8.

I'll still do it if you feel strongly about it though.

[Tom Sepez, 2014-08-22 17:03:53]

> I'll still do it if you feel strongly about it though.
Other nearby callers have a static_cast<uint8>().  That would make it clear
enough I think.

[hubbe, 2014-08-22 17:13:25]

On 2014/08/22 17:03:53, Tom Sepez wrote:
> > I'll still do it if you feel strongly about it though.
> Other nearby callers have a static_cast<uint8>().  That would make it clear
> enough I think.

Alright, I'll do that.

[Tom Sepez, 2014-08-22 17:13:58]

Still LGTM.  Thanks.

[hubbe, 2014-08-22 17:14:06]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-08-22 17:15:10]

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

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

Committed patchset #4 (60001) as 291453