Adding support for MediaStream and PeerConnection functionality.

Adding support for MediaStream and PeerConnection functionality.

Depending on pending and upcoming WebKit patches. I wanted to get this uploaded to get an informal review and get all the dirt off. Unit test will be added soon.

BUG=
TEST=

[grunell (dont use), 2011-09-22 14:48:19]

Tommi, can you take first look at this?

[tommi (sloooow) - chröme, 2011-09-23 13:02:42]

first round of comments

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:60: chrome_worker_thread_.Start();
should we do this in newPeerConnection instead?  (it can fail).

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:80: int requestId,
WebKit::WebGenerateStreamOptionFlags flags,
see 'BadWrappingStyle' under function declarations on
http://www.chromium.org/developers/coding-style

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:83: bool audio = flags &
WebKit::WebGenerateStreamRequestAudio;
won't this generate a compiler warning?  Should perhaps be:

bool audio = (flags & WebKit::WebGenerateStreamRequestAudio) != 0;

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:165: << UTF16ToUTF8(streamLabel) <<
")";
align <<

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:260: media_engine_, new
cricket::DummyDeviceManager(), worker_thread_));
run gcl lint?

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:317: if (label.size() == 0)
if (label.empty())

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:358:
scoped_refptr<media::VideoDecoder> sr_decoder(decoder);
why not make decoder a scoped_refptr from the beginning?

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:375: it = audio_array.begin(); it !=
audio_array.end(); ++it) {
indent

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:375: it = audio_array.begin(); it !=
audio_array.end(); ++it) {
is the iterator used?

Could you instead just do:
int track_num = web_track_vector.size();
while (track_num--) {
  web_track_vector[track_num].initialize(...);
}

?

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:377:
WebKit::WebString::fromUTF8(""),
initialize all these strings outside the loop.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:383: for
(media_stream::StreamDeviceInfoArray::const_iterator
same iterator question

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:384: it = video_array.begin(); it !=
video_array.end(); ++it) {
indent

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:386:
WebKit::WebString::fromUTF8(""),
initialize outside of the loop

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:400: void
MediaStreamImpl::OnStreamGenerationFailed(int requestId) {
fix parameter names to chromium style.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:455: << video << ")";
indent

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:490: if
(!message_loop_proxy_->BelongsToCurrentThread())
use braces

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
File content/renderer/media/media_stream_impl.h (right):

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:65: virtual void
setController(WebKit::WebMediaStreamController*);
Missing parameter names in a few methods.  Also, I believe there is now
preference to use the OVERRIDE macro for implementations of derived virtual
functions.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:120: void
OnAddStreamCallback(std::string streamLabel);
const &

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:121: void
OnRemoveStreamCallback(std::string streamLabel);
const &

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:124: void
initializeWorkerThread(talk_base::Thread** thread,
InitializeWorkerThread

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:144: cricket::HttpPortAllocator*
port_allocator_;
do you need to hang on to all these in the MediaStream class or are they
constructed and used only once and never actually used by MediaStream?

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:146: int peer_connection_id_;
document the purpose of peer_connection_id_ and peer_connection_

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:174: // Call states. Possible
transitions:
empty line before this one as above

http://codereview.chromium.org/7990004/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7990004/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:4462: if (!media_stream_impl_.get())
braces.  basically the only time chromium doesn't use braces for if statements
is when they span only two lines.

[grunell (dont use), 2011-09-29 13:38:07]

Fixed most of the things, see comments. New patch will be uploaded in a new
issue, under my chromium account. Sorry for the inconvenience.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
File content/renderer/media/media_stream_impl.cc (right):

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:60: chrome_worker_thread_.Start();
On 2011/09/23 13:02:42, tommi wrote:
> should we do this in newPeerConnection instead?  (it can fail).

I think so, moved it and added check of return value. We should probably stop it
too when deleting the PeerConnection. Added a todo comment for this.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:80: int requestId,
WebKit::WebGenerateStreamOptionFlags flags,
On 2011/09/23 13:02:42, tommi wrote:
> see 'BadWrappingStyle' under function declarations on
> http://www.chromium.org/developers/coding-style

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:83: bool audio = flags &
WebKit::WebGenerateStreamRequestAudio;
On 2011/09/23 13:02:42, tommi wrote:
> won't this generate a compiler warning?  Should perhaps be:
> 
> bool audio = (flags & WebKit::WebGenerateStreamRequestAudio) != 0;

No it doesn't. It will evaluate and assign with implicit cast. I vote for the
current code being clearer, what do you think?

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:165: << UTF16ToUTF8(streamLabel) <<
")";
On 2011/09/23 13:02:42, tommi wrote:
> align <<

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:260: media_engine_, new
cricket::DummyDeviceManager(), worker_thread_));
On 2011/09/23 13:02:42, tommi wrote:
> run gcl lint?

Yes I had done so, no complaints. I fixed the indent however.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:317: if (label.size() == 0)
On 2011/09/23 13:02:42, tommi wrote:
> if (label.empty())

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:358:
scoped_refptr<media::VideoDecoder> sr_decoder(decoder);
On 2011/09/23 13:02:42, tommi wrote:
> why not make decoder a scoped_refptr from the beginning?

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:375: it = audio_array.begin(); it !=
audio_array.end(); ++it) {
On 2011/09/23 13:02:42, tommi wrote:
> is the iterator used?
> 
> Could you instead just do:
> int track_num = web_track_vector.size();
> while (track_num--) {
>   web_track_vector[track_num].initialize(...);
> }
> 
> ?

Agree, with some modification to set audio and video separately.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:375: it = audio_array.begin(); it !=
audio_array.end(); ++it) {
On 2011/09/23 13:02:42, tommi wrote:
> indent

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:377:
WebKit::WebString::fromUTF8(""),
On 2011/09/23 13:02:42, tommi wrote:
> initialize all these strings outside the loop.

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:377:
WebKit::WebString::fromUTF8(""),
On 2011/09/23 13:02:42, tommi wrote:
> initialize all these strings outside the loop.

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:383: for
(media_stream::StreamDeviceInfoArray::const_iterator
On 2011/09/23 13:02:42, tommi wrote:
> same iterator question

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:384: it = video_array.begin(); it !=
video_array.end(); ++it) {
On 2011/09/23 13:02:42, tommi wrote:
> indent

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:386:
WebKit::WebString::fromUTF8(""),
On 2011/09/23 13:02:42, tommi wrote:
> initialize outside of the loop

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:400: void
MediaStreamImpl::OnStreamGenerationFailed(int requestId) {
On 2011/09/23 13:02:42, tommi wrote:
> fix parameter names to chromium style.

Done for all functions.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:455: << video << ")";
On 2011/09/23 13:02:42, tommi wrote:
> indent

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.cc:490: if
(!message_loop_proxy_->BelongsToCurrentThread())
On 2011/09/23 13:02:42, tommi wrote:
> use braces

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
File content/renderer/media/media_stream_impl.h (right):

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:65: virtual void
setController(WebKit::WebMediaStreamController*);
On 2011/09/23 13:02:42, tommi wrote:
> Missing parameter names in a few methods.  Also, I believe there is now
> preference to use the OVERRIDE macro for implementations of derived virtual
> functions.

Both done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:120: void
OnAddStreamCallback(std::string streamLabel);
On 2011/09/23 13:02:42, tommi wrote:
> const &

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:121: void
OnRemoveStreamCallback(std::string streamLabel);
On 2011/09/23 13:02:42, tommi wrote:
> const &

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:124: void
initializeWorkerThread(talk_base::Thread** thread,
On 2011/09/23 13:02:42, tommi wrote:
> InitializeWorkerThread

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:144: cricket::HttpPortAllocator*
port_allocator_;
On 2011/09/23 13:02:42, tommi wrote:
> do you need to hang on to all these in the MediaStream class or are they
> constructed and used only once and never actually used by MediaStream?

Not port_allocator_, removed this. The other, yes.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:146: int peer_connection_id_;
On 2011/09/23 13:02:42, tommi wrote:
> document the purpose of peer_connection_id_ and peer_connection_

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/media/media_st...
content/renderer/media/media_stream_impl.h:174: // Call states. Possible
transitions:
On 2011/09/23 13:02:42, tommi wrote:
> empty line before this one as above

Done.

http://codereview.chromium.org/7990004/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7990004/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:4462: if (!media_stream_impl_.get())
On 2011/09/23 13:02:42, tommi wrote:
> braces.  basically the only time chromium doesn't use braces for if statements
> is when they span only two lines.

Done.

[grunell (dont use), 2011-09-29 13:50:33]

New patch is in issue 8060055.

[grunell (dont use), 2011-09-29 13:51:26]

On 2011/09/29 13:50:33, Henrik Grunell wrote:
> New patch is in issue 8060055.

http://codereview.chromium.org/8060055/