Cast: Javascript bindings for cast receiver

Cast: Javascript bindings for cast receiver

This adds javascript bindings for the functions in cast_receiver_session, allowing
a cast_receiver to be initiated from a whitelisted javascript extension.

Also fixes a crashing bug in udp_transport.cc.

Committed: https://crrev.com/4449d976059b4d76ca1671560042f291f9a6083d
Cr-Commit-Position: refs/heads/master@{#319175}

[hubbe, 2015-02-20 20:30:36]

hubbe@chromium.org changed reviewers:
+ hclam@chromium.org

[hubbe, 2015-02-20 20:30:36]

Doesn't compile yet since it depends on
https://codereview.chromium.org/883293005/, but works locally, so it's probably
ok to start reviewing.

[Alpha Left Google, 2015-02-25 20:37:28]

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:6: // receiver
session and returns a WebMediaStream.
The term WebMediaStraem is an implementation detail. The client of this JS API
knows only MediaStream.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:49:
CodecSpecificParams[] codecSpecificParams;
Since this is in the receiver there is no codec params: the streams are already
encoded. There might be playback options like error concealment, etc. This
should be in a different name or removed.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:62: //
|localEndpoint| : Set this to bind() to a local UDP port.
I'm not sure how these two parameters are used.

It seems this receiver session can operate in two modes:
1. Server mode with a given port number. What if the port is already used? Can
it bind to a random port?
2. Client mode that connects to a sender. How is this possible with cast sender?

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:64: // |height|
: Video height
Shouldn't these come from the video frames?

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:66: //
|maxFrameRate| : Max video frame rate.
This seems unnecessary. Since this is the max value, can just be 60 internally.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:68: //
|options| : Optional settings.
What are these options? Can you put them in RtpReceiverParams instead?

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:69: [nocompile]
static void create(
Should there be a callback when create is done? So user can add the MediaStream
to a <video> tag? Or the MediaStream can be used immediately after this call?

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:722:
v8::String::NewFromUtf8(args.GetIsolate(), "ARG!!!")));
Should be a more user friendly string.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:767:
video_config.target_frame_rate = fps;
I think the fps parameter in VideoCaptureFormat has little value. You can simply
have a fixed value here.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:775:
audio_config.rtp_timebase / 100);  // frames per buffer,
Divide by audio_config.target_frame_rate instead.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:824: 
nit: remove this extra empty line.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/cast_streaming_native_handler.h (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.h:89: void
CallReceiverCB(
Please be consistent with the rest of this file, i.e. CallReceiverCallback.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.h:104: bool
FrameReceiverFromArg(
Should be FrameReceiverConfigFromArg.

https://codereview.chromium.org/938903003/diff/20001/content/public/renderer/...
File content/public/renderer/media_stream_api.h (right):

https://codereview.chromium.org/938903003/diff/20001/content/public/renderer/...
content/public/renderer/media_stream_api.h:26: //               constraints.
What do you mean by constraints here? MediaStreamConstraints?
MediaTrackConstraints?

https://codereview.chromium.org/938903003/diff/20001/media/cast/net/udp_trans...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/938903003/diff/20001/media/cast/net/udp_trans...
media/cast/net/udp_transport.cc:78: udp_socket_.reset();
Why changes in this file?

[miu, 2015-02-26 06:23:33]

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

[miu, 2015-02-26 06:23:34]

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:37: long?
clockRate;
Do we want clockRate at all?  For audio, IMO, it makes more sense to
automatically use the platform's "native" output sampling rate (there is code I
can point you to find this easily).  For video, clockRate has to be 90000, and
it really doesn't make sense to allow this to be customized.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:40: long?
channels;
ditto (same issue as clockRate)

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/DEPS#new...
chrome/renderer/DEPS:35: "+media/audio",
Did you mean to keep this in here?  If so, what file requires this new
dependency?  Consider creating a DEPS file in the inner-most dir.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:643:
config->rtp_max_delay_ms = params->max_latency;
Please range-check this value, and throw a v8 exception if out-of-range.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:662:
config->rtp_timebase = params->clock_rate ? *params->clock_rate : 0;
This overwrites the previous assignments of config->rtp_timebase above this
line.  However, per my earlier comment, consider NOT setting rtp_timebase from
JS-provided config.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:739: return;
Throw error exception here?

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:767:
video_config.target_frame_rate = fps;
Doesn't a Cast Receiver have NO concept of frame rate?  I think it's supposed to
play back whatever frames it gets at the playout time.  Also, width/height
should be determined per-frame, right?  Is there some way you can start a
session without specifying a VideoCaptureFormat?

If you need to use VideoCaptureFormat, I suggest specifying
MediaStreamVideoSource::kUnknownFrameRate for the FPS.

[hubbe, 2015-02-28 00:17:12]

PTAL

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:6: // receiver
session and returns a WebMediaStream.
On 2015/02/25 20:37:27, Alpha wrote:
> The term WebMediaStraem is an implementation detail. The client of this JS API
> knows only MediaStream.

Rewrote the comment since it's not accurate anymore.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:37: long?
clockRate;
On 2015/02/26 06:23:34, miu wrote:
> Do we want clockRate at all?  For audio, IMO, it makes more sense to
> automatically use the platform's "native" output sampling rate (there is code
I
> can point you to find this easily).  For video, clockRate has to be 90000, and
> it really doesn't make sense to allow this to be customized.

I think clockRate should always be 48000 or 90000, so I'm removing this.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:40: long?
channels;
On 2015/02/26 06:23:34, miu wrote:
> ditto (same issue as clockRate)

Hardcoding to 2.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:49:
CodecSpecificParams[] codecSpecificParams;
On 2015/02/25 20:37:27, Alpha wrote:
> Since this is in the receiver there is no codec params: the streams are
already
> encoded. There might be playback options like error concealment, etc. This
> should be in a different name or removed.

Removed.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:62: //
|localEndpoint| : Set this to bind() to a local UDP port.
On 2015/02/25 20:37:27, Alpha wrote:
> I'm not sure how these two parameters are used.
> 
> It seems this receiver session can operate in two modes:
> 1. Server mode with a given port number. What if the port is already used? Can
> it bind to a random port?

It can bind to a random port, but there is currently no way to find out what
that port is...
This API needs an error reporting method, so I probably need to add a callback

> 2. Client mode that connects to a sender. How is this possible with cast
sender?

I included this mode even though it's not compatible with the current sender to
make it possible to avoid firewall dialogs in some cases.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:64: // |height|
: Video height
On 2015/02/25 20:37:27, Alpha wrote:
> Shouldn't these come from the video frames? 

I need it construct the media stream.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:66: //
|maxFrameRate| : Max video frame rate.
On 2015/02/25 20:37:27, Alpha wrote:
> This seems unnecessary. Since this is the max value, can just be 60
internally.

60 might be ok, I use it construct the media stream and I'm not sure how it's
used in the media stream framework, so it seems safer to make it changeable.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:68: //
|options| : Optional settings.
On 2015/02/25 20:37:27, Alpha wrote:
> What are these options? Can you put them in RtpReceiverParams instead?

They are transport options, renamed.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:69: [nocompile]
static void create(
On 2015/02/25 20:37:27, Alpha wrote:
> Should there be a callback when create is done? So user can add the
MediaStream
> to a <video> tag? Or the MediaStream can be used immediately after this call?

A callback is not needed if everything is successful. The media stream can be
used immediately after (or before) this call and you can always add onTrackAdded
(or whatever it is called) to get a callback when the audio & video tracks are
added.

However, we do need some sort of status callback for when things are not
successful.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/DEPS#new...
chrome/renderer/DEPS:35: "+media/audio",
On 2015/02/26 06:23:34, miu wrote:
> Did you mean to keep this in here?  If so, what file requires this new
> dependency?  Consider creating a DEPS file in the inner-most dir.

Moved to inner level.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:643:
config->rtp_max_delay_ms = params->max_latency;
On 2015/02/26 06:23:34, miu wrote:
> Please range-check this value, and throw a v8 exception if out-of-range.

Done.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:662:
config->rtp_timebase = params->clock_rate ? *params->clock_rate : 0;
On 2015/02/26 06:23:34, miu wrote:
> This overwrites the previous assignments of config->rtp_timebase above this
> line.  However, per my earlier comment, consider NOT setting rtp_timebase from
> JS-provided config.

Done.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:722:
v8::String::NewFromUtf8(args.GetIsolate(), "ARG!!!")));
On 2015/02/25 20:37:28, Alpha wrote:
> Should be a more user friendly string.

