Refactoring: Merge VideoSenderConfig and AudioSenderConfig.

Refactoring: Merge VideoSenderConfig and AudioSenderConfig.
In the CL: 1) VideoSenderConfig and AudioSenderConfig are merged to
FrameSenderConfig. 2) CastRtpParams is removed . RtpParams is directly
converted to/from FrameSenderConfig.

BUG=530839
Committed: https://crrev.com/4c97152e0f2b9bf5883d7ea7f149c932598c3afc
Cr-Commit-Position: refs/heads/master@{#404520}

[xjz, 2016-06-30 18:49:15]

xjz@chromium.org changed reviewers:
+ miu@chromium.org

[xjz, 2016-06-30 18:49:15]

PTAL

[miu, 2016-06-30 21:59:43]

Very nice clean-up! :)

Mostly nits:

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:79: const int
kBitrateMultiplier = 1000;
nit: s/const/constexpr/

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:117: DVLOG(1) <<
"min_playout_delay " << config->min_playout_delay
nit: Let's say "must be greater than zero" rather than "is too small."

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:124: DVLOG(1) <<
"min_playout_delay " << config->min_playout_delay
nit: Let's say "must be less than max playout delay" rather than "is too big."

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:133: << " is out of
range";
nit: Let's say "must be between (inclusive) the min and max playout delay"
rather than "is out of range."

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:143: : 0;
Instead of 0, can the default be kDefaultMaxVideoKbps (or
kDefaultAudioEncoderBitrate for audio)?

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:144:
config->channels = ext_params.channels ? *ext_params.channels : 0;
Let's default this to 1 for video and 2 for audio. Also, can we validate the
number (i.e., it must be 1 or 2 for audio, and 1 for video)?

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:145:
config->frequency = ext_params.clock_rate ? *ext_params.clock_rate : 0;
Let's default this to kDefaultAudioSamplingRate.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:150:
config->min_bitrate = config->max_bitrate;
also: config->start_bitrate = config->max_bitrate  (see comment in cast_config.h
about start_bitrate)

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:170: }
Also for Opus: Let's set config->max_frame_rate to 100 (10ms frames) for
completeness.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:232: if
(config.codec == media::cast::CODEC_AUDIO_OPUS)
With this if...elseif...else statement: I'm thinking it would be more robust to
do this instead for safety:

  switch (config.codec) {
  case CODEC_AUDIO_OPUS: codec_name = kCodecNameOpus;
  case CODEC_VIDEO_VP8: codec_name = kCodecNameVp8;
  case CODEC_VIDEO_H264: codec_name = kCodecNameH264;
  }

This would ensure the compiler let's a developer know they have to also change
this code if they add any codecs to the enum.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.cc:67: config.min_bitrate =
config.max_bitrate =
and start_bitrate (per comment in cast_config.h)

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.cc:78: config.max_playout_delay =
You could omit this, since we set this default in the FrameReceiverConfig ctor.
(and in DefaultOpusConfig() and DefaultH264Config())

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.cc:85: config.channels = 1;
nit: Can you move this to be just before max_bitrate? (so it matches the order
in the struct; easier for a reader to examine the code to make sure all fields
are set)

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
File chrome/renderer/media/cast_rtp_stream.h (left):

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.h:39: struct CastRtpPayloadParams {
Nice! :)

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

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:123: bool use_external_encoder;
Let's add a comment like, "If true, use an external encoder rather than the
built-in software-based one."

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:125: int frequency;
Now might be a good time to rename this to rtp_timebase (like in
FrameReceiverConfig below).

Also, feel free to copy the comment from FrameReceiverConfig to here, and also
for other things like 'channels' and 'codec'.

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:129: // Audio streams only use |max_bitrate|.
Probably not worth mentioning that audio uses only |max_bitrate|. In fact, maybe
we should add a TODO in AudioSender::InsertAudioFrame() to allow
dynamically-adjusting the bitrate based on congestion (just like we do for
VideoSender)?

Also, consider moving start_bitrate back to here.

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:133: double max_frame_rate;
Nice! Thanks for resolving the TODO! :)

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:144: CodecSpecificParams codec_specific_params;
Please rename (for precision). Suggestion:

s/CodecSpecificParams/VideoCodecParams/

and

s/codec_specific_params/video_codec_params/

