Refactoring: Use enum for RtpPayloadType.

chrome/renderer/extensions/cast_streaming_native_handler.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/extensions/cast_streaming_native_handler.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 46 matching lines @@
 const char kInvalidAesIvMask[] = "Invalid value for AES IV mask";
 const char kInvalidAesKey[] = "Invalid value for AES key";
 const char kInvalidAudioParams[] = "Invalid audio params";
 const char kInvalidDestination[] = "Invalid destination";
 const char kInvalidFPS[] = "Invalid FPS";
 const char kInvalidMediaStreamURL[] = "Invalid MediaStream URL";
 const char kInvalidRtpParams[] = "Invalid value for RTP params";
 const char kInvalidLatency[] = "Invalid value for max_latency. (0-1000)";
 const char kInvalidRtpTimebase[] = "Invalid rtp_timebase. (1000-1000000)";
 const char kInvalidStreamArgs[] = "Invalid stream arguments";
+const char kInvalidCodec[] = "Invalid codec";
 const char kRtpStreamNotFound[] = "The RTP stream cannot be found";
 const char kUdpTransportNotFound[] = "The UDP transport cannot be found";
 const char kUnableToConvertArgs[] = "Unable to convert arguments";
 const char kUnableToConvertParams[] = "Unable to convert params";
 
 // These helper methods are used to convert between Extension API
 // types and Cast types.
 void ToCastCodecSpecificParams(const CodecSpecificParams& ext_params,
                                CastCodecSpecificParams* cast_params) {
   cast_params->key = ext_params.key;
@@ skipping 12 matching lines @@
   if (!base::HexStringToBytes(input, &bytes))
     return false;
   output->assign(reinterpret_cast<const char*>(&bytes[0]), bytes.size());
   return true;
 }
 }  // namespace
 
 bool ToCastRtpPayloadParamsOrThrow(v8::Isolate* isolate,
                                    const RtpPayloadParams& ext_params,
                                    CastRtpPayloadParams* cast_params) {
-  cast_params->payload_type = ext_params.payload_type;
   cast_params->max_latency_ms = ext_params.max_latency;
   cast_params->min_latency_ms =
       ext_params.min_latency ? *ext_params.min_latency : ext_params.max_latency;
   cast_params->animated_latency_ms = ext_params.animated_latency
                                          ? *ext_params.animated_latency
                                          : ext_params.max_latency;
   cast_params->codec_name = ext_params.codec_name;
+  if (cast_params->codec_name == "OPUS") {
+    cast_params->payload_type = media::cast::RtpPayloadType::AUDIO_OPUS;
+  } else if (cast_params->codec_name == "PCM16") {
+    cast_params->payload_type = media::cast::RtpPayloadType::AUDIO_PCM16;
+  } else if (cast_params->codec_name == "AAC") {
+    cast_params->payload_type = media::cast::RtpPayloadType::AUDIO_AAC;
+  } else if (cast_params->codec_name == "VP8") {
+    cast_params->payload_type = media::cast::RtpPayloadType::VIDEO_VP8;
+  } else if (cast_params->codec_name == "H264") {
+    cast_params->payload_type = media::cast::RtpPayloadType::VIDEO_H264;
+  } else {
+    isolate->ThrowException(

[Marijn Kruisselbrink, 2016/06/24 16:54:57]

One of the now failing tests seems to be a test to exactly test the behavior
when an invalid codec name was specified. Where the test expects the error
callback to be called, but now you changed that to throw an exception instead. I
don't know if making such breaking API changes is a compatibility issue. And if
"codec_name" has a limited set of hard-coded values that is acceptable, would it
make sense for it to be an enum in the idl files, rather than just an arbitrary
string?

[miu, 2016/06/27 21:00:36]

FWIW, I have a change WIP to clean-up and simplify all these Cast Streaming
APIs. We won't have to worry about backwards-compatibility because they are
private and only used by a few whitelisted extensions our team owns.

I talked about this with xjz@ face-to-face and, for this change, we'll just go
back to the old behavior for now (i.e., the onError event handler will trigger
on a bad codecName, as the API test expects). In my API clean-up change, I will
be using enums instead of free-form string fields for this property; and will do
strict, immediate input validation (rather than delayed via an error handler).

[xjz, 2016/06/28 16:46:26]

Thanks Yuri for the reply. Now revert the CL to Patch #2, which does not handle
the invalid |codec_name| here.

+        v8::Exception::Error(v8::String::NewFromUtf8(isolate, kInvalidCodec)));
+    return false;
+  }
   cast_params->ssrc = ext_params.ssrc;
   cast_params->feedback_ssrc = ext_params.feedback_ssrc;
   cast_params->clock_rate = ext_params.clock_rate ? *ext_params.clock_rate : 0;
   cast_params->min_bitrate =
       ext_params.min_bitrate ? *ext_params.min_bitrate : 0;
   cast_params->max_bitrate =
       ext_params.max_bitrate ? *ext_params.max_bitrate : 0;
   cast_params->channels = ext_params.channels ? *ext_params.channels : 0;
   cast_params->max_frame_rate =
       ext_params.max_frame_rate ? *ext_params.max_frame_rate : 0.0;
@@ skipping 13 matching lines @@
     CastCodecSpecificParams cast_codec_params;
     ToCastCodecSpecificParams(ext_params.codec_specific_params[i],
                               &cast_codec_params);
     cast_params->codec_specific_params.push_back(cast_codec_params);
   }
   return true;
 }
 
 void FromCastRtpPayloadParams(const CastRtpPayloadParams& cast_params,
                               RtpPayloadParams* ext_params) {
-  ext_params->payload_type = cast_params.payload_type;
+  ext_params->payload_type = static_cast<int>(cast_params.payload_type);
   ext_params->max_latency = cast_params.max_latency_ms;
   ext_params->min_latency.reset(new int(cast_params.min_latency_ms));
   ext_params->animated_latency.reset(new int(cast_params.animated_latency_ms));
   ext_params->codec_name = cast_params.codec_name;
   ext_params->ssrc = cast_params.ssrc;
   ext_params->feedback_ssrc = cast_params.feedback_ssrc;
   if (cast_params.clock_rate)
     ext_params->clock_rate.reset(new int(cast_params.clock_rate));
   if (cast_params.min_bitrate)
     ext_params->min_bitrate.reset(new int(cast_params.min_bitrate));
@@ skipping 518 matching lines @@
   config->rtp_max_delay_ms = params->max_latency;
   if (config->rtp_max_delay_ms < 0 || config->rtp_max_delay_ms > 1000) {
     isolate->ThrowException(v8::Exception::TypeError(
         v8::String::NewFromUtf8(isolate, kInvalidLatency)));
     return false;
   }
   config->channels = 2;
   if (params->codec_name == "OPUS") {
     config->codec = media::cast::CODEC_AUDIO_OPUS;
     config->rtp_timebase = 48000;
-    config->rtp_payload_type = media::cast::kDefaultRtpAudioPayloadType;
+    config->rtp_payload_type = media::cast::RtpPayloadType::AUDIO_OPUS;
   } else if (params->codec_name == "PCM16") {
     config->codec = media::cast::CODEC_AUDIO_PCM16;
     config->rtp_timebase = 48000;
-    config->rtp_payload_type = media::cast::kDefaultRtpAudioPayloadType;
+    config->rtp_payload_type = media::cast::RtpPayloadType::AUDIO_PCM16;
   } else if (params->codec_name == "AAC") {
     config->codec = media::cast::CODEC_AUDIO_AAC;
     config->rtp_timebase = 48000;
-    config->rtp_payload_type = media::cast::kDefaultRtpAudioPayloadType;
+    config->rtp_payload_type = media::cast::RtpPayloadType::AUDIO_AAC;
   } else if (params->codec_name == "VP8") {
     config->codec = media::cast::CODEC_VIDEO_VP8;
     config->rtp_timebase = 90000;
-    config->rtp_payload_type = media::cast::kDefaultRtpVideoPayloadType;
+    config->rtp_payload_type = media::cast::RtpPayloadType::VIDEO_VP8;
   } else if (params->codec_name == "H264") {
     config->codec = media::cast::CODEC_VIDEO_H264;
     config->rtp_timebase = 90000;
-    config->rtp_payload_type = media::cast::kDefaultRtpVideoPayloadType;
+    config->rtp_payload_type = media::cast::RtpPayloadType::VIDEO_H264;
+  } else {
+    isolate->ThrowException(
+        v8::Exception::Error(v8::String::NewFromUtf8(isolate, kInvalidCodec)));
+    return false;
   }
   if (params->rtp_timebase) {
     config->rtp_timebase = *params->rtp_timebase;
     if (config->rtp_timebase < 1000 || config->rtp_timebase > 1000000) {
       isolate->ThrowException(v8::Exception::TypeError(
           v8::String::NewFromUtf8(isolate, kInvalidRtpTimebase)));
       return false;
     }
   }
   if (params->aes_key &&
@@ skipping 171 matching lines @@
     LOG(ERROR) << "Failed to add Cast audio track to media stream.";
   }
   if (!content::AddVideoTrackToMediaStream(std::move(video), true,  // is_remote
                                            true,  // is_readonly
                                            &web_stream)) {
     LOG(ERROR) << "Failed to add Cast video track to media stream.";
   }
 }
 
 }  // namespace extensions