OOps, fixed.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:739: return;
On 2015/02/26 06:23:34, miu wrote:
> Throw error exception here?

An exception will have already been raised in this case.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:767:
video_config.target_frame_rate = fps;
On 2015/02/26 06:23:34, miu wrote:
> Doesn't a Cast Receiver have NO concept of frame rate?  I think it's supposed
to
> play back whatever frames it gets at the playout time.  Also, width/height
> should be determined per-frame, right?  Is there some way you can start a
> session without specifying a VideoCaptureFormat?
> 
> If you need to use VideoCaptureFormat, I suggest specifying
> MediaStreamVideoSource::kUnknownFrameRate for the FPS.

Unfortunately capture_format.IsValid() must be true, or the media stream
framework bork.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:767:
video_config.target_frame_rate = fps;
On 2015/02/25 20:37:28, Alpha wrote:
> I think the fps parameter in VideoCaptureFormat has little value. You can
simply
> have a fixed value here.

That might be true, but I don't actually know how this value is used, so it
seems safer to put in a "real" value here.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:775:
audio_config.rtp_timebase / 100);  // frames per buffer,
On 2015/02/25 20:37:28, Alpha wrote:
> Divide by audio_config.target_frame_rate instead.

Done.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:824: 
On 2015/02/25 20:37:28, Alpha wrote:
> nit: remove this extra empty line.

