Cast:Adding cast_transport_config and cleaning up

Cast:Adding cast_transport_config and cleaning up

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

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

[mikhal1, 2013-12-27 22:27:46]

Please review,

-Mikhal

[hubbe, 2013-12-27 23:49:57]

LGTM with a few comments

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
File media/cast/transport/cast_transport_defines.h (right):

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
media/cast/transport/cast_transport_defines.h:27: const size_t kIpPacketSize =
1500;
kMaxIpPacketSize ?
Isn't 1500 bytes the MTU *before* the IP header has been added?
(IE, shouldn't this really be 1480 or something like that?)

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
File media/cast/transport/cast_transport_sender.h (right):

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
media/cast/transport/cast_transport_sender.h:32: class CastTransportSender :
public base::NonThreadSafe {
Since this is an interface, please make sure to document what the functions are
supposed to do and when they are supposed to be called.
Including caveats, like what thread it is supposed to be called on.

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
media/cast/transport/cast_transport_sender.h:45: virtual void
InsertCodedAudioFrame(const EncodedAudioFrame* audio_bus,
audio_bus -> audio_frame

[mikhal1, 2013-12-30 17:33:45]

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
File media/cast/transport/cast_transport_defines.h (right):

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
media/cast/transport/cast_transport_defines.h:27: const size_t kIpPacketSize =
1500;
It is indeed the max packet size, including the header. That's how we use it in
the packetizer. 
+Renamed to kMaxIpPacketSize
On 2013/12/27 23:49:58, hubbe wrote:
> kMaxIpPacketSize ?
> Isn't 1500 bytes the MTU *before* the IP header has been added?
> (IE, shouldn't this really be 1480 or something like that?)

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
File media/cast/transport/cast_transport_sender.h (right):

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
media/cast/transport/cast_transport_sender.h:32: class CastTransportSender :
public base::NonThreadSafe {
On 2013/12/27 23:49:58, hubbe wrote:
> Since this is an interface, please make sure to document what the functions
are
> supposed to do and when they are supposed to be called.
> Including caveats, like what thread it is supposed to be called on.

Done.

https://chromiumcodereview.appspot.com/109413004/diff/50001/media/cast/transp...
media/cast/transport/cast_transport_sender.h:45: virtual void
InsertCodedAudioFrame(const EncodedAudioFrame* audio_bus,
On 2013/12/27 23:49:58, hubbe wrote:
> audio_bus -> audio_frame

Done.

[mikhal1, 2014-01-06 16:33:39]

+scherkus for renderer/

[scherkus (not reviewing), 2014-01-07 01:54:13]

lgtm w/ a nit and a question about the extra namespace

https://chromiumcodereview.appspot.com/109413004/diff/780001/media/cast/trans...
File media/cast/transport/cast_transport_config.h (right):

https://chromiumcodereview.appspot.com/109413004/diff/780001/media/cast/trans...
media/cast/transport/cast_transport_config.h:17: namespace transport {
I'm not familiar w/ what motivated the additional namespace here, but given
there's already cast namespace I wonder how much additional value/typename
collision protection you gain with an additional transport namespace

(the addition of transport:: everywhere else in this CL is a bit unfortunate,
especially if it isn't strictly needed)

https://chromiumcodereview.appspot.com/109413004/diff/780001/media/cast/trans...
File media/cast/transport/pacing/paced_sender.h (right):

https://chromiumcodereview.appspot.com/109413004/diff/780001/media/cast/trans...
media/cast/transport/pacing/paced_sender.h:70: scoped_refptr<base::TaskRunner>
transport_thread_;
nit: this should be transport_task_runner_ (or similar)

[mikhal1, 2014-01-07 16:20:20]

https://chromiumcodereview.appspot.com/109413004/diff/780001/media/cast/trans...
File media/cast/transport/cast_transport_config.h (right):

https://chromiumcodereview.appspot.com/109413004/diff/780001/media/cast/trans...
media/cast/transport/cast_transport_config.h:17: namespace transport {
We decided that it's best to add the name space to resolve confusion and make
the two libraries distinct. 
On 2014/01/07 01:54:13, scherkus wrote:
> I'm not familiar w/ what motivated the additional namespace here, but given
> there's already cast namespace I wonder how much additional value/typename
> collision protection you gain with an additional transport namespace
> 
> (the addition of transport:: everywhere else in this CL is a bit unfortunate,
> especially if it isn't strictly needed)

https://chromiumcodereview.appspot.com/109413004/diff/780001/media/cast/trans...
File media/cast/transport/pacing/paced_sender.h (right):

https://chromiumcodereview.appspot.com/109413004/diff/780001/media/cast/trans...
media/cast/transport/pacing/paced_sender.h:70: scoped_refptr<base::TaskRunner>
transport_thread_;
Changed here and elsewhere.
On 2014/01/07 01:54:13, scherkus wrote:
> nit: this should be transport_task_runner_ (or similar)

[commit-bot: I haz the power, 2014-01-07 21:48:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/109413004/1120001

[commit-bot: I haz the power, 2014-01-08 00:54:26]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-08 15:55:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/109413004/1120001

[commit-bot: I haz the power, 2014-01-08 15:56:12]

Failed to apply patch for chrome/renderer/media/cast_rtp_stream.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/media/cast_rtp_stream.cc
  Hunk #2 FAILED at 78.
  Hunk #3 succeeded at 104 with fuzz 2 (offset 4 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/renderer/media/cast_rtp_stream.cc.rej

Patch:       chrome/renderer/media/cast_rtp_stream.cc
Index: chrome/renderer/media/cast_rtp_stream.cc
diff --git a/chrome/renderer/media/cast_rtp_stream.cc
b/chrome/renderer/media/cast_rtp_stream.cc
index
cd1a7aea901f6c7afc5da1473318fc60bb5c652c..71a6651788019552e5f3b8612fc1eba5b18bb4fe
100644
--- a/chrome/renderer/media/cast_rtp_stream.cc
+++ b/chrome/renderer/media/cast_rtp_stream.cc
@@ -15,6 +15,7 @@
 #include "media/cast/cast_config.h"
 #include "media/cast/cast_defines.h"
 #include "media/cast/cast_sender.h"
+#include "media/cast/transport/cast_transport_config.h"
 #include "third_party/WebKit/public/platform/WebMediaStreamSource.h"
 
 using media::cast::AudioSenderConfig;
@@ -77,9 +78,9 @@ bool ToAudioSenderConfig(const CastRtpParams& params,
   config->frequency = payload_params.clock_rate;
   config->channels = payload_params.channels;
   config->bitrate = payload_params.max_bitrate;
-  config->codec = media::cast::kPcm16;
+  config->codec = media::cast::transport::kPcm16;
   if (payload_params.codec_name == kCodecNameOpus)
-    config->codec = media::cast::kOpus;
+    config->codec = media::cast::transport::kOpus;
   else
     return false;
   return true;
@@ -99,7 +100,7 @@ bool ToVideoSenderConfig(const CastRtpParams& params,
   config->min_bitrate = config->start_bitrate = payload_params.min_bitrate;
   config->max_bitrate = payload_params.max_bitrate;
   if (payload_params.codec_name == kCodecNameVp8)
-    config->codec = media::cast::kVp8;
+    config->codec = media::cast::transport::kVp8;
   else
     return false;
   return true;

[commit-bot: I haz the power, 2014-01-08 17:00:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/109413004/1380001

[commit-bot: I haz the power, 2014-01-08 18:17:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/109413004/1860001

[commit-bot: I haz the power, 2014-01-08 18:36:39]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-08 18:39:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/109413004/1400004

[commit-bot: I haz the power, 2014-01-08 21:14:58]

Change committed as 243647

[bajones, 2014-01-08 21:31:41]

A revert of this CL has been created in
https://codereview.chromium.org/126843003/ by bajones@chromium.org.

The reason for reverting is: Reverting, since this seems to have broken the GPU
bots. Logs:
http://build.chromium.org/p/chromium.gpu/builders/GPU%20Linux%20Builder/build...
http://build.chromium.org/p/chromium.gpu/builders/GPU%20Mac%20Builder/builds/....

[commit-bot: I haz the power, 2014-01-08 21:38:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/109413004/2390001

[commit-bot: I haz the power, 2014-01-08 22:37:12]

Retried try job too often on linux_rel for step(s) check_deps2git
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-09 00:17:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhal@chromium.org/109413004/2390001

[commit-bot: I haz the power, 2014-01-09 04:06:07]

Change committed as 243755