https://codereview.chromium.org/2113783002/diff/1/media/cast/sender/fake_soft...
File media/cast/sender/fake_software_video_encoder.cc (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/sender/fake_soft...
media/cast/sender/fake_software_video_encoder.cc:75:
DCHECK_GT(video_config_.max_frame_rate, 0);
Seems that this could be in the ctor (to just check once).

https://codereview.chromium.org/2113783002/diff/1/media/cast/sender/fake_soft...
File media/cast/sender/fake_software_video_encoder.h (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/sender/fake_soft...
media/cast/sender/fake_software_video_encoder.h:31: FrameSenderConfig
video_config_;
const please.

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/cast_benchm...
File media/cast/test/cast_benchmarks.cc (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/cast_benchm...
media/cast/test/cast_benchmarks.cc:262: base::TimeDelta::FromSeconds(1) /
video_sender_config_.max_frame_rate;
nit: base::TimeDelta::FromSecondsF(1.0 / video_sender_config_.max_frame_rate);

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/cast_benchm...
media/cast/test/cast_benchmarks.cc:289: video_sender_config_.max_frame_rate;
ditto: base::TimeDelta::FromSecondsF(1.0 / video_sender_config_.max_frame_rate);

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/end2end_uni...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/end2end_uni...
media/cast/test/end2end_unittest.cc:493: video_sender_config_.max_frame_rate;
Now this is a double being assigned to an int-typed field. Did you want to
either: 1) Add round-to-nearest int; or 2) Change
FrameReceiverConfig::target_frame_rate to be a double type (which may or may not
trickle into a whole bunch more files)>

I'll leave it up to you how far you want to go in this change. ;-)

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/utility/def...
File media/cast/test/utility/default_config.cc (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/utility/def...
media/cast/test/utility/default_config.cc:91:
config.codec_specific_params.number_of_encode_threads = 1;
This was "2" before. Did you mean to change it?

[xjz, 2016-07-01 23:04:24]

Patchset #2 (id:20001) has been deleted

[xjz, 2016-07-01 23:52:10]

Addressed comments. PTAL.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:79: const int
kBitrateMultiplier = 1000;
On 2016/06/30 21:59:42, miu wrote:
> nit: s/const/constexpr/

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:117: DVLOG(1) <<
"min_playout_delay " << config->min_playout_delay
On 2016/06/30 21:59:42, miu wrote:
> nit: Let's say "must be greater than zero" rather than "is too small."

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:124: DVLOG(1) <<
"min_playout_delay " << config->min_playout_delay
On 2016/06/30 21:59:42, miu wrote:
> nit: Let's say "must be less than max playout delay" rather than "is too big."

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:133: << " is out of
range";
On 2016/06/30 21:59:42, miu wrote:
> nit: Let's say "must be between (inclusive) the min and max playout delay"
> rather than "is out of range."

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:143: : 0;
On 2016/06/30 21:59:42, miu wrote:
> Instead of 0, can the default be kDefaultMaxVideoKbps (or
> kDefaultAudioEncoderBitrate for audio)?

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:144:
config->channels = ext_params.channels ? *ext_params.channels : 0;
On 2016/06/30 21:59:42, miu wrote:
> Let's default this to 1 for video and 2 for audio. Also, can we validate the
> number (i.e., it must be 1 or 2 for audio, and 1 for video)?

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:145:
config->frequency = ext_params.clock_rate ? *ext_params.clock_rate : 0;
On 2016/06/30 21:59:42, miu wrote:
> Let's default this to kDefaultAudioSamplingRate.

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:150:
config->min_bitrate = config->max_bitrate;
On 2016/06/30 21:59:42, miu wrote:
> also: config->start_bitrate = config->max_bitrate  (see comment in
cast_config.h
> about start_bitrate)

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:170: }
On 2016/06/30 21:59:42, miu wrote:
> Also for Opus: Let's set config->max_frame_rate to 100 (10ms frames) for
> completeness.

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:232: if
(config.codec == media::cast::CODEC_AUDIO_OPUS)
On 2016/06/30 21:59:42, miu wrote:
> With this if...elseif...else statement: I'm thinking it would be more robust
to
> do this instead for safety:
> 
>   switch (config.codec) {
>   case CODEC_AUDIO_OPUS: codec_name = kCodecNameOpus;
>   case CODEC_VIDEO_VP8: codec_name = kCodecNameVp8;
>   case CODEC_VIDEO_H264: codec_name = kCodecNameH264;
>   }
> 
> This would ensure the compiler let's a developer know they have to also change
> this code if they add any codecs to the enum.

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.cc:67: config.min_bitrate =
config.max_bitrate =
On 2016/06/30 21:59:43, miu wrote:
> and start_bitrate (per comment in cast_config.h)

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.cc:78: config.max_playout_delay =
On 2016/06/30 21:59:42, miu wrote:
> You could omit this, since we set this default in the FrameReceiverConfig
ctor.
> (and in DefaultOpusConfig() and DefaultH264Config())

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.cc:85: config.channels = 1;
On 2016/06/30 21:59:43, miu wrote:
> nit: Can you move this to be just before max_bitrate? (so it matches the order
> in the struct; easier for a reader to examine the code to make sure all fields
> are set)

Done.

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
File chrome/renderer/media/cast_rtp_stream.h (left):

https://codereview.chromium.org/2113783002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.h:39: struct CastRtpPayloadParams {
On 2016/06/30 21:59:43, miu wrote:
> Nice! :)

