When MediaStreamTrack.enabled is changed, event handler can't be triggered in the browser. The same problem is also occurred when LocalMediaStream.stop is called.
Created attachment 190405 [details] Patch
Comment on attachment 190405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190405&action=review > Source/WebCore/ChangeLog:10 > + When MediaStreamTrack.enabled is changed, event handler can't be triggered > + in the browser.The same problem is also occurred when LocalMediaStream.stop > + is called. Would you also add a spec link here? > Source/WebCore/ChangeLog:14 > + No new tests because the followed tests have covered it already. > + fast/mediastream/MediaStreamTrack.html > + fast/mediastream/LocalMediaStream-onended.html I expected some change in these test results, because the patch is changing the behavior. Why no changes in test results?
Comment on attachment 190405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190405&action=review > Source/WebCore/platform/mediastream/chromium/MediaStreamCenterChromium.cpp:80 > + component->source()->setReadyState(MediaStreamSource::ReadyStateLive); This is the wrong behaviour completely. A track has two things which decides if media flows through it: 1) If its underlying source is live or not and 2) if the tracks enables state is true or not. This patch connects these two states into one which is very wrong.
http://dev.w3.org/2011/webrtc/editor/getusermedia.html#mediastreamtrack
Comment on attachment 190405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190405&action=review >> Source/WebCore/platform/mediastream/chromium/MediaStreamCenterChromium.cpp:80 >> + component->source()->setReadyState(MediaStreamSource::ReadyStateLive); > > This is the wrong behaviour completely. > > A track has two things which decides if media flows through it: > > 1) If its underlying source is live or not > and > 2) if the tracks enables state is true or not. > > This patch connects these two states into one which is very wrong. DumpRenderTree does it like this as well. I will delete the related implementations in DumpRenderTree.(Such as function didEnableMediaStreamTrack in MockWebMediaStreamCenter.cpp) And the state of component has been checked already in MediaStreamTeack.cpp. Do you think it is necessary to check the state of source?
Created attachment 190463 [details] Patch
What I am saying is that your patch is wrong according to the standard. Unfortunately for technical reasons we have not been able to make DumpRenderTree behave like a proper client. Today the DRT mocks are there just so that as much as possible of the code can be tested. We'll get to that later when chromium switches to content_shell instead of DRT; then we'll start to use as much of the real code as possible. Maybe even doing the full monty and using a local server like WebSockets. But for now DRT and some LayoutTests are just there for exercising WebKit.
(In reply to comment #7) > What I am saying is that your patch is wrong according to the standard. > > Unfortunately for technical reasons we have not been able to make DumpRenderTree behave like a proper client. Today the DRT mocks are there just so that as much as possible of the code can be tested. > > We'll get to that later when chromium switches to content_shell instead of DRT; then we'll start to use as much of the real code as possible. Maybe even doing the full monty and using a local server like WebSockets. But for now DRT and some LayoutTests are just there for exercising WebKit. If that is true, when onmute, onunmue, onended event handle will be triggered?
(In reply to comment #8) > > If that is true, when onmute, onunmue, onended event handle will be triggered? We are working on it but no ETA yet.
(In reply to comment #9) > (In reply to comment #8) > > > > If that is true, when onmute, onunmue, onended event handle will be triggered? > > We are working on it but no ETA yet. It isn't introduced in the Spec, and could you please give some clarifications why the tests MediaStreamTrack.html and LocalMediaStream-onended.html are not reasonbable?
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > > > > If that is true, when onmute, onunmue, onended event handle will be triggered? > > > > We are working on it but no ETA yet. > > It isn't introduced in the Spec, and could you please give some clarifications why the tests MediaStreamTrack.html and LocalMediaStream-onended.html are not reasonbable? Don't really understand what you are mean. First here's some snippets from the spec that explicitly states that the behavior you added in this patch is wrong, please carefully read the description for the live state: "When a MediaStreamTrack object is created, its readyState is either live or muted, depending on the state of the track’s underlying media source. For example, a track in a LocalMediaStream, created with getUserMedia(), must initially have its readyState attribute set to live." MediaStreamTrack::readyState: "live: The track is active (the track’s underlying media source is making a best-effort attempt to provide data in real time). The output of a track in the live state can be switched on and off with the enabled attribute." "muted: The track is muted (the track’s underlying media source is temporarily unable to provide data)." "ended: The track has ended (the track’s underlying media source is no longer providing data, and will never provide more data for this track)." Secondly when I said that we are working on it I mean that the WebRTC team in chrome is working on enabling the MediaStreamTrack readyState for the chrome browser. Thirdly not all LayoutTests are "wrong" in the sense that they don't implement realistic use cases; but if the choice have been between "not testable" and "unrealistic usage but properly exercises the API" we have chosen the latter for the time being.
(In reply to comment #11) > Don't really understand what you are mean. > Sorry, I meant to write "Don't really understand what you mean."
(In reply to comment #11) > "ended: The track has ended (the track’s underlying media source is no longer providing data, and will never provide more data for this track)." Thanks for your detailed clarification. You are right, MediaStreamTrack.enable can't control the readyState is "mute" or "unmute", I had a wrong understanding. Thanks for your correcting. And from the 4.7 Event summary chapter of spec, ended event handler should be triggered, when stop() is called. If that is true, the modification in my patch about MediaStreamCenterChromium::didStopLocalMediaStream should be correct. Do you think so?
(In reply to comment #13) > And from the 4.7 Event summary chapter of spec, ended event handler should be triggered, when stop() is called. > If that is true, the modification in my patch about MediaStreamCenterChromium::didStopLocalMediaStream should be correct. > > Do you think so? Yeah, this does seem right. Since the LocalMediaStream has a strong reference to its sources all tracks based on these sources should be ended: "The concept with strong and weak references to media sources allows the web application to derive new MediaStream objects from LocalMediaStream objects (created via getUserMedia()) and still be able to revoke all given permissions with LocalMediaStream.stop()." Feel free to update the patch to just include this and please add a link to the standard in the change log. http://dev.w3.org/2011/webrtc/editor/getusermedia.html#mediastreamtrack
Created attachment 190711 [details] Patch
(In reply to comment #14) > (In reply to comment #13) > Feel free to update the patch to just include this and please add a link to the standard in the change log. > http://dev.w3.org/2011/webrtc/editor/getusermedia.html#mediastreamtrack Thanks for your comments. The patch has been updated, please have a look again.
Comment on attachment 190711 [details] Patch LGTM but I am not an reviewer so you need someone to give you the R+. Thanks for working with me to improve the code!
(In reply to comment #2) > (From update of attachment 190405 [details]) > Would you also add a spec link here? Done. > I expected some change in these test results, because the patch is changing the behavior. Why no changes in test results? For ended event, DRT and Browser used different code path, DRT did the correct thing, ended event was triggered when stop was called. I changed browser code, let its behavior be correct like what DRT is doing.
Hi Kentaro, Could you please review the patch? Thanks in advance.
Comment on attachment 190711 [details] Patch r+ing based on tommy's LGTM.
Comment on attachment 190711 [details] Patch Clearing flags on attachment: 190711 Committed r144410: <http://trac.webkit.org/changeset/144410>
All reviewed patches have been landed. Closing bug.