Cast: Simplify code path for RTCP sender report

Cast: Simplify code path for RTCP sender report

Sending a RTCP sender report used to require a couple round trips of
IPCs. This is much simplified in this change. AudioSender or VideoSender
sends the latest known NTP time and corresponding RTP timestamp. Sends
it to CastTransportSender, which appends the current packet ccount and
octet count to form a sender report.

There is no need for AudioSender and VideoSender to know the current
packet count and octet count.

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

[hubbe, 2014-05-12 19:46:04]

LGTM, with one question/nit.

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

https://codereview.chromium.org/281453003/diff/1/media/cast/cast_defines.h#ne...
media/cast/cast_defines.h:197: *rtp_timestamp = last_rtp_timestamp_ +
elapsed_time.InMilliseconds() *
Is Millisecond accurate enough here?

[Alpha Left Google, 2014-05-12 19:49:33]

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

https://codereview.chromium.org/281453003/diff/1/media/cast/cast_defines.h#ne...
media/cast/cast_defines.h:197: *rtp_timestamp = last_rtp_timestamp_ +
elapsed_time.InMilliseconds() *
On 2014/05/12 19:46:04, hubbe wrote:
> Is Millisecond accurate enough here?

I tried not to change the algorithm used here so I preserve the usage of
milliseconds. I think in practice this is good enough.

[miu, 2014-05-13 01:11:56]

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

https://codereview.chromium.org/281453003/diff/1/media/cast/cast_defines.h#ne...
media/cast/cast_defines.h:179: class RtpTimestampHelper {
Please make this class Chromium-style compliant, and implement methods in a .cc
file, rather than in-line.

https://codereview.chromium.org/281453003/diff/1/media/cast/cast_defines.h#ne...
media/cast/cast_defines.h:193: uint32* rtp_timestamp) {
Method should be const.

https://codereview.chromium.org/281453003/diff/1/media/cast/cast_defines.h#ne...
media/cast/cast_defines.h:214: bool stored_;
You don't need this extra boolean member.  Just check for
last_capture_time.is_null().

https://codereview.chromium.org/281453003/diff/1/media/cast/rtcp/rtcp_unittes...
File media/cast/rtcp/rtcp_unittest.cc (right):

https://codereview.chromium.org/281453003/diff/1/media/cast/rtcp/rtcp_unittes...
media/cast/rtcp/rtcp_unittest.cc:203: base::TimeDelta start_time_;
You don't need this extra member (see comment below).

https://codereview.chromium.org/281453003/diff/1/media/cast/rtcp/rtcp_unittes...
media/cast/rtcp/rtcp_unittest.cc:209: base::TimeTicks start_time;
Suggestion: Delete this line and the next, and replace with:

  const base::TimeTicks start_time = testing_clock_->NowTicks();

https://codereview.chromium.org/281453003/diff/1/media/cast/video_sender/vide...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/281453003/diff/1/media/cast/video_sender/vide...
media/cast/video_sender/video_sender.cc:223: if
(rtp_timestamp_helper_.GetCurrentTimeAsRtpTimestamp(
We need to document in code comments why we're rolling forward the NTP and RTP
timestamps to "now."  Either here, or maybe better to do it in rtcp.h where
SendRtcpFromRtpSender() is declared.

[Alpha Left Google, 2014-05-13 21:45:38]

+jorgelo for *messages*.h

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

https://codereview.chromium.org/281453003/diff/1/media/cast/cast_defines.h#ne...
media/cast/cast_defines.h:179: class RtpTimestampHelper {
On 2014/05/13 01:11:56, miu wrote:
> Please make this class Chromium-style compliant, and implement methods in a
.cc
> file, rather than in-line.

Done.

https://codereview.chromium.org/281453003/diff/1/media/cast/cast_defines.h#ne...
media/cast/cast_defines.h:193: uint32* rtp_timestamp) {
On 2014/05/13 01:11:56, miu wrote:
> Method should be const.

Done.

https://codereview.chromium.org/281453003/diff/1/media/cast/cast_defines.h#ne...
media/cast/cast_defines.h:214: bool stored_;
On 2014/05/13 01:11:56, miu wrote:
> You don't need this extra boolean member.  Just check for
> last_capture_time.is_null().

Done.

https://codereview.chromium.org/281453003/diff/1/media/cast/rtcp/rtcp_unittes...
File media/cast/rtcp/rtcp_unittest.cc (right):

https://codereview.chromium.org/281453003/diff/1/media/cast/rtcp/rtcp_unittes...
media/cast/rtcp/rtcp_unittest.cc:203: base::TimeDelta start_time_;
On 2014/05/13 01:11:56, miu wrote:
> You don't need this extra member (see comment below).

Done.

https://codereview.chromium.org/281453003/diff/1/media/cast/rtcp/rtcp_unittes...
media/cast/rtcp/rtcp_unittest.cc:209: base::TimeTicks start_time;
On 2014/05/13 01:11:56, miu wrote:
> Suggestion: Delete this line and the next, and replace with:
> 
>   const base::TimeTicks start_time = testing_clock_->NowTicks();

Done.

https://codereview.chromium.org/281453003/diff/1/media/cast/video_sender/vide...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/281453003/diff/1/media/cast/video_sender/vide...
media/cast/video_sender/video_sender.cc:223: if
(rtp_timestamp_helper_.GetCurrentTimeAsRtpTimestamp(
On 2014/05/13 01:11:56, miu wrote:
> We need to document in code comments why we're rolling forward the NTP and RTP
> timestamps to "now."  Either here, or maybe better to do it in rtcp.h where

Already documented in rtcp.h.

[miu, 2014-05-14 03:56:30]

lgtm

[Jorge Lucangeli Obes, 2014-05-14 11:40:16]

chrome/common/cast_messages.h lgtm

[Alpha Left Google, 2014-05-14 17:37:37]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-05-14 17:38:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/281453003/20001

[commit-bot: I haz the power, 2014-05-14 18:08:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-14 18:12:18]

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

[commit-bot: I haz the power, 2014-05-14 18:12:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Alpha Left Google, 2014-05-14 21:28:07]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-05-14 21:28:53]

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

[Alpha Left Google, 2014-05-14 22:43:36]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-05-14 22:45:06]

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

[commit-bot: I haz the power, 2014-05-15 03:27:44]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-15 04:20:55]

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

[commit-bot: I haz the power, 2014-05-15 04:20:56]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Alpha Left Google, 2014-05-16 01:47:20]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-05-16 01:47:37]

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

[commit-bot: I haz the power, 2014-05-16 02:20:49]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-16 02:24:23]

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

[commit-bot: I haz the power, 2014-05-16 02:24:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Alpha Left Google, 2014-05-16 04:13:30]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-05-16 04:13:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/281453003/100001

[commit-bot: I haz the power, 2014-05-16 07:55:32]

Change committed as 270954