Connect webrtc MediaSourceInterface ready states with webkit WebMediaStreamSource

Connect webrtc MediaSourceInterface ready states with webkit WebMediaStreamSource

BUG=225385, webrtc:960
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196724

[kcwu, 2013-04-18 09:56:41]

Revised previous changeset of issue 13461002.
The differences to previous are:
- adding code in media_stream_imp.cc to break circular reference
- fix memory leak in unittests
- add comments to describe the circular reference

[perkj_chrome, 2013-04-18 13:36:20]

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:229: void
MediaStreamImpl::ReleaseSourceObservers(
Why does this method has to be static? 
You only call it from within UserMediaRequestInfo::~UserMediaRequestInfo() ?

This does exactly what I suggested and have the same problem- ie - if a track is
removed from |descriptor| the WebKitMediaStreamSource and its extra data field
is leaked.

Please add a todo with this - same as in your media_stream_source_observer.h

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:235:
descriptor.audioSources(audio_tracks);
use audioTracks

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:237: const
WebKit::WebMediaStreamSource& source = audio_tracks[i].source();
Thinking about this- You can do 
source.setExtraData(NULL);
source.setReadyState(ReadyStateEnded) ended here.

To get a non const WebKitMediaStreamSource do 
WebKitMediaStreamSource source(audio_tracks[i].source());

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:246:
descriptor.videoSources(video_tracks);
use videoTracks

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:626:
ReleaseSourceObservers(descriptor);
Did I miss something or can you move the ReleaseSourceObservers implementation
to here?

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl_unittest.cc:5: #include
"base/memory/scoped_ptr.h"
Can you add a unit test to test that the WebKit source is changed here.

Inspiration can be found here:
https://codereview.chromium.org/14200016/

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl_unittest.cc:178:
MediaStreamImpl::ReleaseSourceObservers(mixed_desc);
You should not have to do this since you call
MediaStreamImpl::ReleaseSourceObservers from the ~UserMediaRequestInfo

Here and below

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
File content/renderer/media/media_stream_source_observer.cc (right):

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_source_observer.cc:39: /* kInitializing is
not ready state. */
Can you rewrite this comment a bit. Ie kInitializing does not exist  as  a
WebKitSource::ReadyState.

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
File content/renderer/media/media_stream_source_observer.h (right):

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_source_observer.h:33: const
WebKit::WebMediaStreamSource& webkit_source);
How about preparing for  ExtraData::Owner()  ? 

const WebKit::WebMediaStreamSource& ExtraData::Owner() is landed?

Ie- let the ctor take a pointer to MediaStreamSourceExtraData as argument
instead of |webkit_source|.

Add a method MediaStreamSourceExtraData::Owner that returns 
|webkit_source_|.

That way you will not have to change this class (or at least minimal) once
::Owner is landed.

This is just a suggestion- if you rather want to do this change later I am ok
with it.

[Avi (use Gerrit), 2013-04-18 14:43:33]

Content stamp lgtm; make sure you get the OK from the media experts before
landing.

[tommi (sloooow) - chröme, 2013-04-23 09:50:55]

will wait with further review until rebase has been done and the comments so far
have been addressed.

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:229: void
MediaStreamImpl::ReleaseSourceObservers(
On 2013/04/18 13:36:20, perkj wrote:
> Why does this method has to be static? 
> You only call it from within UserMediaRequestInfo::~UserMediaRequestInfo() ?
> 
> This does exactly what I suggested and have the same problem- ie - if a track
is
> removed from |descriptor| the WebKitMediaStreamSource and its extra data field
> is leaked.
> 
> Please add a todo with this - same as in your media_stream_source_observer.h

...and file a bug as well?

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_impl.cc:237: const
WebKit::WebMediaStreamSource& source = audio_tracks[i].source();
On 2013/04/18 13:36:20, perkj wrote:
> Thinking about this- You can do 
> source.setExtraData(NULL);
> source.setReadyState(ReadyStateEnded) ended here.
> 
> To get a non const WebKitMediaStreamSource do 
> WebKitMediaStreamSource source(audio_tracks[i].source());
> 

If you create a new source object from another const one, then do they still
represent the same source?  I.e. changes in one will be reflected in the other?

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
File content/renderer/media/media_stream_source_extra_data.h (right):

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_source_extra_data.h:42:
source_observer_.reset(observer);
If the observer interface is implemented by an owner of the
MediaStreamSourceExtraData (indirectly), then don't we have a problem?

(i.e. is this the cause of the circular dependency?)

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
File content/renderer/media/media_stream_source_observer.cc (right):

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_source_observer.cc:30: 
one empty line

https://codereview.chromium.org/14346002/diff/9001/content/renderer/media/med...
content/renderer/media/media_stream_source_observer.cc:39: /* kInitializing is
not ready state. */
On 2013/04/18 13:36:20, perkj wrote:
> Can you rewrite this comment a bit. Ie kInitializing does not exist  as  a
> WebKitSource::ReadyState.

This looks like copy/paste from another changelist, right?
Maybe you need to rebase?

[kcwu, 2013-04-26 04:36:48]

Thanks for perkj's help. Apply his work.

[kcwu, 2013-04-26 08:26:46]

rebased

[tommi (sloooow) - chröme, 2013-04-26 08:54:48]

lgtm % comments below.

https://codereview.chromium.org/14346002/diff/22001/content/renderer/media/me...
File content/renderer/media/media_stream_source_observer.cc (right):

https://codereview.chromium.org/14346002/diff/22001/content/renderer/media/me...
content/renderer/media/media_stream_source_observer.cc:18:
DCHECK(CalledOnValidThread());
nit: this isn't needed in a ctor.

https://codereview.chromium.org/14346002/diff/22001/content/renderer/media/me...
content/renderer/media/media_stream_source_observer.cc:23: if (webrtc_source_)
...but checking if we're on the correct thread in the dtor is a good idea.

https://codereview.chromium.org/14346002/diff/22001/content/renderer/media/me...
content/renderer/media/media_stream_source_observer.cc:31: 
only one empty line

[kcwu, 2013-04-26 10:20:52]

https://codereview.chromium.org/14346002/diff/22001/content/renderer/media/me...
File content/renderer/media/media_stream_source_observer.cc (right):

https://codereview.chromium.org/14346002/diff/22001/content/renderer/media/me...
content/renderer/media/media_stream_source_observer.cc:18:
DCHECK(CalledOnValidThread());
On 2013/04/26 08:54:48, tommi wrote:
> nit: this isn't needed in a ctor.

Done.

https://codereview.chromium.org/14346002/diff/22001/content/renderer/media/me...
content/renderer/media/media_stream_source_observer.cc:23: if (webrtc_source_)
On 2013/04/26 08:54:48, tommi wrote:
> ...but checking if we're on the correct thread in the dtor is a good idea.

Done.

https://codereview.chromium.org/14346002/diff/22001/content/renderer/media/me...
content/renderer/media/media_stream_source_observer.cc:31: 
On 2013/04/26 08:54:48, tommi wrote:
> only one empty line

Done.

[commit-bot: I haz the power, 2013-04-26 10:30:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kcwu@chromium.org/14346002/27002

[commit-bot: I haz the power, 2013-04-26 13:52:04]

Change committed as 196724