Bug 71678

Created attachment 113858 [details] Patch

Comment on attachment 113858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113858&action=review I would have expected the Client to implement UserMediaClient rather then having WebKit/chromium/src implement UserMediaClient in terms of WebKitPlatformSupport. UserMediaClient seems to be asking for a policy decision, which is the purview of the Client. > Source/WebKit/chromium/public/WebKitPlatformSupport.h:332 > + virtual void requestUserMedia(const WebUserMediaRequest&) { } > + virtual void cancelUserMediaRequest(const WebUserMediaRequest&) { } I would have expected pointers, not references.

Created attachment 114065 [details] Patch

Something along these lines?

Comment on attachment 114065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114065&action=review Yes. This looks great. > Source/WebKit/chromium/src/UserMediaClientImpl.h:37 > + Extra blank line here.

Comment on attachment 114065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114065&action=review > Source/WebKit/chromium/public/WebUserMediaClient.h:36 > + virtual void userMediaControllerDestroyed() = 0; What "user media controller" does this method correspond to? maybe this method should be named cancelAllUserMediaRequests? is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called? do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors? no need to report an error code of some sort in other words? > Source/WebKit/chromium/public/WebUserMediaClient.h:38 > + virtual void cancelUserMediaRequest(const WebUserMediaRequest&) = 0; it seems like the embedder will need some way to compare WebUserMediaRequest objects for equality. otherwise, it will be difficult to correlate a cancelUserMediaRequest to a requestUserMedia call. the WebUserMediaRequest object could be copied, so you cannot use pointer identity. > Source/WebKit/chromium/public/WebUserMediaRequest.h:49 > + void requestSucceeded(const WebVector<WebMediaStreamSource>&); it looks like these will require WEBKIT_EXPORT too

Comment on attachment 114065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114065&action=review >> Source/WebKit/chromium/public/WebUserMediaClient.h:36 >> + virtual void userMediaControllerDestroyed() = 0; > > What "user media controller" does this method correspond to? maybe this method should be named cancelAllUserMediaRequests? is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called? do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors? no need to report an error code of some sort in other words? This is the UserMediaController hanging of the page. I have asked for clarification regarding your second question from the WebCore patch author, and would prefer if cancelUserMediaRequest is called for each outstanding request in addition to this function. And lastly, no we do not care about what went wrong since unfortunately there isn't a way to convey that error currently in the WebRTC spec. >> Source/WebKit/chromium/public/WebUserMediaClient.h:38 >> + virtual void cancelUserMediaRequest(const WebUserMediaRequest&) = 0; > > it seems like the embedder will need some way to compare WebUserMediaRequest objects for equality. otherwise, it will be difficult to correlate a cancelUserMediaRequest to a requestUserMedia call. the WebUserMediaRequest object could be copied, so you cannot use pointer identity. Fixed. >> Source/WebKit/chromium/public/WebUserMediaRequest.h:49 >> + void requestSucceeded(const WebVector<WebMediaStreamSource>&); > > it looks like these will require WEBKIT_EXPORT too Fixed. >> Source/WebKit/chromium/src/UserMediaClientImpl.h:37 >> + > > Extra blank line here. Fixed.

Created attachment 114245 [details] Patch

Created attachment 114255 [details] Patch

Created attachment 114652 [details] Patch

> > Source/WebKit/chromium/public/WebUserMediaClient.h:36 > > + virtual void userMediaControllerDestroyed() = 0; > > What "user media controller" does this method correspond to? maybe this method should be named cancelAllUserMediaRequests? is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called? do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors? no need to report an error code of some sort in other words? > All outstanding request will be cancelled before this call. This function is more along the general cleanup line.