:)

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

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:123: bool use_external_encoder;
On 2016/06/30 21:59:43, miu wrote:
> Let's add a comment like, "If true, use an external encoder rather than the
> built-in software-based one."

Done.

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:125: int frequency;
On 2016/06/30 21:59:43, miu wrote:
> Now might be a good time to rename this to rtp_timebase (like in
> FrameReceiverConfig below).
> 
> Also, feel free to copy the comment from FrameReceiverConfig to here, and also
> for other things like 'channels' and 'codec'.

Done.

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:129: // Audio streams only use |max_bitrate|.
On 2016/06/30 21:59:43, miu wrote:
> Probably not worth mentioning that audio uses only |max_bitrate|. In fact,
maybe
> we should add a TODO in AudioSender::InsertAudioFrame() to allow
> dynamically-adjusting the bitrate based on congestion (just like we do for
> VideoSender)?
> 
> Also, consider moving start_bitrate back to here.

Moved |start_bitrate| back to here.
Added comments for audio streams.

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:133: double max_frame_rate;
On 2016/06/30 21:59:43, miu wrote:
> Nice! Thanks for resolving the TODO! :)

:)

https://codereview.chromium.org/2113783002/diff/1/media/cast/cast_config.h#ne...
media/cast/cast_config.h:144: CodecSpecificParams codec_specific_params;
On 2016/06/30 21:59:43, miu wrote:
> Please rename (for precision). Suggestion:
> 
> s/CodecSpecificParams/VideoCodecParams/
> 
> and
> 
> s/codec_specific_params/video_codec_params/

Done.