Done.

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/cast_streaming_native_handler.h (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.h:89: void
CallReceiverCB(
On 2015/02/25 20:37:28, Alpha wrote:
> Please be consistent with the rest of this file, i.e. CallReceiverCallback. 

Renamed to AddTracksToMediaStream

https://codereview.chromium.org/938903003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.h:104: bool
FrameReceiverFromArg(
On 2015/02/25 20:37:28, Alpha wrote:
> Should be FrameReceiverConfigFromArg.

Done.

https://codereview.chromium.org/938903003/diff/20001/content/public/renderer/...
File content/public/renderer/media_stream_api.h (right):

https://codereview.chromium.org/938903003/diff/20001/content/public/renderer/...
content/public/renderer/media_stream_api.h:26: //               constraints.
On 2015/02/25 20:37:28, Alpha wrote:
> What do you mean by constraints here? MediaStreamConstraints?
> MediaTrackConstraints?

MediaTrackConstraints (updated)

https://codereview.chromium.org/938903003/diff/20001/media/cast/net/udp_trans...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/938903003/diff/20001/media/cast/net/udp_trans...
media/cast/net/udp_transport.cc:78: udp_socket_.reset();
On 2015/02/25 20:37:28, Alpha wrote:
> Why changes in this file?

Moved to separate cl.

[miu, 2015-02-28 04:32:58]

https://codereview.chromium.org/938903003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://codereview.chromium.org/938903003/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:66: long
height,
Rename to maxWidth/maxHeight, if these are needed for MediaStream.

Also, consider ordering the args as width then height to be consistent with
other APIs.

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

https://codereview.chromium.org/938903003/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:656:
config->rtp_timebase = 48000;
Hmm...Taking a closer look at FrameReceiver, it seems this value has to match
whatever the sender is using.  Otherwise, the time calculations involving the
frame RTP timestamps won't work.  Perhaps it does need to be passed-in via the
JS API after all?

I'm wonder if (later CL), we should add a separate sample_rate config property,
passed to AudioDecoder.  We should be asking Opus to decode using the system's
native output sampling rate to avoid unnecessary resampling.

  media::AudioHardwareConfig* const hardware_config =
      RenderThreadImpl::current()->GetAudioHardwareConfig();
  config->sample_rate = hardware_config->GetOutputSampleRate();

[hubbe, 2015-03-03 01:23:03]

PTAL

https://codereview.chromium.org/938903003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://codereview.chromium.org/938903003/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:66: long
height,
On 2015/02/28 04:32:58, miu wrote:
> Rename to maxWidth/maxHeight, if these are needed for MediaStream.
> 
> Also, consider ordering the args as width then height to be consistent with
> other APIs.

Done.

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

https://codereview.chromium.org/938903003/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/cast_streaming_native_handler.cc:656:
config->rtp_timebase = 48000;
On 2015/02/28 04:32:58, miu wrote:
> Hmm...Taking a closer look at FrameReceiver, it seems this value has to match
> whatever the sender is using.  Otherwise, the time calculations involving the
> frame RTP timestamps won't work.  Perhaps it does need to be passed-in via the
> JS API after all?
> 

Made it optional.

> I'm wonder if (later CL), we should add a separate sample_rate config
property,
> passed to AudioDecoder.  We should be asking Opus to decode using the system's
> native output sampling rate to avoid unnecessary resampling.
> 
>   media::AudioHardwareConfig* const hardware_config =
>       RenderThreadImpl::current()->GetAudioHardwareConfig();
>   config->sample_rate = hardware_config->GetOutputSampleRate();

File a bug.

[miu, 2015-03-03 04:36:50]

lgtm

[hubbe, 2015-03-03 18:46:27]

hubbe@chromium.org changed reviewers:
+ jam@chromium.org

[hubbe, 2015-03-03 18:46:28]

Ok, time for OWNERS review, jam, if you would?

I'll need your approval for these files, but feel free to look at all of them:

  chrome/common/extensions/api/_api_features.json
  chrome/common/extensions/api/cast_streaming_receiver_session.idl
  chrome/common/extensions/api/schemas.gypi
  chrome/renderer/extensions/DEPS
  chrome/renderer/extensions/cast_streaming_native_handler.cc
  chrome/renderer/extensions/cast_streaming_native_handler.h
  chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
  chrome/renderer/media/cast_receiver_session.cc
  chrome/renderer/media/cast_receiver_session.h
  chrome/renderer/media/cast_receiver_session_delegate.cc
  chrome/renderer/media/cast_receiver_session_delegate.h
  chrome/renderer/media/cast_session.cc
  chrome/renderer/media/cast_session.h
  chrome/renderer/media/cast_session_delegate.cc
  chrome/renderer/media/cast_session_delegate.h
  chrome/renderer/media/cast_udp_transport.cc
  chrome/renderer/media/cast_udp_transport.h
 
chrome/renderer/resources/extensions/cast_streaming_receiver_session_custom_bindings.js
  chrome/renderer/resources/renderer_resources.grd
  content/public/renderer/BUILD.gn
  content/public/renderer/media_stream_api.cc
  content/public/renderer/media_stream_api.h

[jam, 2015-03-03 19:08:05]

On 2015/03/03 18:46:28, hubbe wrote:
> Ok, time for OWNERS review, jam, if you would?
> 
> I'll need your approval for these files, but feel free to look at all of them:
> 
>   chrome/common/extensions/api/_api_features.json
>   chrome/common/extensions/api/cast_streaming_receiver_session.idl
>   chrome/common/extensions/api/schemas.gypi
>   chrome/renderer/extensions/DEPS
>   chrome/renderer/extensions/cast_streaming_native_handler.cc
>   chrome/renderer/extensions/cast_streaming_native_handler.h
>   chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
>   chrome/renderer/media/cast_receiver_session.cc
>   chrome/renderer/media/cast_receiver_session.h
>   chrome/renderer/media/cast_receiver_session_delegate.cc
>   chrome/renderer/media/cast_receiver_session_delegate.h
>   chrome/renderer/media/cast_session.cc
>   chrome/renderer/media/cast_session.h
>   chrome/renderer/media/cast_session_delegate.cc
>   chrome/renderer/media/cast_session_delegate.h
>   chrome/renderer/media/cast_udp_transport.cc
>   chrome/renderer/media/cast_udp_transport.h
>  
>
chrome/renderer/resources/extensions/cast_streaming_receiver_session_custom_bindings.js
>   chrome/renderer/resources/renderer_resources.grd
>   content/public/renderer/BUILD.gn
>   content/public/renderer/media_stream_api.cc
>   content/public/renderer/media_stream_api.h

you shoudl look for media owners to review most of these files.

[hubbe, 2015-03-03 21:41:40]

hubbe@chromium.org changed reviewers:
+ yoz@chromium.org

[hubbe, 2015-03-03 21:41:41]

+yoz

After hacking "git cl onwers" a bit, I got more resonable file lists (no
duplicates):

yoz:
 chrome/common/extensions/api/_api_features.json
 chrome/common/extensions/api/cast_streaming_receiver_session.idl
 chrome/common/extensions/api/schemas.gypi
 chrome/renderer/extensions/DEPS
 chrome/renderer/extensions/cast_streaming_native_handler.cc
 chrome/renderer/extensions/cast_streaming_native_handler.h
 chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc

chrome/renderer/resources/extensions/cast_streaming_receiver_session_custom_bindings.js

jam:
 chrome/renderer/resources/renderer_resources.grd
 content/public/renderer/BUILD.gn
 content/public/renderer/media_stream_api.cc
 content/public/renderer/media_stream_api.h

hclam:
 chrome/renderer/media/cast_receiver_session.cc
 chrome/renderer/media/cast_receiver_session.h
 chrome/renderer/media/cast_receiver_session_delegate.cc
 chrome/renderer/media/cast_receiver_session_delegate.h
 chrome/renderer/media/cast_session.cc
 chrome/renderer/media/cast_session.h
 chrome/renderer/media/cast_session_delegate.cc
 chrome/renderer/media/cast_session_delegate.h
 chrome/renderer/media/cast_udp_transport.cc
 chrome/renderer/media/cast_udp_transport.h

[Alpha Left Google, 2015-03-03 21:53:46]

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:62: //
|localEndpoint| : Set this to bind() to a local UDP port.
On 2015/02/28 00:17:11, hubbe wrote:
> On 2015/02/25 20:37:27, Alpha wrote:
> > I'm not sure how these two parameters are used.
> > 
> > It seems this receiver session can operate in two modes:
> > 1. Server mode with a given port number. What if the port is already used?
Can
> > it bind to a random port?
> 
> It can bind to a random port, but there is currently no way to find out what
> that port is...
> This API needs an error reporting method, so I probably need to add a callback
> 
> > 2. Client mode that connects to a sender. How is this possible with cast
> sender?
> 
> I included this mode even though it's not compatible with the current sender
to
> make it possible to avoid firewall dialogs in some cases.

If this mode is not compatible with sender and we don't plan to have such mode I
think this should be removed. I would prefer an API with less ambiguity. For
example this API can be called "createAndBind". Then later on when we want to
create a receiver in client mode we'll have a "createAndConnect" method.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:79: };
Is a destroy method needed? Or the receiver will be torn down if all
MediaStreams are not referenced?

[hubbe, 2015-03-03 22:36:16]

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:62: //
|localEndpoint| : Set this to bind() to a local UDP port.
On 2015/03/03 21:53:45, Alpha wrote:
> On 2015/02/28 00:17:11, hubbe wrote:
> > On 2015/02/25 20:37:27, Alpha wrote:
> > > I'm not sure how these two parameters are used.
> > > 
> > > It seems this receiver session can operate in two modes:
> > > 1. Server mode with a given port number. What if the port is already used?
> Can
> > > it bind to a random port?
> > 
> > It can bind to a random port, but there is currently no way to find out what
> > that port is...
> > This API needs an error reporting method, so I probably need to add a
callback
> > 
> > > 2. Client mode that connects to a sender. How is this possible with cast
> > sender?
> > 
> > I included this mode even though it's not compatible with the current sender
> to
> > make it possible to avoid firewall dialogs in some cases.
> 
> If this mode is not compatible with sender and we don't plan to have such mode
I
> think this should be removed. I would prefer an API with less ambiguity. For
> example this API can be called "createAndBind". Then later on when we want to
> create a receiver in client mode we'll have a "createAndConnect" method.

Done.

https://codereview.chromium.org/938903003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:79: };
On 2015/03/03 21:53:45, Alpha wrote:
> Is a destroy method needed? Or the receiver will be torn down if all
> MediaStreams are not referenced?

Yes, the receiver goes away with the mediastream.

[Alpha Left Google, 2015-03-03 22:43:48]

LGTM on cast related files.

[Yoyo Zhou, 2015-03-04 03:12:16]

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/common/ex...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/common/ex...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:16: dictionary
CodecSpecificParams {
Is this actually used?

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/common/ex...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:34: long
feedbackSsrc;
Document this

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/common/ex...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:36: // Defaults
to 48000 for audio and 90000 for video.
This should say something like the comment I found while trying to understand
this: "RTP timebase: The number of RTP units advanced per one second"

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/common/ex...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:52: // The
|audioParams| and |videoParams| is generally provided by the
nit: 'is' -> 'are'

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/common/ex...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:55: //
|audioParams| : the video stream parameters
The audio stream parameters.

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/common/ex...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:56: //
|videoParams| : the video stream parameters
nit: Be consistent capitalizing and punctuating.

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/common/ex...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:61: //
|mediaStreamURL| : URL of MediaStream to add the audio & video to.
nit: 'and' rather than '&'

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
File chrome/renderer/extensions/DEPS (right):

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
chrome/renderer/extensions/DEPS:4: 
nit: looks like there's an extra newline

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:645:
config->receiver_ssrc = params->feedback_ssrc;
I'm not sure if you intend for these to have different names - a lot of them are
named differently.

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:648: if
(config->rtp_max_delay_ms < 0 || config->rtp_max_delay_ms > 1000) {
I'm not sure how important it is for this private API, but if we hit this error
condition, we have to read the source code here to find the valid range of
values.

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:677: if
(config->rtp_timebase < 1000 || config->rtp_timebase > 100000) {
likewise here.

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
chrome/renderer/extensions/cast_streaming_native_handler.cc:719: if
(destination->address != "" || !empty_ok) {
I would rewrite this (assuming I understand it correctly):
if (!empty_ok || (destination->address != "" &&
!net::ParseIPLiteralToNumber...))

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
File chrome/renderer/extensions/cast_streaming_native_handler.h (right):

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
chrome/renderer/extensions/cast_streaming_native_handler.h:89: void
AddTracksToMediaStream(
Function documentation?

https://chromiumcodereview.appspot.com/938903003/diff/120001/chrome/renderer/...
chrome/renderer/extensions/cast_streaming_native_handler.h:108: bool
FrameReceiverConfigFromArg(
Ditto.

[jam, 2015-03-04 07:07:07]

rubberstamp lgtm for the files you listed for me.

there needs to be a tracking bug for this work, where is it?

https://codereview.chromium.org/938903003/diff/120001/content/public/renderer...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/938903003/diff/120001/content/public/renderer...
content/public/renderer/media_stream_api.cc:51: new
content::MediaStreamVideoCapturerSource(
ntit: here and elsewhere, no content:: inside content

https://codereview.chromium.org/938903003/diff/120001/content/public/renderer...
content/public/renderer/media_stream_api.cc:97: new MediaStreamAudioSource(
nit: indentation

[hubbe, 2015-03-04 23:12:34]

All comments addressed.
PTAL.

https://codereview.chromium.org/938903003/diff/120001/chrome/common/extension...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://codereview.chromium.org/938903003/diff/120001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:16: dictionary
CodecSpecificParams {
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> Is this actually used?

Not anymore -- deleted.

https://codereview.chromium.org/938903003/diff/120001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:34: long
feedbackSsrc;
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> Document this

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:36: // Defaults
to 48000 for audio and 90000 for video.
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> This should say something like the comment I found while trying to understand
> this: "RTP timebase: The number of RTP units advanced per one second"

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:52: // The
|audioParams| and |videoParams| is generally provided by the
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> nit: 'is' -> 'are'

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:55: //
|audioParams| : the video stream parameters
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> The audio stream parameters.

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:56: //
|videoParams| : the video stream parameters
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> nit: Be consistent capitalizing and punctuating.

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/common/extension...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:61: //
|mediaStreamURL| : URL of MediaStream to add the audio & video to.
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> nit: 'and' rather than '&'

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
File chrome/renderer/extensions/DEPS (right):

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
chrome/renderer/extensions/DEPS:4: 
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> nit: looks like there's an extra newline

Done.

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

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:645:
config->receiver_ssrc = params->feedback_ssrc;
On 2015/03/04 03:12:16, Yoyo Zhou wrote:
> I'm not sure if you intend for these to have different names - a lot of them
are
> named differently.

Result of a merge, fixed now.

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:648: if
(config->rtp_max_delay_ms < 0 || config->rtp_max_delay_ms > 1000) {
On 2015/03/04 03:12:15, Yoyo Zhou wrote:
> I'm not sure how important it is for this private API, but if we hit this
error
> condition, we have to read the source code here to find the valid range of
> values.

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:677: if
(config->rtp_timebase < 1000 || config->rtp_timebase > 100000) {
On 2015/03/04 03:12:16, Yoyo Zhou wrote:
> likewise here.

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:719: if
(destination->address != "" || !empty_ok) {
On 2015/03/04 03:12:16, Yoyo Zhou wrote:
> I would rewrite this (assuming I understand it correctly):
> if (!empty_ok || (destination->address != "" &&
> !net::ParseIPLiteralToNumber...))

Since empty_ok is now always false, I simplified it a bit.

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
File chrome/renderer/extensions/cast_streaming_native_handler.h (right):

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.h:89: void
AddTracksToMediaStream(
On 2015/03/04 03:12:16, Yoyo Zhou wrote:
> Function documentation?

Done.

https://codereview.chromium.org/938903003/diff/120001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.h:108: bool
FrameReceiverConfigFromArg(
On 2015/03/04 03:12:16, Yoyo Zhou wrote:
> Ditto.

Done.

https://codereview.chromium.org/938903003/diff/120001/content/public/renderer...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/938903003/diff/120001/content/public/renderer...
content/public/renderer/media_stream_api.cc:51: new
content::MediaStreamVideoCapturerSource(
On 2015/03/04 07:07:06, jam wrote:
> ntit: here and elsewhere, no content:: inside content

Done.

https://codereview.chromium.org/938903003/diff/120001/content/public/renderer...
content/public/renderer/media_stream_api.cc:97: new MediaStreamAudioSource(
On 2015/03/04 07:07:06, jam wrote:
> nit: indentation

Done.

[Yoyo Zhou, 2015-03-04 23:31:33]

LGTM

https://chromiumcodereview.appspot.com/938903003/diff/200001/chrome/common/ex...
File chrome/common/extensions/api/cast_streaming_receiver_session.idl (right):

https://chromiumcodereview.appspot.com/938903003/diff/200001/chrome/common/ex...
chrome/common/extensions/api/cast_streaming_receiver_session.idl:53: //
|localEndpoint| : Local ip and port to bind to.
nit: IP

https://chromiumcodereview.appspot.com/938903003/diff/200001/chrome/renderer/...
File chrome/renderer/extensions/cast_streaming_native_handler.h (right):

https://chromiumcodereview.appspot.com/938903003/diff/200001/chrome/renderer/...
chrome/renderer/extensions/cast_streaming_native_handler.h:114: // equivialent.
(cast.streaming.receiverSession.RrtpReciverParams)
check the spelling of this function

[hubbe, 2015-03-04 23:38:29]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2015-03-04 23:38:30]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
jam@chromium.org, hclam@chromium.org, yoz@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/938903003/#ps220001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-03-04 23:39:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/938903003/220001

[commit-bot: I haz the power, 2015-03-05 00:40:43]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-05 00:42:05]

Patchset 12 (id:??) landed as
https://crrev.com/4449d976059b4d76ca1671560042f291f9a6083d
Cr-Commit-Position: refs/heads/master@{#319175}