(In reply to comment #7) > (From update of attachment 114065 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114065&action=review > > >> Source/WebKit/chromium/public/WebUserMediaClient.h:36 > >> + virtual void userMediaControllerDestroyed() = 0; > > > > What "user media controller" does this method correspond to? maybe this method should be named cancelAllUserMediaRequests? is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called? do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors? no need to report an error code of some sort in other words? > > This is the UserMediaController hanging of the page. From the point of view of the embedder, who only sees WebKit APIs, the UserMediaController handing off of the page does not exist. Why do we need this method on WebUserMediaClient, and can we give it a name that makes sense from the perspective of the embedder?

> From the point of view of the embedder, who only sees WebKit APIs, the UserMediaController handing off of the page does not exist. ... and now it doesn't exist in WebCore either. :)

Comment on attachment 114065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114065&action=review >>>> Source/WebKit/chromium/public/WebUserMediaClient.h:36 >>>> + virtual void userMediaControllerDestroyed() = 0; >>> >>> What "user media controller" does this method correspond to? maybe this method should be named cancelAllUserMediaRequests? is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called? do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors? no need to report an error code of some sort in other words? >> >> This is the UserMediaController hanging of the page. >> >> I have asked for clarification regarding your second question from the WebCore patch author, and would prefer if cancelUserMediaRequest is called for each outstanding request in addition to this function. >> >> And lastly, no we do not care about what went wrong since unfortunately there isn't a way to convey that error currently in the WebRTC spec. > > From the point of view of the embedder, who only sees WebKit APIs, the UserMediaController handing off of the page does not exist. Why do we need this method on WebUserMediaClient, and can we give it a name that makes sense from the perspective of the embedder? This functionality is very important to have, because it signals that the system should shut down all LocalMediaStreams and their camera/audio drivers etc. Would calling it close() or shutdown() be better?

(In reply to comment #13) > > From the point of view of the embedder, who only sees WebKit APIs, the UserMediaController handing off of the page does not exist. > > ... and now it doesn't exist in WebCore either. :) Unfortunately it got removed :(

> This functionality is very important to have, because it signals that the system should shut down all LocalMediaStreams and their camera/audio drivers etc. Would calling it close() or shutdown() be better? Why doesn't each LocalMediaStream signal that individually?

(In reply to comment #16) > > This functionality is very important to have, because it signals that the system should shut down all LocalMediaStreams and their camera/audio drivers etc. Would calling it close() or shutdown() be better? > > Why doesn't each LocalMediaStream signal that individually? We need to now when the page is going away (page reload, closing page etc) not when the objects are garbage collected. I thought that it was best to detect that in the Document object but I am happy to learn other and better ways to reach the same goal.

It sounds like whatever is holding the LocalMediaStream objects should watch for being disconnected from the Frame (or removed from the document, etc) and then close the LocalMediaStream (which should signal the platform to spin down the devices).

Tommy and I chatted about this topic, and we think the best solution is to make MediaStream an ActiveDOMObject, which will let it get a stop() callback when the Document detaches from the Frame. It will also let it get suspend/resume calls for browsers that implement bfcache. Everything should be set for that approach because MediaStream already gets a ScriptExecutionContext in its constructor.

(In reply to comment #19) > Tommy and I chatted about this topic, and we think the best solution is to make MediaStream an ActiveDOMObject, which will let it get a stop() callback when the Document detaches from the Frame. It will also let it get suspend/resume calls for browsers that implement bfcache. > > Everything should be set for that approach because MediaStream already gets a ScriptExecutionContext in its constructor. I'm not so sure that making MediaStream an ActiveDOMObject is the right approach. We see MediaStream/LocalMediaStream as an abstract representation of an actual media stream in the platform (similar to how a Blob can represent a file in the filesystem). The underlying media stream in the platform is set up when the media stream is consumed. The consumers of MediaStream objects are (currently) PeerConnection and the media elements, which have well defined lifetime handling. Another aspect is that a MediaStream object can go away but the underlying media stream must still be consumable if there is a URL created for it using createObjectURL(). That's why we have MediaStreamDescriptor (to allow MediaStream objects to be garbage collected). Perhaps we should move this discussion to the MediaStream platform bug (http://webkit.org/b/70895)?

Regarding the discussion taking place in the last comments, I have decided to put this on hold to se if we can get the necessary signalling from the video decoder instead.

Created attachment 115578 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Created attachment 115579 [details] Patch

Comment on attachment 115579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115579&action=review > Source/WebKit/chromium/src/WebUserMediaRequest.cpp:122 > + UserMediaRequest* p = other.m_private.get(); > + if (p) > + p->ref(); > + m_private = p; Is there a way to do this without manual reference counting?

Comment on attachment 115579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115579&action=review >> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:122 >> + m_private = p; > > Is there a way to do this without manual reference counting? The WebPrivatePtr takes over the refcounted object using leakref() and the calls dererf() at destructor time, so for an object referenced from elsewhere this is needed. Dunno exactly about the design philosophy but hope that Darin can teach me the correct way here.

Comment on attachment 115579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115579&action=review >>> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:122 >>> + UserMediaRequest* p = other.m_private.get(); >>> + if (p) >>> + p->ref(); >>> + m_private = p; >> >> Is there a way to do this without manual reference counting? > > The WebPrivatePtr takes over the refcounted object using leakref() and the calls dererf() at destructor time, so for an object referenced from elsewhere this is needed. Dunno exactly about the design philosophy but hope that Darin can teach me the correct way here. Can we use "RefPtr<UserMediaRequest> p" instead? (I guess I could look at the definition of WebPrivatePtr in more detail).

Comment on attachment 115579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115579&action=review > Source/WebKit/chromium/public/WebUserMediaClient.h:28 > +#include "WebVector.h" nit: please just forward declare WebVector (see for example WebPageSerializer.h) > Source/WebKit/chromium/public/WebUserMediaClient.h:39 > + virtual void pageDestroyed() = 0; "page" (referring to WebCore::Page) is also a name that doesn't exist in the WebKit API. I think a name like "cancelAllUserMediaRequests" might be better here. It is much more specific. > Source/WebKit/chromium/public/WebUserMediaRequest.h:49 > + WEBKIT_EXPORT WebUserMediaRequest& operator=(const WebUserMediaRequest&); as with other interface, please define an assign method, and implement operator= in terms of that. > Source/WebKit/chromium/public/WebUserMediaRequest.h:60 > + WEBKIT_EXPORT void requestSucceeded(const WebVector<WebMediaStreamSource>&); There was some discussion (over email) about making requestSucceeded and requestFailed virtual to support mocking in unit tests. I commented that I thought perhaps that suggested that these functions really don't belong here on the 'request' object. Compare this to WebURLRequest, which just describes the parameters of a request. It is just data. > Source/WebKit/chromium/public/WebUserMediaRequest.h:63 > + WEBKIT_EXPORT bool operator==(const WebUserMediaRequest&) const; nit: we don't export operators in the WebKit API. Please add an equals method, and then define an inline operator== that calls the equals method. See WebNode.h for example. > Source/WebKit/chromium/public/WebUserMediaRequest.h:66 > + WebUserMediaRequest(WebCore::UserMediaRequest*); nit: do you really need both forms of constructor and both forms of operator here? we don't elsewhere in the API. > Source/WebKit/chromium/src/WebUserMediaRequest.cpp:102 > + m_private->succeed(s); I'm trying to study what happens when succeed and fail are called, but I don't see UserMediaRequest checked into the tree. Is there a separate bug with that patch? I'm a little concerned about the idea that UserMediaRequest is both data representing the request and controller code for what happens when the request succeeds or fails.

> I'm trying to study what happens when succeed and fail are called, but I don't see > UserMediaRequest checked into the tree. Is there a separate bug with that patch? http://trac.webkit.org/browser/trunk/Source/WebCore/mediastream/UserMediaRequest.h http://trac.webkit.org/browser/trunk/Source/WebCore/mediastream/UserMediaRequest.cpp It might not have been rolled into Chromium yet.

(In reply to comment #29) > It might not have been rolled into Chromium yet. Ah, yes. I was using codesearch :-( So, UserMediaRequest ends up having a m_scriptExecutionContext member precisely for it to handle these events. It seems unfortunate for UserMediaRequest to be both data representing the request as well as controller that knows how to process completion of the request.

> So, UserMediaRequest ends up having a m_scriptExecutionContext member precisely for it to handle these events. It seems unfortunate for UserMediaRequest to be both data representing the request as well as controller that knows how to process completion of the request. I've been pushing this code not to have long-lived (e.g., Document- or Page-scoped) controllers. Maybe the best solution is to split UserMediaRequest into UserMediaRequester and UserMediaRequest, with the later being just pure data. That would keep the lifetime of these objects short (i.e., limited to the lifetime of the request).

(In reply to comment #31) > > So, UserMediaRequest ends up having a m_scriptExecutionContext member precisely for it to handle these events. It seems unfortunate for UserMediaRequest to be both data representing the request as well as controller that knows how to process completion of the request. > > I've been pushing this code not to have long-lived (e.g., Document- or Page-scoped) controllers. Maybe the best solution is to split UserMediaRequest into UserMediaRequester and UserMediaRequest, with the later being just pure data. That would keep the lifetime of these objects short (i.e., limited to the lifetime of the request). I see. Hmm...

Created attachment 116057 [details] Patch

Comment on attachment 115579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115579&action=review >> Source/WebKit/chromium/public/WebUserMediaClient.h:28 >> +#include "WebVector.h" > > nit: please just forward declare WebVector (see for example WebPageSerializer.h) Done. >> Source/WebKit/chromium/public/WebUserMediaClient.h:39 >> + virtual void pageDestroyed() = 0; > > "page" (referring to WebCore::Page) is also a name that doesn't exist in the WebKit API. > > I think a name like "cancelAllUserMediaRequests" might be better here. It is much more specific. Since all outstanding requests should have been cancelled before, I'm just calling it close() >> Source/WebKit/chromium/public/WebUserMediaRequest.h:49 >> + WEBKIT_EXPORT WebUserMediaRequest& operator=(const WebUserMediaRequest&); > > as with other interface, please define an assign method, and implement operator= > in terms of that. That is exactly what I had done, but I guess you mean to also inline operator=? >> Source/WebKit/chromium/public/WebUserMediaRequest.h:60 >> + WEBKIT_EXPORT void requestSucceeded(const WebVector<WebMediaStreamSource>&); > > There was some discussion (over email) about making requestSucceeded and requestFailed > virtual to support mocking in unit tests. I commented that I thought perhaps that > suggested that these functions really don't belong here on the 'request' object. > > Compare this to WebURLRequest, which just describes the parameters of a request. It > is just data. I'm just wrapping existing WebCore code. Do you want me to split this class into data and controller objects here in the chromium embedder layer or does the WebCore class need to be rewritten before we can try to add the corresponding WebKit code again? >> Source/WebKit/chromium/public/WebUserMediaRequest.h:63 >> + WEBKIT_EXPORT bool operator==(const WebUserMediaRequest&) const; > > nit: we don't export operators in the WebKit API. Please add an equals method, > and then define an inline operator== that calls the equals method. See WebNode.h > for example. Done. >> Source/WebKit/chromium/public/WebUserMediaRequest.h:66 >> + WebUserMediaRequest(WebCore::UserMediaRequest*); > > nit: do you really need both forms of constructor and both forms of operator here? > we don't elsewhere in the API. Removed. >>>> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:122 >>>> + m_private = p; >>> >>> Is there a way to do this without manual reference counting? >> >> The WebPrivatePtr takes over the refcounted object using leakref() and the calls dererf() at destructor time, so for an object referenced from elsewhere this is needed. Dunno exactly about the design philosophy but hope that Darin can teach me the correct way here. > > Can we use "RefPtr<UserMediaRequest> p" instead? (I guess I could look at the definition of WebPrivatePtr in more detail). The pattern is to use WebPrivatePtr

Created attachment 116081 [details] Patch

The difference between the latest patch and the one before it is that I have introduced a WebUserMediaController and made WebUserMediaRequest a data only object. Is this a better approach?

Comment on attachment 116081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116081&action=review > Source/WebKit/chromium/public/WebUserMediaClient.h:39 > + virtual void useController(WebUserMediaController*); I would think that this should be a WebView::userMediaController getter, just as you have WebViewClient::userMediaClient. > Source/WebKit/chromium/public/WebUserMediaClient.h:40 > + virtual void close() = 0; If this is not about cancelling pending WebUserMediaRequests, then what is it for? Why do we need this |close| function?

Comment on attachment 116081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116081&action=review > Source/WebKit/chromium/src/UserMediaClientImpl.h:43 > +class UserMediaClientImpl : public WebCore::UserMediaClient, public WebUserMediaController { If WebUserMediaController is the way to go, then it seems UserMediaController would also be warranted for the WebCore side.

Created attachment 116110 [details] Patch

Comment on attachment 116110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116110&action=review > Source/WebKit/chromium/src/UserMediaClientImpl.cpp:14 > + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY nit: this copyright notice incorrectly says that this software is provided by "APPLE" I think you should switch "APPLE" to "GOOGLE" here, and in the other newly added files. > Source/WebKit/chromium/src/UserMediaClientImpl.h:47 > + virtual void pageDestroyed(); nit: Maybe we can delete UserMediaClient::pageDestroyed now?

Comment on attachment 116110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116110&action=review >> Source/WebKit/chromium/src/UserMediaClientImpl.cpp:14 >> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY > > nit: this copyright notice incorrectly says that this software is provided by "APPLE" > > I think you should switch "APPLE" to "GOOGLE" here, and in the other newly added files. Will fix. >> Source/WebKit/chromium/src/UserMediaClientImpl.h:47 >> + virtual void pageDestroyed(); > > nit: Maybe we can delete UserMediaClient::pageDestroyed now? I think that Ericsson needs this for their GTK/gstreamer port.

Created attachment 116121 [details] Patch

Comment on attachment 116121 [details] Patch Clearing flags on attachment: 116121 Committed r100970: <http://trac.webkit.org/changeset/100970>

All reviewed patches have been landed. Closing bug.

The difference between the latest patch and the one before it is that I have introduced a WebUserMediaController and made WebUserMediaRequest a data only object. Is this a better approach?

Created attachment 116203 [details] Patch

Comment on attachment 116203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116203&action=review > Source/WebKit/chromium/src/UserMediaClientImpl.cpp:46 > + : m_webClient(client ? client->userMediaClient() : 0) This is the only line that differs from the previous patch.

Comment on attachment 116203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116203&action=review > Source/WebKit/chromium/src/UserMediaClientImpl.h:49 > + UserMediaClientImpl(WebViewClient*); nit: more conventional to initialize with a WebViewImpl*. see EditorClientImpl. > Source/WebKit/chromium/src/UserMediaClientImpl.h:59 > + WebUserMediaClient* m_webClient; nit: more conventional to just call this variable m_client. > Source/WebKit/chromium/src/WebViewImpl.cpp:375 > + , m_userMediaClient(adoptPtr(new UserMediaClientImpl(client))) why does m_userMediaClient need to be heap allocated? see m_editorClientImpl for example. it would also be a bit more conventional to name m_userMediaClient as m_userMediaClientImpl.

Comment on attachment 116203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116203&action=review >> Source/WebKit/chromium/src/UserMediaClientImpl.h:49 >> + UserMediaClientImpl(WebViewClient*); > > nit: more conventional to initialize with a WebViewImpl*. see EditorClientImpl. Done. >> Source/WebKit/chromium/src/UserMediaClientImpl.h:59 >> + WebUserMediaClient* m_webClient; > > nit: more conventional to just call this variable m_client. Done. >> Source/WebKit/chromium/src/WebViewImpl.cpp:375 >> + , m_userMediaClient(adoptPtr(new UserMediaClientImpl(client))) > > why does m_userMediaClient need to be heap allocated? see m_editorClientImpl for example. > it would also be a bit more conventional to name m_userMediaClient as m_userMediaClientImpl. Fixed^2. It is a mix of heap allocated and contained objects in this class and I just choose the wrong one.

Created attachment 116253 [details] Patch

Comment on attachment 116253 [details] Patch Clearing flags on attachment: 116253 Committed r101058: <http://trac.webkit.org/changeset/101058>

All reviewed patches have been landed. Closing bug.