https://codereview.chromium.org/2113783002/diff/1/media/cast/sender/fake_soft...
File media/cast/sender/fake_software_video_encoder.cc (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/sender/fake_soft...
media/cast/sender/fake_software_video_encoder.cc:75:
DCHECK_GT(video_config_.max_frame_rate, 0);
On 2016/06/30 21:59:43, miu wrote:
> Seems that this could be in the ctor (to just check once).

Done.

https://codereview.chromium.org/2113783002/diff/1/media/cast/sender/fake_soft...
File media/cast/sender/fake_software_video_encoder.h (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/sender/fake_soft...
media/cast/sender/fake_software_video_encoder.h:31: FrameSenderConfig
video_config_;
On 2016/06/30 21:59:43, miu wrote:
> const please.

Done.

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/cast_benchm...
File media/cast/test/cast_benchmarks.cc (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/cast_benchm...
media/cast/test/cast_benchmarks.cc:262: base::TimeDelta::FromSeconds(1) /
video_sender_config_.max_frame_rate;
On 2016/06/30 21:59:43, miu wrote:
> nit: base::TimeDelta::FromSecondsF(1.0 / video_sender_config_.max_frame_rate);

Done.

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/cast_benchm...
media/cast/test/cast_benchmarks.cc:289: video_sender_config_.max_frame_rate;
On 2016/06/30 21:59:43, miu wrote:
> ditto: base::TimeDelta::FromSecondsF(1.0 /
video_sender_config_.max_frame_rate);

Done.

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/end2end_uni...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/end2end_uni...
media/cast/test/end2end_unittest.cc:493: video_sender_config_.max_frame_rate;
On 2016/06/30 21:59:43, miu wrote:
> Now this is a double being assigned to an int-typed field. Did you want to
> either: 1) Add round-to-nearest int; or 2) Change
> FrameReceiverConfig::target_frame_rate to be a double type (which may or may
not
> trickle into a whole bunch more files)>
> 
> I'll leave it up to you how far you want to go in this change. ;-)

I changed the target_frame_rate to double too.

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/utility/def...
File media/cast/test/utility/default_config.cc (right):

https://codereview.chromium.org/2113783002/diff/1/media/cast/test/utility/def...
media/cast/test/utility/default_config.cc:91:
config.codec_specific_params.number_of_encode_threads = 1;
On 2016/06/30 21:59:43, miu wrote:
> This was "2" before. Did you mean to change it?

Oh, no. Changed it back to 2.

[miu, 2016-07-06 21:20:26]

Looking good. Just a few little things:

https://codereview.chromium.org/2113783002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/2113783002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:394:
ASSERT_FALSE(RunExtensionSubtest("cast_streaming", "rtp_stream_error.html"));
This doesn't look right. Did you mean to adjust something in the javascript code
for this test?

https://codereview.chromium.org/2113783002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/2113783002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:79: constexpr int
kBitrateMultiplier = 1000;
naming nit: How about kBitsPerKilobit to make code that uses this more readable?

https://codereview.chromium.org/2113783002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:255: VLOG(1) <<
"FromFrameSenderConfig: Unsupported codec " << config.codec;
Should this be NOTREACHED() instead of VLOG(1)? It feels like a bug for us to
propose an unsupported codec_name when the JavaScript calls
getSupportedParams().

https://codereview.chromium.org/2113783002/diff/40001/media/cast/receiver/fra...
File media/cast/receiver/frame_receiver.cc (right):

https://codereview.chromium.org/2113783002/diff/40001/media/cast/receiver/fra...
media/cast/receiver/frame_receiver.cc:52: base::TimeDelta::FromSeconds(1.0 /
config.target_frame_rate)),
s/FromSeconds/FromSecondsD/

https://codereview.chromium.org/2113783002/diff/40001/media/cast/test/cast_be...
File media/cast/test/cast_benchmarks.cc (right):

https://codereview.chromium.org/2113783002/diff/40001/media/cast/test/cast_be...
media/cast/test/cast_benchmarks.cc:263: base::TimeDelta::FromSeconds(1.0 /
video_sender_config_.max_frame_rate);
s/FromSeconds/FromSecondsD/

https://codereview.chromium.org/2113783002/diff/40001/media/cast/test/cast_be...
media/cast/test/cast_benchmarks.cc:289: return frame_number *
base::TimeDelta::FromSeconds(
s/FromSeconds/FromSecondsD/

[xjz, 2016-07-06 23:01:55]

Patchset #3 (id:60001) has been deleted

[xjz, 2016-07-06 23:57:38]

xjz@chromium.org changed reviewers:
+ mek@chromium.org

[xjz, 2016-07-06 23:57:39]

Addressed comments. PTAL.

mek@: need owner's approval on cast_streaming_native_handler.cc.

https://codereview.chromium.org/2113783002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/2113783002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:394:
ASSERT_FALSE(RunExtensionSubtest("cast_streaming", "rtp_stream_error.html"));
On 2016/07/06 21:20:26, miu wrote:
> This doesn't look right. Did you mean to adjust something in the javascript
code
> for this test?

Done.

https://codereview.chromium.org/2113783002/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/2113783002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:79: constexpr int
kBitrateMultiplier = 1000;
On 2016/07/06 21:20:26, miu wrote:
> naming nit: How about kBitsPerKilobit to make code that uses this more
readable?

Done.

https://codereview.chromium.org/2113783002/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:255: VLOG(1) <<
"FromFrameSenderConfig: Unsupported codec " << config.codec;
On 2016/07/06 21:20:26, miu wrote:
> Should this be NOTREACHED() instead of VLOG(1)? It feels like a bug for us to
> propose an unsupported codec_name when the JavaScript calls
> getSupportedParams().

Done.

https://codereview.chromium.org/2113783002/diff/40001/media/cast/receiver/fra...
File media/cast/receiver/frame_receiver.cc (right):

https://codereview.chromium.org/2113783002/diff/40001/media/cast/receiver/fra...
media/cast/receiver/frame_receiver.cc:52: base::TimeDelta::FromSeconds(1.0 /
config.target_frame_rate)),
On 2016/07/06 21:20:26, miu wrote:
> s/FromSeconds/FromSecondsD/

