MediaStream should fire ended event when all tracks are ended

MediaStream should fire ended event when all tracks are ended


BUG=227485
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148133

[Li Yin, 2013-04-08 06:20:57]

Spec:http://dev.w3.org/2011/webrtc/editor/getusermedia.html#event-mediastream-ended

[abarth-chromium, 2013-04-08 06:24:21]

https://codereview.chromium.org/13776002/diff/1/Source/WebCore/Modules/medias...
File Source/WebCore/Modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/13776002/diff/1/Source/WebCore/Modules/medias...
Source/WebCore/Modules/mediastream/MediaStream.cpp:336: if
(!m_audioTracks[i].get()->ended())
There's no need to call "get()" here.  operator-> works on smart pointers too.

https://codereview.chromium.org/13776002/diff/1/Source/WebCore/Modules/medias...
File Source/WebCore/Modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/13776002/diff/1/Source/WebCore/Modules/medias...
Source/WebCore/Modules/mediastream/MediaStreamTrack.h:91: MediaStream* m_stream;
What's the ownership relationship between MediaStreamTrack and m_stream?  They
both appear to be refcounted, so it's not clear to me what prevents m_stream
from pointing to freed memory.

[Li Yin, 2013-04-08 08:26:45]

On 2013/04/08 06:24:21, abarth wrote:
>
https://codereview.chromium.org/13776002/diff/1/Source/WebCore/Modules/medias...
> File Source/WebCore/Modules/mediastream/MediaStream.cpp (right):
> 
>
https://codereview.chromium.org/13776002/diff/1/Source/WebCore/Modules/medias...
> Source/WebCore/Modules/mediastream/MediaStream.cpp:336: if
> (!m_audioTracks[i].get()->ended())
> There's no need to call "get()" here.  operator-> works on smart pointers too.

Fixed.

> 
>
https://codereview.chromium.org/13776002/diff/1/Source/WebCore/Modules/medias...
> File Source/WebCore/Modules/mediastream/MediaStreamTrack.h (right):
> 
>
https://codereview.chromium.org/13776002/diff/1/Source/WebCore/Modules/medias...
> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:91: MediaStream*
m_stream;
> What's the ownership relationship between MediaStreamTrack and m_stream?  They
> both appear to be refcounted, so it's not clear to me what prevents m_stream
> from pointing to freed memory.

MediaStreamTrack is created and only owned by MediaStream, when the MediaStream
is freed, it assumes all of MediaStreamTrack are destructed as well.
Anyway, I still added a obvious protect for a better code understanding.

Thanks for your review.

[Tommy Widenflycht, 2013-04-08 14:01:07]

Inofficial LGTM.

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
File Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:155:
m_stream->trackEndedNotification();
This is a bit overkill because when stop() is called the world is going down.

[abarth-chromium, 2013-04-08 16:52:34]

> MediaStreamTrack is created and only owned by MediaStream, when the
MediaStream
> is freed, it assumes all of MediaStreamTrack are destructed as well.
> Anyway, I still added a obvious protect for a better code understanding.

It looks like MediaStreamTrack is reference counted.  How do we know that no one
else has taken a reference to the MediaStreamTrack?  MediaStreamTrack is a
JavaScript object, which means a script can take a reference to the object and
keep it around forever.

[abarth-chromium, 2013-04-08 16:58:41]

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
File Source/WebCore/Modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStream.cpp:194:
m_audioTracks.remove(pos);
Will |track| have a dangling reference to this MediaStream object?

The approach you've taken in this patch to clearing m_stream is very fragile. 
I'd recommend creating an object that wraps Vector that knows how to clear the
m_stream when something is removed.

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStream.cpp:312: tracks->remove(index);
Here's another remove that might leave a dangling reference.

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
File Source/WebCore/Modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStream.h:82: void
trackEndedNotification();
trackEndedNotification -> didEndTrack?

Generally we like to use Objective-C style names for these sorts of
"notification" calls.

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
File Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:126: void
MediaStreamTrack::sourceChangedState()
Who calls this function?  Maybe it would be easier to have the caller pass in
the MediaStream* instead of keeping it on this object.

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:155:
m_stream->trackEndedNotification();
On 2013/04/08 14:01:07, Tommy Widenflycht wrote:
> This is a bit overkill because when stop() is called the world is going down.

Yeah.

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
File Source/WebCore/Modules/mediastream/MediaStreamTrack.h (right):

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStreamTrack.h:57: void
setStream(MediaStream* stream) { m_stream = stream; }
I'd probably have used "clearStream" here rather than setStream because we only
ever us it to clear the stream and we don't actually support moving a track from
one stream to another.

