Refactoring: Use enum for RtpPayloadType.

Use enum for RtpPayloadType.
Change |rtp_payload_type| to enum to specify both the type
(video/audio) and encoding codec of the RTP streaming.

BUG=620445
Committed: https://crrev.com/1a96dfbda016edeb50e631e20fb3cd28bda49908
Cr-Commit-Position: refs/heads/master@{#403490}

[xjz, 2016-06-15 22:44:03]

Description was changed from

==========
Use enum for RtpPayloadType.
Change |rtp_payload_type| to enum to specify both the type
(video/audio) and encoding codec of the RTP streaming.

BUG=620445
==========

to

==========
Use enum for RtpPayloadType.
Change |rtp_payload_type| to enum to specify both the type
(video/audio) and encoding codec of the RTP streaming.

BUG=620445
==========

[xjz, 2016-06-15 22:44:04]

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

[xjz, 2016-06-20 17:05:17]

PTAL

[miu, 2016-06-20 21:00:27]

https://codereview.chromium.org/2068133005/diff/1/chrome/common/cast_messages.h
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/2068133005/diff/1/chrome/common/cast_messages...
chrome/common/cast_messages.h:64:
IPC_ENUM_TRAITS_MAX_VALUE(media::cast::RtpPayloadType,
Because the minimum is non-zero:

  IPC_ENUM_TRAITS_MIN_MAX_VALUE(media::cast::RtpPayloadType,
                                media::cast::RtpPayloadType::FIRST,
                                media::cast::RtpPayloadType::LAST);

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

https://codereview.chromium.org/2068133005/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:99:
DCHECK_GE(ext_params.payload_type, media::cast::RTP_PAYLOAD_AUDIO_OPUS);
The process shouldn't crash on invalid inputs. Instead, the method should return
false.

Suggest you add a RtpPlayloadType::FIRST and LAST to the enum, then just test
the user's input with:

  if (ext_params.payload_type < FIRST || > LAST) {
    isolate->ThrowException(v8::Exception::Error(
        v8::String::NewFromUtf8(isolate, kInvalid...)));
    return false;
  }
  cast_params->payload_type = static_cast<...>(...);

https://codereview.chromium.org/2068133005/diff/1/media/cast/net/cast_transpo...
File media/cast/net/cast_transport_config.h (right):

https://codereview.chromium.org/2068133005/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_config.h:34: enum RtpPayloadType {
nit style suggestion: Use an enum class:

  enum class RtpPayloadType : int {
    UNKNOWN,
    AUDIO_OPUS = 96,
    AUDIO_AAC,
    ...
  };

https://codereview.chromium.org/2068133005/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_config.h:39: RTP_PAYLOAD_AUDIO_OPUS = 96,
If we add new ones in the future, the numbering might become messed up (if any
code depends on it). You might just assign an explicit value to each of these
(97 for AAC, 98 for PCM16, and so on).

[xjz, 2016-06-21 17:24:31]

Addressed comments. PTAL.

https://codereview.chromium.org/2068133005/diff/1/chrome/common/cast_messages.h
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/2068133005/diff/1/chrome/common/cast_messages...
chrome/common/cast_messages.h:64:
IPC_ENUM_TRAITS_MAX_VALUE(media::cast::RtpPayloadType,
On 2016/06/20 21:00:27, miu wrote:
> Because the minimum is non-zero:
> 
>   IPC_ENUM_TRAITS_MIN_MAX_VALUE(media::cast::RtpPayloadType,
>                                 media::cast::RtpPayloadType::FIRST,
>                                 media::cast::RtpPayloadType::LAST);

Done.

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

https://codereview.chromium.org/2068133005/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:99:
DCHECK_GE(ext_params.payload_type, media::cast::RTP_PAYLOAD_AUDIO_OPUS);
On 2016/06/20 21:00:27, miu wrote:
> The process shouldn't crash on invalid inputs. Instead, the method should
return
> false.
> 
> Suggest you add a RtpPlayloadType::FIRST and LAST to the enum, then just test
> the user's input with:
> 
>   if (ext_params.payload_type < FIRST || > LAST) {
>     isolate->ThrowException(v8::Exception::Error(
>         v8::String::NewFromUtf8(isolate, kInvalid...)));
>     return false;
>   }
>   cast_params->payload_type = static_cast<...>(...);

Ignored |ext_params.payload_type| and set the |cast_params->payload_type|
according to |codec_name| instead.

https://codereview.chromium.org/2068133005/diff/1/media/cast/net/cast_transpo...
File media/cast/net/cast_transport_config.h (right):

https://codereview.chromium.org/2068133005/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_config.h:34: enum RtpPayloadType {
On 2016/06/20 21:00:27, miu wrote:
> nit style suggestion: Use an enum class:
> 
>   enum class RtpPayloadType : int {
>     UNKNOWN,
>     AUDIO_OPUS = 96,
>     AUDIO_AAC,
>     ...
>   };

Done.

https://codereview.chromium.org/2068133005/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_config.h:39: RTP_PAYLOAD_AUDIO_OPUS = 96,
On 2016/06/20 21:00:27, miu wrote:
> If we add new ones in the future, the numbering might become messed up (if any
> code depends on it). You might just assign an explicit value to each of these
> (97 for AAC, 98 for PCM16, and so on).

Done.

[xjz, 2016-06-21 20:52:54]

Patchset #2 (id:20001) has been deleted

[miu, 2016-06-23 23:19:03]

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

https://codereview.chromium.org/2068133005/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:116: }
nit: Please add a fail clause for an unrecognized codec:

  } else {
    isolate->ThrowException(v8::Exception::Error(
        v8::String::NewFromUtf8(isolate, kInvalidCodec)));
    return false;
  }

https://codereview.chromium.org/2068133005/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:150:
ext_params->payload_type = static_cast<int>(cast_params.payload_type);
Is the static_cast<> still needed if you add " : int" to where the enum is
declared:

  enum class RtpPayloadParams : int {
    ...

https://codereview.chromium.org/2068133005/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:706: }
ditto: Please add fail clause, like above.

https://codereview.chromium.org/2068133005/diff/40001/media/cast/net/rtp/rtp_...
File media/cast/net/rtp/rtp_sender.cc (right):

https://codereview.chromium.org/2068133005/diff/40001/media/cast/net/rtp/rtp_...
media/cast/net/rtp/rtp_sender.cc:42: config_.payload_type =
static_cast<int>(config.rtp_payload_type);
ditto here: Is static_cast<> needed?

https://codereview.chromium.org/2068133005/diff/40001/media/cast/test/receive...
File media/cast/test/receiver.cc (right):

https://codereview.chromium.org/2068133005/diff/40001/media/cast/test/receive...
media/cast/test/receiver.cc:127:
std::to_string(static_cast<int>(RtpPayloadType::AUDIO_OPUS)),
ditto here: Is static_cast<> needed?

https://codereview.chromium.org/2068133005/diff/40001/media/cast/test/receive...
media/cast/test/receiver.cc:145:
std::to_string(static_cast<int>(RtpPayloadType::VIDEO_VP8)),
ditto here: Is static_cast<> needed?

[xjz, 2016-06-24 01:12:41]

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

[xjz, 2016-06-24 01:18:24]

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

[xjz, 2016-06-24 01:22:45]

PTAL

mek@: PTAL change in cast_streaming_native_handler.cc.

dcheng@: PTAL change in cast_messages.h.

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

https://codereview.chromium.org/2068133005/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:116: }
On 2016/06/23 23:19:03, miu wrote:
> nit: Please add a fail clause for an unrecognized codec:
> 
>   } else {
>     isolate->ThrowException(v8::Exception::Error(
>         v8::String::NewFromUtf8(isolate, kInvalidCodec)));
>     return false;
>   }

Done.

https://codereview.chromium.org/2068133005/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:150:
ext_params->payload_type = static_cast<int>(cast_params.payload_type);
On 2016/06/23 23:19:03, miu wrote:
> Is the static_cast<> still needed if you add " : int" to where the enum is
> declared:
> 
>   enum class RtpPayloadParams : int {
>     ...

Yes, static_cast<int> is still needed even if I add ": int" to enum class
RtpPayloadType. Otherwise causes compiling error. Actually, by default the type
should be "int" if not explicitly declared.

https://codereview.chromium.org/2068133005/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:706: }
On 2016/06/23 23:19:03, miu wrote:
> ditto: Please add fail clause, like above.

Done.

https://codereview.chromium.org/2068133005/diff/40001/media/cast/net/rtp/rtp_...
File media/cast/net/rtp/rtp_sender.cc (right):

https://codereview.chromium.org/2068133005/diff/40001/media/cast/net/rtp/rtp_...
media/cast/net/rtp/rtp_sender.cc:42: config_.payload_type =
static_cast<int>(config.rtp_payload_type);
On 2016/06/23 23:19:03, miu wrote:
> ditto here: Is static_cast<> needed?

Yes, it is needed, as replied in another comment.

https://codereview.chromium.org/2068133005/diff/40001/media/cast/test/receive...
File media/cast/test/receiver.cc (right):

https://codereview.chromium.org/2068133005/diff/40001/media/cast/test/receive...
media/cast/test/receiver.cc:127:
std::to_string(static_cast<int>(RtpPayloadType::AUDIO_OPUS)),
On 2016/06/23 23:19:03, miu wrote:
> ditto here: Is static_cast<> needed?

Yes, it is needed, as replied in another comment.

https://codereview.chromium.org/2068133005/diff/40001/media/cast/test/receive...
media/cast/test/receiver.cc:145:
std::to_string(static_cast<int>(RtpPayloadType::VIDEO_VP8)),
On 2016/06/23 23:19:03, miu wrote:
> ditto here: Is static_cast<> needed?

Yes, it is needed, as replied in another comment.

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

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

https://codereview.chromium.org/2068133005/diff/60001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:118:
isolate->ThrowException(
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]

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

https://codereview.chromium.org/2068133005/diff/60001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:118:
isolate->ThrowException(
On 2016/06/24 16:54:57, Marijn Kruisselbrink wrote:
> 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?

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]

Addressed comments. PTAL.

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

https://codereview.chromium.org/2068133005/diff/60001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:118:
isolate->ThrowException(
On 2016/06/27 21:00:36, miu wrote:
> On 2016/06/24 16:54:57, Marijn Kruisselbrink wrote:
> > 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?
> 
> 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).

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

[miu, 2016-06-28 17:52:12]

lgtm

[xjz, 2016-06-30 18:02:10]

mek@ and dcheng@: LGTY?

[Marijn Kruisselbrink, 2016-06-30 18:05:57]

lgtm

[dcheng, 2016-07-01 12:31:58]

ipc lgtm

[xjz, 2016-07-01 17:09:21]

The CQ bit was checked by xjz@chromium.org

[commit-bot: I haz the power, 2016-07-01 17:09:45]

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-01 18:41:29]

Description was changed from

==========
Use enum for RtpPayloadType.
Change |rtp_payload_type| to enum to specify both the type
(video/audio) and encoding codec of the RTP streaming.

BUG=620445
==========

to

==========
Use enum for RtpPayloadType.
Change |rtp_payload_type| to enum to specify both the type
(video/audio) and encoding codec of the RTP streaming.

BUG=620445
==========

[commit-bot: I haz the power, 2016-07-01 18:41:31]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-07-01 18:43:46]

Description was changed from

==========
Use enum for RtpPayloadType.
Change |rtp_payload_type| to enum to specify both the type
(video/audio) and encoding codec of the RTP streaming.

BUG=620445
==========

to

==========
Use enum for RtpPayloadType.
Change |rtp_payload_type| to enum to specify both the type
(video/audio) and encoding codec of the RTP streaming.

BUG=620445

Committed: https://crrev.com/1a96dfbda016edeb50e631e20fb3cd28bda49908
Cr-Commit-Position: refs/heads/master@{#403490}
==========

[commit-bot: I haz the power, 2016-07-01 18:43:48]

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