Done.

https://codereview.chromium.org/2113783002/diff/40001/media/cast/test/cast_be...
File media/cast/test/cast_benchmarks.cc (right):

https://codereview.chromium.org/2113783002/diff/40001/media/cast/test/cast_be...
media/cast/test/cast_benchmarks.cc:263: base::TimeDelta::FromSeconds(1.0 /
video_sender_config_.max_frame_rate);
On 2016/07/06 21:20:26, miu wrote:
> s/FromSeconds/FromSecondsD/

Done.

https://codereview.chromium.org/2113783002/diff/40001/media/cast/test/cast_be...
media/cast/test/cast_benchmarks.cc:289: return frame_number *
base::TimeDelta::FromSeconds(
On 2016/07/06 21:20:26, miu wrote:
> s/FromSeconds/FromSecondsD/

Done.

[miu, 2016-07-07 04:44:16]

lgtm

[Marijn Kruisselbrink, 2016-07-08 17:03:28]

lgtm, with one question

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js
(right):

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js:50:
chrome.test.succeed();
I don't know enough of this code to really know what's going on here; but does
calling succeed here still give the onError handler a chance to actually assert
that the error callback was called correctly?

[xjz, 2016-07-08 17:11:26]

Thanks for the review!

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js
(right):

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js:50:
chrome.test.succeed();
On 2016/07/08 17:03:28, Marijn Kruisselbrink wrote:
> I don't know enough of this code to really know what's going on here; but does
> calling succeed here still give the onError handler a chance to actually
assert
> that the error callback was called correctly?

No. There will be no error callback by calling succeed. Calling succeed() is
only used for testing purpose, and this test is to verify that with error input,
the rtpStream.start() will now thow an exception (it was handled by error
callback before).

[xjz, 2016-07-08 17:11:45]

The CQ bit was checked by xjz@chromium.org

[commit-bot: I haz the power, 2016-07-08 17:12:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-07-08 17:12:13]

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js
(right):

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js:50:
chrome.test.succeed();
On 2016/07/08 at 17:11:26, xjz wrote:
> On 2016/07/08 17:03:28, Marijn Kruisselbrink wrote:
> > I don't know enough of this code to really know what's going on here; but
does
> > calling succeed here still give the onError handler a chance to actually
assert
> > that the error callback was called correctly?
> 
> No. There will be no error callback by calling succeed. Calling succeed() is
only used for testing purpose, and this test is to verify that with error input,
the rtpStream.start() will now thow an exception (it was handled by error
callback before).

In that case shouldn't you delete the error callback?

[xjz, 2016-07-08 17:18:42]

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js
(right):

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js:50:
chrome.test.succeed();
On 2016/07/08 17:12:13, Marijn Kruisselbrink wrote:
> On 2016/07/08 at 17:11:26, xjz wrote:
> > On 2016/07/08 17:03:28, Marijn Kruisselbrink wrote:
> > > I don't know enough of this code to really know what's going on here; but
> does
> > > calling succeed here still give the onError handler a chance to actually
> assert
> > > that the error callback was called correctly?
> > 
> > No. There will be no error callback by calling succeed. Calling succeed() is
> only used for testing purpose, and this test is to verify that with error
input,
> the rtpStream.start() will now thow an exception (it was handled by error
> callback before).
> 
> In that case shouldn't you delete the error callback?

The error callback is also used in other failures. For example, it will be
called when creating encoder fails. So it can't be deleted. :)

[Marijn Kruisselbrink, 2016-07-08 17:23:23]

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js
(right):

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js:50:
chrome.test.succeed();
On 2016/07/08 at 17:18:42, xjz wrote:
> On 2016/07/08 17:12:13, Marijn Kruisselbrink wrote:
> > On 2016/07/08 at 17:11:26, xjz wrote:
> > > On 2016/07/08 17:03:28, Marijn Kruisselbrink wrote:
> > > > I don't know enough of this code to really know what's going on here;
but
> > does
> > > > calling succeed here still give the onError handler a chance to actually
> > assert
> > > > that the error callback was called correctly?
> > > 
> > > No. There will be no error callback by calling succeed. Calling succeed()
is
> > only used for testing purpose, and this test is to verify that with error
input,
> > the rtpStream.start() will now thow an exception (it was handled by error
> > callback before).
> > 
> > In that case shouldn't you delete the error callback?
> 
> The error callback is also used in other failures. For example, it will be
called when creating encoder fails. So it can't be deleted. :)

I didn't mean get rid of the error event in general. But in this specific test,
you said that onError will never be called. If that is the case, why do you even
have an onError listener in this test?

[xjz, 2016-07-08 17:39:11]

The CQ bit was unchecked by xjz@chromium.org

[xjz, 2016-07-08 17:49:56]

Addressed mek's comment in PS# 4.

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js
(right):

https://codereview.chromium.org/2113783002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cast_streaming/rtp_stream_error.js:50:
chrome.test.succeed();
On 2016/07/08 17:23:23, Marijn Kruisselbrink wrote:
> On 2016/07/08 at 17:18:42, xjz wrote:
> > On 2016/07/08 17:12:13, Marijn Kruisselbrink wrote:
> > > On 2016/07/08 at 17:11:26, xjz wrote:
> > > > On 2016/07/08 17:03:28, Marijn Kruisselbrink wrote:
> > > > > I don't know enough of this code to really know what's going on here;
> but
> > > does
> > > > > calling succeed here still give the onError handler a chance to
actually
> > > assert
> > > > > that the error callback was called correctly?
> > > > 
> > > > No. There will be no error callback by calling succeed. Calling
succeed()
> is
> > > only used for testing purpose, and this test is to verify that with error
> input,
> > > the rtpStream.start() will now thow an exception (it was handled by error
> > > callback before).
> > > 
> > > In that case shouldn't you delete the error callback?
> > 
> > The error callback is also used in other failures. For example, it will be
> called when creating encoder fails. So it can't be deleted. :)
> 
> I didn't mean get rid of the error event in general. But in this specific
test,
> you said that onError will never be called. If that is the case, why do you
even
> have an onError listener in this test?

Yes, you are correct. Thanks for pointing this out. Removed the onError
listener. PTAL PS #4.

[Marijn Kruisselbrink, 2016-07-08 17:56:10]

thanks! lgtm

[xjz, 2016-07-08 18:01:00]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-07-08 18:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2113783002/#ps100001
(title: "Addressed mek's comment.")

[commit-bot: I haz the power, 2016-07-08 18:02:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 20:03:59]

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

[commit-bot: I haz the power, 2016-07-08 20:04:01]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[xjz, 2016-07-08 22:34:52]

The CQ bit was checked by xjz@chromium.org

[commit-bot: I haz the power, 2016-07-08 22:35:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 22:52:11]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-07-08 22:52:20]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 22:53:33]