[Li Yin, 2013-04-09 03:11:53]

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
File Source/WebCore/Modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStream.cpp:194:
m_audioTracks.remove(pos);
On 2013/04/08 16:58:41, abarth wrote:
> Will |track| have a dangling reference to this MediaStream object?
> 
> The approach you've taken in this patch to clearing m_stream is very fragile. 
> I'd recommend creating an object that wraps Vector that knows how to clear the
> m_stream when something is removed.

Thanks for your reminder.
If we implement a wrap object for Vector, it seems we still need a public
function to return Vector, because MediaStreamTrackVector is exposed to JS.

[Li Yin, 2013-04-09 06:18:39]

On 2013/04/08 16:52:34, abarth wrote:
> > MediaStreamTrack is created and only owned by MediaStream, when the
> MediaStream
> > is freed, it assumes all of MediaStreamTrack are destructed as well.
> > Anyway, I still added a obvious protect for a better code understanding.
> 
> It looks like MediaStreamTrack is reference counted.  How do we know that no
one
> else has taken a reference to the MediaStreamTrack?  MediaStreamTrack is a
> JavaScript object, which means a script can take a reference to the object and
> keep it around forever.

Sorry, my understanding is wrong, thanks for your correcting.

[Li Yin, 2013-04-09 06:34:07]

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
File Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/13776002/diff/1007/Source/WebCore/Modules/med...
Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:126: void
MediaStreamTrack::sourceChangedState()
On 2013/04/08 16:58:41, abarth wrote:
> Who calls this function?  Maybe it would be easier to have the caller pass in
> the MediaStream* instead of keeping it on this object.

It is a callback from MediaStreamSource::setReadyState.
There is not MediaStream instance in MediaStreamSource, so it is impossible to
pass it as a parameter.

[eseidel, 2013-04-09 06:42:25]

https://codereview.chromium.org/13776002/diff/14001/Source/WebCore/Modules/me...
File Source/WebCore/Modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/13776002/diff/14001/Source/WebCore/Modules/me...
Source/WebCore/Modules/mediastream/MediaStream.cpp:342: void
MediaStream::didEndTrack()
If this dispatches synchronous javascript events, then this is a security
disaster waiting to happen. :)  It's not clear to me the behavior here. 
Synchronous javascript calls can tear down or re-arrange your world in arbitrary
ways. :)

[Li Yin, 2013-04-09 07:38:33]

https://codereview.chromium.org/13776002/diff/14001/Source/WebCore/Modules/me...
File Source/WebCore/Modules/mediastream/MediaStream.cpp (right):

https://codereview.chromium.org/13776002/diff/14001/Source/WebCore/Modules/me...
Source/WebCore/Modules/mediastream/MediaStream.cpp:342: void
MediaStream::didEndTrack()
On 2013/04/09 06:42:25, Eric Seidel (Google) wrote:
> If this dispatches synchronous javascript events, then this is a security
> disaster waiting to happen. :)  It's not clear to me the behavior here. 
> Synchronous javascript calls can tear down or re-arrange your world in
arbitrary
> ways. :)
didEndTrack is only checking whether the event should be triggered, if yes, it
calls existing function "streamEnded".
And streamEnded is using timer to trigger the event.

[abarth-chromium, 2013-04-11 00:48:24]

This looks much better.  Thank you!  LGTM.

https://codereview.chromium.org/13776002/diff/20001/Source/WebCore/platform/m...
File Source/WebCore/platform/mediastream/MediaStreamDescriptor.h (right):

https://codereview.chromium.org/13776002/diff/20001/Source/WebCore/platform/m...
Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:48: virtual void
streamEnded() = 0;
The naming of these two function is inconsistent.  Perhaps we should have used
trackEnded after all...

[Li Yin, 2013-04-11 00:52:39]

https://codereview.chromium.org/13776002/diff/20001/Source/WebCore/platform/m...
File Source/WebCore/platform/mediastream/MediaStreamDescriptor.h (right):

https://codereview.chromium.org/13776002/diff/20001/Source/WebCore/platform/m...
Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:48: virtual void
streamEnded() = 0;
On 2013/04/11 00:48:24, abarth wrote:
> The naming of these two function is inconsistent.  Perhaps we should have used
> trackEnded after all...

Okay, I will change "didEndTrack()" to be "trackEnded()" in
MediaStreamDescriptorClient. And change "trackEnded()" to be "didEndTrack()" in
MediaStreamTrack.

[commit-bot: I haz the power, 2013-04-11 01:23:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/13776002/24001

[commit-bot: I haz the power, 2013-04-11 01:29:06]

Presubmit check for 13776002-24001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 7 file(s).

[abarth-chromium, 2013-04-11 01:42:25]

Committed patchset #5 manually as r148133 (presubmit successful).

[Tommy Widenflycht, 2013-04-11 07:29:15]

Neat! Thanks for the patch.

[Li Yin, 2013-04-15 08:04:57]

Patch Set 6 is uploaded to wrong place. Please ignore it.
I am sorry for the noisy.