Description was changed from

==========
Refactoring: Merge VideoSenderConfig and AudioSenderConfig.
In the CL: 1) VideoSenderConfig and AudioSenderConfig are merged to
FrameSenderConfig. 2) CastRtpParams is removed . RtpParams is directly
converted to/from FrameSenderConfig.

BUG=530839
==========

to

==========
Refactoring: Merge VideoSenderConfig and AudioSenderConfig.
In the CL: 1) VideoSenderConfig and AudioSenderConfig are merged to
FrameSenderConfig. 2) CastRtpParams is removed . RtpParams is directly
converted to/from FrameSenderConfig.

BUG=530839

Committed: https://crrev.com/4c97152e0f2b9bf5883d7ea7f149c932598c3afc
Cr-Commit-Position: refs/heads/master@{#404520}
==========

[commit-bot: I haz the power, 2016-07-08 22:53:35]

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

[danakj, 2016-07-08 23:11:39]

A revert of this CL (patchset #4 id:100001) has been created in
https://codereview.chromium.org/2133223003/ by danakj@chromium.org.

The reason for reverting is: [1226/2497] CXX
obj/media/cast/sender/cast_h264_vt_encoder_unittests.h264_vt_encoder_unittest.o
FAILED:
obj/media/cast/sender/cast_h264_vt_encoder_unittests.h264_vt_encoder_unittest.o 
/b/build/slave/cache/cipd/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/media/cast/sender/cast_h264_vt_encoder_unittests.h264_vt_encoder_unittest.o.d
-DV8_DEPRECATION_WARNINGS -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0
-DCHROMIUM_BUILD -DCR_CLANG_REVISION=274369-1 -DCOMPONENT_BUILD
-DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1
-DENABLE_PEPPER_CDMS -DENABLE_NOTIFICATIONS -DUSE_EXTERNAL_POPUP_MENU
-DFIELDTRIAL_TESTING_ENABLED -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1
-DENABLE_PDF=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1
-DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0
-DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DUNIT_TEST -DGTEST_HAS_RTTI=0
-DUSE_LIBPCI=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -Igen -I../..
-I../../third_party/khronos -I../../gpu -I../.. -I../../skia/config -Igen/angle
-Igen/protoc_out -I../../third_party/protobuf/src -I../../testing/gmock/include
-I../../testing/gtest/include
-I../../third_party/ffmpeg/chromium/config/Chromium/mac/x64
-I../../third_party/ffmpeg -isysroot
/b/build/slave/Mac_GYP__dbg_/build/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
-O0 -gdwarf-2 -fvisibility=hidden -Werror -mmacosx-version-min=10.7 -arch x86_64
-Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
-Wno-selector-type-mismatch -Wpartial-availability -Wheader-hygiene
-Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wno-undefined-var-template
-Wno-nonportable-include-path -std=c++11 -stdlib=libc++ -fno-rtti
-fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics
-fcolor-diagnostics -Xclang -load -Xclang
/b/build/slave/Mac_GYP__dbg_/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -Xclang
-plugin-arg-find-bad-constructs -Xclang follow-macro-expansion -Xclang
-plugin-arg-find-bad-constructs -Xclang check-implicit-copy-ctors
-fno-strict-aliasing -fstack-protector-strong  -c
../../media/cast/sender/h264_vt_encoder_unittest.cc -o
obj/media/cast/sender/cast_h264_vt_encoder_unittests.h264_vt_encoder_unittest.o
../../media/cast/sender/h264_vt_encoder_unittest.cc:267:10: error: unknown type
name 'VideoSenderConfig'; did you mean 'VideoDecoderConfig'?
  static VideoSenderConfig video_sender_config_;
         ^~~~~~~~~~~~~~~~~
         VideoDecoderConfig
../../media/base/video_decoder_config.h:26:20: note: 'VideoDecoderConfig'
declared here
class MEDIA_EXPORT VideoDecoderConfig {
                   ^
../../media/cast/sender/h264_vt_encoder_unittest.cc:235:24: error: no matching
constructor for initialization of 'media::cast::H264VideoToolboxEncoder'
    encoder_.reset(new H264VideoToolboxEncoder(
                       ^
../../media/cast/sender/h264_vt_encoder.h:38:3: note: candidate constructor not
viable: no known conversion from 'media::VideoDecoderConfig' to 'const
media::cast::FrameSenderConfig' for 2nd argument
  H264VideoToolboxEncoder(
  ^
../../media/cast/sender/h264_vt_encoder.h:125:28: note: candidate constructor
not viable: requires 1 argument, but 3 were provided
  DISALLOW_COPY_AND_ASSIGN(H264VideoToolboxEncoder);
                           ^
../../media/cast/sender/h264_vt_encoder_unittest.cc:256:26: error: no viable
overloaded '='
    video_sender_config_ = GetDefaultVideoSenderConfig();
    ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../media/base/video_decoder_config.h:26:20: note: candidate function (the
implicit copy assignment operator) not viable: no known conversion from
'media::cast::FrameSenderConfig' to 'const media::VideoDecoderConfig' for 1st
argument
class MEDIA_EXPORT VideoDecoderConfig {
                   ^
../../media/cast/sender/h264_vt_encoder_unittest.cc:257:26: error: reference to
non-static member function must be called; did you mean to call it with no
arguments?
    video_sender_config_.codec = CODEC_VIDEO_H264;
    ~~~~~~~~~~~~~~~~~~~~~^~~~~
                              ()
../../media/cast/sender/h264_vt_encoder_unittest.cc:257:32: error: expression is
not assignable
    video_sender_config_.codec = CODEC_VIDEO_H264;
    ~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
../../media/cast/sender/h264_vt_encoder_unittest.cc:283:1: error: unknown type
name 'VideoSenderConfig'; did you mean 'VideoDecoderConfig'?
VideoSenderConfig H264VideoToolboxEncoderTest::video_sender_config_;
^~~~~~~~~~~~~~~~~
VideoDecoderConfig
../../media/base/video_decoder_config.h:26:20: note: 'VideoDecoderConfig'
declared here
class MEDIA_EXPORT VideoDecoderConfig {
                   ^
6 errors generated.
.