Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10928043:
Media Related changes for TabCapture API (Closed)
|
Created:
8 years, 3 months ago by justinlin Modified:
7 years, 10 months ago Reviewers:
Tommy Widenflycht, jam, no longer working on chromium, miu, perkj_chrome, wjia(left Chromium) CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, feature-media-reviews_chromium.org, jam, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, hclam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis is the media related changes for tab capture API's:
- Add new MediaObserver method to enable new tab capture chrome extension API's to be notified of stream request changes for Tab type streams.
- Adds constants for chrome-specific MediaStreamConstraints: mediaSource and mediaSourceId to be used to request video/audio streams with tab media.
- Plumbing/Tweaks to GenerateStreamForDevice for the API to work.
Dependent change: http://codereview.chromium.org/11038021
BUG=153388
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161881
Patch Set 1 #Patch Set 2 : Initial #
Total comments: 10
Patch Set 3 : Address comments, use MediaConstraints, split CL into 2. #Patch Set 4 : Fix unit test #
Total comments: 47
Patch Set 5 : Address comments, fixes, make tab capture skip approval. #
Total comments: 12
Patch Set 6 : Review fixes #
Total comments: 34
Patch Set 7 : Review fixes #Patch Set 8 : #
Total comments: 26
Patch Set 9 : #
Total comments: 4
Patch Set 10 : Review fixes, fix unit tests. #
Total comments: 14
Patch Set 11 : Review changes #Patch Set 12 : Fix Windows build issue #
Total comments: 2
Patch Set 13 : rename REQUEST_*->MEDIA_REQUEST_* #
Total comments: 1
Messages
Total messages: 36 (0 generated)
Hi tommyw@, perkj@, aa@, please take a look. There's still a few loose ends because some of the backends interfaces are not implemented yet, but looking for an initial review/feedback, thanks!
Cool- I am afraid I can't help you much on the extension parts. Is the idea that the screen share will implement the camera interface in the browser process and behave as USB camera or how will webrtc and normal rendering be able to get the video buffers? Do you have some more info on how this will fit together? https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... File content/renderer/media/media_stream_impl.cc (right): https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... content/renderer/media/media_stream_impl.cc:180: const std::string& device_id) { unused? https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... content/renderer/media/media_stream_impl.cc:204: media_stream_dispatcher_->GenerateStream( what is the difference between these two cases? https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... File content/renderer/media/media_stream_impl.h (right): https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... content/renderer/media/media_stream_impl.h:109: const WebKit::WebString& wk_device_id) OVERRIDE; What is a user_media_request here? Would it make sense to just specify the new type source request in WebUserMediaRequest and use the existing requestUserMedia? Ie class WebUserMediaRequest { --- WEBKIT_EXPORT bool audio() const; WEBKIT_EXPORT bool video() const; WEBKIT_EXPORT bool screen_share() const; }
Updated the patch set. I think it might be a little more clear now since miu@'s patched landed a few weeks ago and the methods on the browser side are there now. The idea is to have a different implementation of VideoCaptureDevice for device media streams (e.g. web_contents/tab video capture device). We want to be able to use these streams with webRTC. I think this is ready for review. Please take a look, thanks! On 2012/09/07 11:41:48, perkj wrote: > Cool- > I am afraid I can't help you much on the extension parts. Is the idea that the > screen share will implement the camera interface in the browser process and > behave as USB camera or how will webrtc and normal rendering be able to get the > video buffers? > > Do you have some more info on how this will fit together? > > https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... > File content/renderer/media/media_stream_impl.cc (right): > > https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... > content/renderer/media/media_stream_impl.cc:180: const std::string& device_id) { > unused? > > https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... > content/renderer/media/media_stream_impl.cc:204: > media_stream_dispatcher_->GenerateStream( > what is the difference between these two cases? > > https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... > File content/renderer/media/media_stream_impl.h (right): > > https://chromiumcodereview.appspot.com/10928043/diff/2001/content/renderer/me... > content/renderer/media/media_stream_impl.h:109: const WebKit::WebString& > wk_device_id) OVERRIDE; > What is a user_media_request here? Would it make sense to just specify the new > type source request in WebUserMediaRequest and use the existing > requestUserMedia? > > Ie > > class WebUserMediaRequest { > --- > WEBKIT_EXPORT bool audio() const; > WEBKIT_EXPORT bool video() const; > WEBKIT_EXPORT bool screen_share() const; > }
I would recommend you try to split this cl. http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:221: request.state[options.video_type] = Why is there two ways of setting the RequestState? Should this also use UpdateRequestState http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:410: void MediaStreamManager::UpdateRequestState( No need to belong to MediaStreamManager. Can be static http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:810: void MediaStreamManager::NotifyObserverRequestStateChange( No need to belong to MediaStreamManager- can be static. http://codereview.chromium.org/10928043/diff/14001/content/public/renderer/re... File content/public/renderer/render_view.h (right): http://codereview.chromium.org/10928043/diff/14001/content/public/renderer/re... content/public/renderer/render_view.h:121: virtual WebKit::WebUserMediaClient *GetUserMediaClient() = 0; no way of getting hold of RenderViewImpl instead and use the existing method? http://codereview.chromium.org/10928043/diff/14001/content/renderer/render_vi... File content/renderer/render_view_impl.h (right): http://codereview.chromium.org/10928043/diff/14001/content/renderer/render_vi... content/renderer/render_view_impl.h:726: virtual WebKit::WebUserMediaClient *GetUserMediaClient() OVERRIDE; virtual WebKit::WebUserMediaClient* GetUserMediaClient() Why do you need this? userMediaClient() is in this header file.
On 2012/09/27 13:21:48, perkj wrote: > I would recommend you try to split this cl. > > http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... > File content/browser/renderer_host/media/media_stream_manager.cc (right): > > http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:221: > request.state[options.video_type] = > Why is there two ways of setting the RequestState? Should this also use > UpdateRequestState > > http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:410: void > MediaStreamManager::UpdateRequestState( > No need to belong to MediaStreamManager. Can be static > > http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:810: void > MediaStreamManager::NotifyObserverRequestStateChange( > No need to belong to MediaStreamManager- can be static. > > http://codereview.chromium.org/10928043/diff/14001/content/public/renderer/re... > File content/public/renderer/render_view.h (right): > > http://codereview.chromium.org/10928043/diff/14001/content/public/renderer/re... > content/public/renderer/render_view.h:121: virtual WebKit::WebUserMediaClient > *GetUserMediaClient() = 0; > no way of getting hold of RenderViewImpl instead and use the existing method? > > http://codereview.chromium.org/10928043/diff/14001/content/renderer/render_vi... > File content/renderer/render_view_impl.h (right): > > http://codereview.chromium.org/10928043/diff/14001/content/renderer/render_vi... > content/renderer/render_view_impl.h:726: virtual WebKit::WebUserMediaClient > *GetUserMediaClient() OVERRIDE; > virtual WebKit::WebUserMediaClient* GetUserMediaClient() > > Why do you need this? > userMediaClient() is in this header file. Thanks! Split this CL as requested. Please take another look.
http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:221: request.state[options.video_type] = On 2012/09/27 13:21:48, perkj wrote: > Why is there two ways of setting the RequestState? Should this also use > UpdateRequestState Done. Missed this one. Made the field private so as not to miss any. http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:410: void MediaStreamManager::UpdateRequestState( On 2012/09/27 13:21:48, perkj wrote: > No need to belong to MediaStreamManager. Can be static Done. http://codereview.chromium.org/10928043/diff/14001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:810: void MediaStreamManager::NotifyObserverRequestStateChange( On 2012/09/27 13:21:48, perkj wrote: > No need to belong to MediaStreamManager- can be static. Done. http://codereview.chromium.org/10928043/diff/14001/content/public/renderer/re... File content/public/renderer/render_view.h (right): http://codereview.chromium.org/10928043/diff/14001/content/public/renderer/re... content/public/renderer/render_view.h:121: virtual WebKit::WebUserMediaClient *GetUserMediaClient() = 0; On 2012/09/27 13:21:48, perkj wrote: > no way of getting hold of RenderViewImpl instead and use the existing method? Done. Don't need this stuff anymore thanks to the WebUserMediaConstraints. http://codereview.chromium.org/10928043/diff/14001/content/renderer/render_vi... File content/renderer/render_view_impl.h (right): http://codereview.chromium.org/10928043/diff/14001/content/renderer/render_vi... content/renderer/render_view_impl.h:726: virtual WebKit::WebUserMediaClient *GetUserMediaClient() OVERRIDE; On 2012/09/27 13:21:48, perkj wrote: > virtual WebKit::WebUserMediaClient* GetUserMediaClient() > > Why do you need this? > userMediaClient() is in this header file. Done.
Sorry - I missed this. xians is better at the MediaStreamManager. I am not an owner so you need wjia as well. http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... File chrome/browser/media/media_internals_observer.h (right): http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... chrome/browser/media/media_internals_observer.h:17: virtual void OnRequestUpdate( I don't understand this. What is it doing and why? http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:76: // TODO(xians): Merge DeviceRequest with MediaStreamRequest. Is this todo fixed by this? http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:90: MediaStreamRequest::STATE_NOT_REQUESTED) { indentation http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:105: MediaStreamRequest::STATE_NOT_REQUESTED) { indentation http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:126: } empty line before privat: section http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:137: content::GetContentClient()->browser()->GetMediaObserver(); I am not familiar with GetMediaObserver. Why is this necessary? http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:863: MediaStreamRequest::STATE_DONE || Fits on line above, here and elsewhere? http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... File content/public/common/media_stream_request.h (right): http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... content/public/common/media_stream_request.h:18: // MediaStreamConstraint keys. ? This does not seem to have anything todo with constraints? http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:121: bool isTabCapture = false; Can this whole thing be broken out to a new function? Something like if IsScreenCapture(user_media_request) { stream_source_id = GetStreamSourceId(user_media_request); .. } http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:131: UTF8ToUTF16(content::kMediaStreamSource), audioSource); indentation http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:148: UTF8ToUTF16(content::kMediaStreamSourceId), source_id); indentation http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:165: UTF16ToUTF8(source_id), The source_id should be per StreamComponent in StreamOptions so you can specify both the audio and video source id? And then you would not need two GenerateStream methods. http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:278: audio_array.size()); indentation - should be 4 if you break a line. Here and elsewere.
http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... File content/public/common/media_stream_request.h (right): http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... content/public/common/media_stream_request.h:18: // MediaStreamConstraint keys. On 2012/10/04 08:19:25, perkj wrote: > ? This does not seem to have anything todo with constraints? oh - I see. But media_stream_request.h is not used in the render process. Ie - none of the types below are used. Move these to media_stream_options.h and please add a comment about what it is.
Great, thanks for the comments! I'll be more careful with style. And, sorry, to clarify a bit, the observer changes are used in the related CL mentioned in the description (design doc is in that CL too): http://codereview.chromium.org/11038021/diff/3001/chrome/browser/extensions/a... We want to be able to track state changes to a MediaStreamRequest from extension API's (so that extension writers can update their UI and such accordingly). There is one thing though which may come up, and I haven't figure how to do yet, which is that we probably want these new MediaStreamConstraints (mediaSource{id}) to be only usable from an extension API for now. Not sure if you have any suggestions for how to do that. Alternatively, protect this behind a flag, but that may be suboptimal. On 2012/10/04 08:19:25, perkj wrote: > Sorry - I missed this. > xians is better at the MediaStreamManager. I am not an owner so you need wjia as > well. > > http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... > File chrome/browser/media/media_internals_observer.h (right): > > http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... > chrome/browser/media/media_internals_observer.h:17: virtual void > OnRequestUpdate( > I don't understand this. What is it doing and why? > > http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... > File content/browser/renderer_host/media/media_stream_manager.cc (right): > > http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:76: // TODO(xians): > Merge DeviceRequest with MediaStreamRequest. > Is this todo fixed by this? > > http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:90: > MediaStreamRequest::STATE_NOT_REQUESTED) { > indentation > > http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:105: > MediaStreamRequest::STATE_NOT_REQUESTED) { > indentation > > http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:126: } > empty line before privat: section > > http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:137: > content::GetContentClient()->browser()->GetMediaObserver(); > I am not familiar with GetMediaObserver. Why is this necessary? > > http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... > content/browser/renderer_host/media/media_stream_manager.cc:863: > MediaStreamRequest::STATE_DONE || > Fits on line above, here and elsewhere? > > http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... > File content/public/common/media_stream_request.h (right): > > http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... > content/public/common/media_stream_request.h:18: // MediaStreamConstraint keys. > ? This does not seem to have anything todo with constraints? > > http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... > File content/renderer/media/media_stream_impl.cc (right): > > http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:121: bool isTabCapture = false; > Can this whole thing be broken out to a new function? > Something like > if IsScreenCapture(user_media_request) { > stream_source_id = GetStreamSourceId(user_media_request); > .. > } > > http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:131: > UTF8ToUTF16(content::kMediaStreamSource), audioSource); > indentation > > http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:148: > UTF8ToUTF16(content::kMediaStreamSourceId), source_id); > indentation > > http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:165: UTF16ToUTF8(source_id), > The source_id should be per StreamComponent in StreamOptions so you can specify > both the audio and video source id? And then you would not need two > GenerateStream methods. > > http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:278: audio_array.size()); > indentation - should be 4 if you break a line. Here and elsewere.
Hi Justin, I took a quick look, and found quite some questions/problems on the code. Since Some of them is partially because the messy code base in media_stream_manager, I wonder if you can wait for a few days? I am in the middle of doing a big refactoring on the media_stream_manager and the relevant UI code, and it will be much easier for you to work on after I land the CL. http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... File chrome/browser/media/media_internals.cc (right): http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... chrome/browser/media/media_internals.cc:80: OnMediaRequestStateChange(render_process_id, render_view_id, *it, Do we send the StateChange notification for all the opened/closed devices? And how the extensions figure out if those devices are what they are interested in? http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... chrome/browser/media/media_internals.cc:101: void MediaInternals::OnMediaRequestStateChange( OnMediaRequestStateChanged http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:104: state_(content::NUM_MEDIA_TYPES, This is a struct, use state or make the struct into class. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:120: void setState(int index, MediaStreamRequest::RequestState newState) { Oh, since you are adding functions to this struct, we should make it a class instead. Otherwise, keep the old way. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:143: content::MediaStreamDevice(stream_type,device_id, ""), one space http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:154: NotifyObserverRequestStateChange(request, stream_type, newState, device_id); It does not gain anything to break it into two functions. Merge them into one instead. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:511: } add back one empty line http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:626: UpdateRequestState(&it->second, stream_type, Are you sure you want this? The request might not from GenerateStreamForDevice http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.h (right): http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.h:90: struct DeviceRequest; Do we have to do this?
Hi, thanks for the quick looks! Addressed the comments. Please take another look. xians: I took a quick look at your other CL, and I don't think it interferes or helps much for this one, from what I can tell. Answered some of your questions in the replies. http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... File chrome/browser/media/media_internals.cc (right): http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... chrome/browser/media/media_internals.cc:80: OnMediaRequestStateChange(render_process_id, render_view_id, *it, On 2012/10/04 18:41:49, xians1 wrote: > Do we send the StateChange notification for all the opened/closed devices? > And how the extensions figure out if those devices are what they are interested > in? The extension will listen to all the TAB_MEDIA type stream changes. Actually, only VIDEO_TAB_MEDIA right now since audio is not ready yet and otherwise we will get dupes. Moved this loop back into media_stream_manager.cc http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... chrome/browser/media/media_internals.cc:101: void MediaInternals::OnMediaRequestStateChange( On 2012/10/04 18:41:49, xians1 wrote: > OnMediaRequestStateChanged Done. http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... File chrome/browser/media/media_internals_observer.h (right): http://codereview.chromium.org/10928043/diff/28001/chrome/browser/media/media... chrome/browser/media/media_internals_observer.h:17: virtual void OnRequestUpdate( On 2012/10/04 08:19:25, perkj wrote: > I don't understand this. What is it doing and why? It's to allow chrome extensions to listen to stream change notifications, specifically for tab media streams. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:76: // TODO(xians): Merge DeviceRequest with MediaStreamRequest. On 2012/10/04 08:19:25, perkj wrote: > Is this todo fixed by this? Not quite. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:90: MediaStreamRequest::STATE_NOT_REQUESTED) { On 2012/10/04 08:19:25, perkj wrote: > indentation Done. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:104: state_(content::NUM_MEDIA_TYPES, On 2012/10/04 18:41:49, xians1 wrote: > This is a struct, use state or make the struct into class. Done. It's kind of in an awkward state now, not sure if it's better to fully "class"-ify this or keep it like it was before but be careful with changing the state variable (i.e. always call notify after modifying state). http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:105: MediaStreamRequest::STATE_NOT_REQUESTED) { On 2012/10/04 08:19:25, perkj wrote: > indentation Done. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:120: void setState(int index, MediaStreamRequest::RequestState newState) { On 2012/10/04 18:41:49, xians1 wrote: > Oh, since you are adding functions to this struct, we should make it a class > instead. Otherwise, keep the old way. Done. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:126: } On 2012/10/04 08:19:25, perkj wrote: > empty line before privat: section Done. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:137: content::GetContentClient()->browser()->GetMediaObserver(); On 2012/10/04 08:19:25, perkj wrote: > I am not familiar with GetMediaObserver. Why is this necessary? Same as the other methods that notify observers. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:143: content::MediaStreamDevice(stream_type,device_id, ""), On 2012/10/04 18:41:49, xians1 wrote: > one space Done. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:154: NotifyObserverRequestStateChange(request, stream_type, newState, device_id); On 2012/10/04 18:41:49, xians1 wrote: > It does not gain anything to break it into two functions. Merge them into one > instead. Done. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:511: } On 2012/10/04 18:41:49, xians1 wrote: > add back one empty line Done. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:626: UpdateRequestState(&it->second, stream_type, On 2012/10/04 18:41:49, xians1 wrote: > Are you sure you want this? > The request might not from GenerateStreamForDevice Yes, we filter by the stream type later in the extension. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:863: MediaStreamRequest::STATE_DONE || On 2012/10/04 08:19:25, perkj wrote: > Fits on line above, here and elsewhere? Done. http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.h (right): http://codereview.chromium.org/10928043/diff/28001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.h:90: struct DeviceRequest; On 2012/10/04 18:41:49, xians1 wrote: > Do we have to do this? Yes, otherwise the statics cannot access this struct. http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... File content/public/common/media_stream_request.h (right): http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... content/public/common/media_stream_request.h:18: // MediaStreamConstraint keys. On 2012/10/04 08:19:25, perkj wrote: > ? This does not seem to have anything todo with constraints? Done. http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... content/public/common/media_stream_request.h:18: // MediaStreamConstraint keys. On 2012/10/04 08:24:59, perkj wrote: > On 2012/10/04 08:19:25, perkj wrote: > > ? This does not seem to have anything todo with constraints? > > oh - I see. But media_stream_request.h is not used in the render process. Ie - > none of the types below are used. Move these to media_stream_options.h and > please add a comment about what it is. Done. http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:121: bool isTabCapture = false; On 2012/10/04 08:19:25, perkj wrote: > Can this whole thing be broken out to a new function? > Something like > if IsScreenCapture(user_media_request) { > stream_source_id = GetStreamSourceId(user_media_request); > .. > } I cleaned this up a bit with helper function. Do you think it should be separated further into more functions? http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:131: UTF8ToUTF16(content::kMediaStreamSource), audioSource); On 2012/10/04 08:19:25, perkj wrote: > indentation Done. http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:148: UTF8ToUTF16(content::kMediaStreamSourceId), source_id); On 2012/10/04 08:19:25, perkj wrote: > indentation Done. http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:165: UTF16ToUTF8(source_id), On 2012/10/04 08:19:25, perkj wrote: > The source_id should be per StreamComponent in StreamOptions so you can specify > both the audio and video source id? And then you would not need two > GenerateStream methods. I think we still may need separate GenerateStream methods because the behavior is slightly different (i.e. TabCapture should bypass the user prompt but have a different way to notify user). It might make sense to have separate device_id for audio/video, but the API currently only lets you specify one id (does not really make sense right now to grab audio/video from different tabs), so it maybe OK for not to not have it per component. http://codereview.chromium.org/10928043/diff/28001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:278: audio_array.size()); On 2012/10/04 08:19:25, perkj wrote: > indentation - should be 4 if you break a line. Here and elsewere. Reverted this.
http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... File content/public/common/media_stream_request.h (right): http://codereview.chromium.org/10928043/diff/28001/content/public/common/medi... content/public/common/media_stream_request.h:18: // MediaStreamConstraint keys. On 2012/10/04 08:24:59, perkj wrote: > On 2012/10/04 08:19:25, perkj wrote: > > ? This does not seem to have anything todo with constraints? > > oh - I see. But media_stream_request.h is not used in the render process. Ie - > none of the types below are used. Move these to media_stream_options.h and > please add a comment about what it is. Actually, I need access to access these from chrome/, would it make sense to move mode_stream_request.h into the public interface of content/? Or, should I move these 2 constants back to media_stream_options?
I have only looked at the MediaStreamImpl changes. http://codereview.chromium.org/10928043/diff/36001/content/browser/renderer_h... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/10928043/diff/36001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_manager.cc:176: vc_device_name.unique_id = "/dev/video1"; needed? http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:13: #include "content/public/common/media_stream_request.h" Get rid of this include. It does not belong here. http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:44: const WebKit::WebMediaConstraints constraints, const std::string& key) { const WebKit::WebMediaConstraints& http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:137: media_stream::kMediaStreamSource) == "tab") { define "tab" as media_stream::kMediaStreamSourceTab to remove magic string? http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:151: DVLOG(1) << "Tried to use tab capture outside extension API."; Change to error log http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:159: if (audio == content::MEDIA_TAB_AUDIO_CAPTURE || So it looks like you need video == content::MEDIA_TAB_VIDEO_CAPTURE otherwise GetMandatoryStreamConstraint(user_media_request.videoConstraints() will not return something valid. Why do you check the media_stream::kMediaStreamSource constraint for both audio and video and then have this if statement audio || video I still think this would be a lot cleaner if you add one or two strings to media_stream::StreamOptions for the device_id and got rid of the GenerateStreamForDevice from the MediaStreamDispatcher. Ie.. struct CONTENT_EXPORT StreamOptions { ... MediaStreamType audio_type; std::string audio_device_id; MediaStreamType video_type; std::string video_device_id; } In MediaStreamDispatcherHost you can take appropriate action based on the audio_type and video_type and the device_id. };
Agree with perkj@ on the changes to struct StreamOptions (and getting rid of
GenerateStreamForDevice). Now that I've thought about things more, I don't
think we ever needed a separate GenerateStreamForDevice() call. We can simply
pass in empty strings for device_ids to let the user choose in the infobar
pop-up (i.e., the behavior of GenerateStream()).
On 2012/10/09 11:10:02, perkj wrote:
> struct CONTENT_EXPORT StreamOptions {
> ...
> MediaStreamType audio_type;
> std::string audio_device_id;
>
> MediaStreamType video_type;
> std::string video_device_id;
> }
Thanks, please take another look. Note that getting rid of GenerateStreamForDevice altogether is a pretty big task, and I don't think it makes sense to do it in this CL. On 2012/10/09 11:10:02, perkj wrote: > I have only looked at the MediaStreamImpl changes. > > http://codereview.chromium.org/10928043/diff/36001/content/browser/renderer_h... > File content/browser/renderer_host/media/video_capture_manager.cc (right): > > http://codereview.chromium.org/10928043/diff/36001/content/browser/renderer_h... > content/browser/renderer_host/media/video_capture_manager.cc:176: > vc_device_name.unique_id = "/dev/video1"; > needed? > > http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... > File content/renderer/media/media_stream_impl.cc (right): > > http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:13: #include > "content/public/common/media_stream_request.h" > Get rid of this include. It does not belong here. > > http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:44: const > WebKit::WebMediaConstraints constraints, const std::string& key) { > const WebKit::WebMediaConstraints& > > http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:137: > media_stream::kMediaStreamSource) == "tab") { > define "tab" as media_stream::kMediaStreamSourceTab to remove magic string? > > http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:151: DVLOG(1) << "Tried to use tab > capture outside extension API."; > Change to error log > > http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... > content/renderer/media/media_stream_impl.cc:159: if (audio == > content::MEDIA_TAB_AUDIO_CAPTURE || > So it looks like you need video == content::MEDIA_TAB_VIDEO_CAPTURE otherwise > GetMandatoryStreamConstraint(user_media_request.videoConstraints() will not > return something valid. > Why do you check the media_stream::kMediaStreamSource constraint for both audio > and video and then have this if statement audio || video > > I still think this would be a lot cleaner if you add one or two strings to > media_stream::StreamOptions for the device_id and got rid of the > GenerateStreamForDevice from the MediaStreamDispatcher. > Ie.. > struct CONTENT_EXPORT StreamOptions { > ... > MediaStreamType audio_type; > std::string audio_device_id; > > MediaStreamType video_type; > std::string video_device_id; > } > > In MediaStreamDispatcherHost you can take appropriate action based on the > audio_type and video_type and the device_id. > > > };
http://codereview.chromium.org/10928043/diff/36001/content/browser/renderer_h... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/10928043/diff/36001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_manager.cc:176: vc_device_name.unique_id = "/dev/video1"; On 2012/10/09 11:10:02, perkj wrote: > needed? Yes, for now otherwise the fake device returns null. miu@ will have the implementation for this and replace it soon. http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:13: #include "content/public/common/media_stream_request.h" On 2012/10/09 11:10:02, perkj wrote: > Get rid of this include. It does not belong here. Done. http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:44: const WebKit::WebMediaConstraints constraints, const std::string& key) { On 2012/10/09 11:10:02, perkj wrote: > const WebKit::WebMediaConstraints& Done. http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:137: media_stream::kMediaStreamSource) == "tab") { On 2012/10/09 11:10:02, perkj wrote: > define "tab" as media_stream::kMediaStreamSourceTab to remove magic string? Done. http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:151: DVLOG(1) << "Tried to use tab capture outside extension API."; On 2012/10/09 11:10:02, perkj wrote: > Change to error log Done. http://codereview.chromium.org/10928043/diff/36001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:159: if (audio == content::MEDIA_TAB_AUDIO_CAPTURE || On 2012/10/09 11:10:02, perkj wrote: > So it looks like you need video == content::MEDIA_TAB_VIDEO_CAPTURE otherwise > GetMandatoryStreamConstraint(user_media_request.videoConstraints() will not > return something valid. > Why do you check the media_stream::kMediaStreamSource constraint for both audio > and video and then have this if statement audio || video > > I still think this would be a lot cleaner if you add one or two strings to > media_stream::StreamOptions for the device_id and got rid of the > GenerateStreamForDevice from the MediaStreamDispatcher. > Ie.. > struct CONTENT_EXPORT StreamOptions { > ... > MediaStreamType audio_type; > std::string audio_device_id; > > MediaStreamType video_type; > std::string video_device_id; > } > > In MediaStreamDispatcherHost you can take appropriate action based on the > audio_type and video_type and the device_id. > > > }; > > > Done.
On 2012/10/10 23:37:43, miu wrote:
> Agree with perkj@ on the changes to struct StreamOptions (and getting rid of
> GenerateStreamForDevice). Now that I've thought about things more, I don't
> think we ever needed a separate GenerateStreamForDevice() call. We can simply
> pass in empty strings for device_ids to let the user choose in the infobar
> pop-up (i.e., the behavior of GenerateStream()).
>
> On 2012/10/09 11:10:02, perkj wrote:
> > struct CONTENT_EXPORT StreamOptions {
> > ...
> > MediaStreamType audio_type;
> > std::string audio_device_id;
> >
> > MediaStreamType video_type;
> > std::string video_device_id;
> > }
Right - or maybe even better - let the behavior be based on the the video_type
or audio_type.
The MediaStreamImpl things start to look good. I realized there is one leak in there. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_dispatcher.cc:77: if (components.audio_type == content::MEDIA_TAB_AUDIO_CAPTURE || Why did you you keep MediaStreamHostMsg_GenerateStreamForDevice? If you want to clean it up in a separate cl that is fine. Just add a todo with your name. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:166: bool isTabMediaRequest = CheckAndUpdateAsTabMediaRequest(user_media_request, camel case is not used in chrome. change to is_tab_media_request. or if(CheckAndUpdate....) { http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:173: return; I realized you can not just return here. You need to complete the |user_media_request|. Since the URL is sent down via the dispatcher- would it make sence to keep all decisions wheater the request succceeds or fails in the browser process? In a not to far future we would like to allow this in ordinary JS as well. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:185: media_stream_dispatcher_->GenerateStream( nice
Hi, sorry for the late review, I have been busy with some M23 bugs. It looks much better now, but I still have some confusions, please address the comments. A meeting sounds great, please schedule it. http://codereview.chromium.org/10928043/diff/45001/chrome/browser/media/media... File chrome/browser/media/media_internals_observer.h (right): http://codereview.chromium.org/10928043/diff/45001/chrome/browser/media/media... chrome/browser/media/media_internals_observer.h:15: virtual void OnUpdate(const string16& javascript) {} add an empty line to separate the function and comment. http://codereview.chromium.org/10928043/diff/45001/chrome/browser/media/media... chrome/browser/media/media_internals_observer.h:20: virtual ~MediaInternalsObserver() {} ditto http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:77: class MediaStreamManager::DeviceRequest { Nice, thanks. http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:149: request->requested_device_id, Do we still notify the observer if requested_device_id is empty? http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:253: UpdateRequestState(&request, options.audio_type, Are you sure that you want to update the observer_ consecutively? I simply think STATE_REQUESTED should not be used here. http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:268: DevicesAccepted(*label, devices); ?? It is bypassing the infobar, don't you need users' approval on the access of the devices? http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:322: UpdateRequestState(&it->second, i, MediaStreamRequest::STATE_CLOSING); I think I have already asked the question before, but I might miss your feedback, please correct me if you have already answered. Lots code is shared by GenerateStream and GenerateStreamForDevice or other types of requests, do you want to update the observer if it is not a TAB request? http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:45: return ""; I know both ways work, but what about using std::string()? http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:54: content::MediaStreamDeviceType* audio, pass a StreamOptions* as input param here. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:60: media_stream::kMediaStreamSource) == odd indentation. if it fits 80 chars, it should be GetMandatoryStreamConstraint(user_media_request.audioConstraints(), edia_stream::kMediaStreamSource) == http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:70: media_stream::kMediaStreamSource) == ditto http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:138: content::MediaStreamDeviceType audio = content::MEDIA_NO_SERVICE; we should simply use a media_stream::StreamOptions here to make the code clean as well as save these local variables.
Thanks for the reviews! Please take another look. http://codereview.chromium.org/10928043/diff/45001/chrome/browser/media/media... File chrome/browser/media/media_internals_observer.h (right): http://codereview.chromium.org/10928043/diff/45001/chrome/browser/media/media... chrome/browser/media/media_internals_observer.h:15: virtual void OnUpdate(const string16& javascript) {} On 2012/10/11 09:07:25, xians1 wrote: > add an empty line to separate the function and comment. Done. http://codereview.chromium.org/10928043/diff/45001/chrome/browser/media/media... chrome/browser/media/media_internals_observer.h:20: virtual ~MediaInternalsObserver() {} On 2012/10/11 09:07:25, xians1 wrote: > ditto Done. http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:149: request->requested_device_id, On 2012/10/11 09:07:25, xians1 wrote: > Do we still notify the observer if requested_device_id is empty? Done. We could short-circuit it here with that condition if you'd like since we would be the only ones using this now. http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:253: UpdateRequestState(&request, options.audio_type, On 2012/10/11 09:07:25, xians1 wrote: > Are you sure that you want to update the observer_ consecutively? I simply think > STATE_REQUESTED should not be used here. We might remove the "requested" state notification if it doesn't make sense to have it later, but I think the observers should be notified of entering this "state". http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:268: DevicesAccepted(*label, devices); On 2012/10/11 09:07:25, xians1 wrote: > ?? It is bypassing the infobar, don't you need users' approval on the access of > the devices? Yes, I think we eventually want to add a notification infobar that will tell the user a current tab is being captured in the captured tab. We can't reuse the approval infobar right now because the media request is typically made from the context of an extension's background page (which has no visible UI). Long story short, this is needed because if we do it in the context of the current page, the stream gets lost if the user changes the URL, but we want the capture to follow URL changes. So, we want to bypass for now. We can discuss this in meeting if needed. http://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:322: UpdateRequestState(&it->second, i, MediaStreamRequest::STATE_CLOSING); On 2012/10/11 09:07:25, xians1 wrote: > I think I have already asked the question before, but I might miss your > feedback, please correct me if you have already answered. > Lots code is shared by GenerateStream and GenerateStreamForDevice or other types > of requests, do you want to update the observer if it is not a TAB request? Right, for now probably not. Added short-circuit in the UpdateRequestState function like you suggested in a previous comment so it doesn't notify if there's no device_id. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_dispatcher.cc:77: if (components.audio_type == content::MEDIA_TAB_AUDIO_CAPTURE || On 2012/10/11 08:01:54, perkj wrote: > Why did you you keep MediaStreamHostMsg_GenerateStreamForDevice? If you want to > clean it up in a separate cl that is fine. Just add a todo with your name. Done. Moved it to the host part and added TODO there. There's more plumbing to be done to get rid of it completely.. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:45: return ""; On 2012/10/11 09:07:25, xians1 wrote: > I know both ways work, but what about using std::string()? Done. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:54: content::MediaStreamDeviceType* audio, On 2012/10/11 09:07:25, xians1 wrote: > pass a StreamOptions* as input param here. Done. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:60: media_stream::kMediaStreamSource) == On 2012/10/11 09:07:25, xians1 wrote: > odd indentation. if it fits 80 chars, it should be > GetMandatoryStreamConstraint(user_media_request.audioConstraints(), > edia_stream::kMediaStreamSource) == Done. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:70: media_stream::kMediaStreamSource) == On 2012/10/11 09:07:25, xians1 wrote: > ditto Done. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:138: content::MediaStreamDeviceType audio = content::MEDIA_NO_SERVICE; On 2012/10/11 09:07:25, xians1 wrote: > we should simply use a media_stream::StreamOptions here to make the code clean > as well as save these local variables. Done. Thanks, looks better. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:166: bool isTabMediaRequest = CheckAndUpdateAsTabMediaRequest(user_media_request, On 2012/10/11 08:01:54, perkj wrote: > camel case is not used in chrome. change to is_tab_media_request. > > or if(CheckAndUpdate....) { Done. http://codereview.chromium.org/10928043/diff/45001/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:173: return; On 2012/10/11 08:01:54, perkj wrote: > I realized you can not just return here. You need to complete the > |user_media_request|. Since the URL is sent down via the dispatcher- would it > make sence to keep all decisions wheater the request succceeds or fails in the > browser process? > In a not to far future we would like to allow this in ordinary JS as well. Done. Thanks, this is cleaner.
https://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:135: static void UpdateRequestState( I guess you need to call this function only when request is related to tab capture. If you can guarantee the request->requested_device_id will not overlap between regular device capture and tab capture, you can check the device_id inside media_observer->OnMediaRequestStateChanged(). Otherwise, you need a way to distinguish device capture and tab capture requests, such as new types in RequestType. https://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:268: DevicesAccepted(*label, devices); you need post a task, instead of calling DevicesAccepted directly, since the requester might not have a chance to register the label yet (check media_stream_dispatcher_host.cc).
Done, PTAL thanks! https://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:135: static void UpdateRequestState( On 2012/10/11 20:21:07, wjia wrote: > I guess you need to call this function only when request is related to tab > capture. If you can guarantee the request->requested_device_id will not overlap > between regular device capture and tab capture, you can check the device_id > inside media_observer->OnMediaRequestStateChanged(). Otherwise, you need a way > to distinguish device capture and tab capture requests, such as new types in > RequestType. Done. Changed the check to just use options.{video,audio}_type instead as discussed. https://codereview.chromium.org/10928043/diff/45001/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:268: DevicesAccepted(*label, devices); On 2012/10/11 20:21:07, wjia wrote: > you need post a task, instead of calling DevicesAccepted directly, since the > requester might not have a chance to register the label yet (check > media_stream_dispatcher_host.cc). Done. Thanks.
Nice. Last round from me, hopefully. :-) https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_dispatcher_host.cc (right): https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... content/browser/renderer_host/media/media_stream_dispatcher_host.cc:191: void MediaStreamDispatcherHost::OnGenerateStreamForDevice( You can remove this handler function since the corresponding message has been removed. https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:149: if (request->options.video_type != content::MEDIA_TAB_VIDEO_CAPTURE && could you merge these 2 if's? https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:337: for (MediaStreamType i = content::MEDIA_NO_SERVICE; you probably want to start with content::MEDIA_NO_SERVICE + 1 here. https://codereview.chromium.org/10928043/diff/42003/content/renderer/media/me... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/10928043/diff/42003/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:158: UpdateOptionsIfTabMediaRequest(user_media_request, &options); Do we need any check after this function returns? E.g., DEVICE_*_CAPTURE will not pair with TAB_*_CAPTURE?
thanks again! ptal http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_dispatcher_host.cc (right): http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_dispatcher_host.cc:191: void MediaStreamDispatcherHost::OnGenerateStreamForDevice( On 2012/10/11 22:13:27, wjia wrote: > You can remove this handler function since the corresponding message has been > removed. Done. http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:149: if (request->options.video_type != content::MEDIA_TAB_VIDEO_CAPTURE && On 2012/10/11 22:13:27, wjia wrote: > could you merge these 2 if's? Done. http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:253: CancelGenerateStream(*label); Changed this to also PostTask for the same reason as the other one. http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:337: for (MediaStreamType i = content::MEDIA_NO_SERVICE; On 2012/10/11 22:13:27, wjia wrote: > you probably want to start with content::MEDIA_NO_SERVICE + 1 here. Done. http://codereview.chromium.org/10928043/diff/42003/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/42003/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:158: UpdateOptionsIfTabMediaRequest(user_media_request, &options); On 2012/10/11 22:13:27, wjia wrote: > Do we need any check after this function returns? E.g., DEVICE_*_CAPTURE will > not pair with TAB_*_CAPTURE? Good observation! Right now, this can't really happen because the tab capture stuff can only be used from an extension API, which will force audio/video streams to tab type or none. Conversely, you can't use the tab stuff from a non-extension. Maybe, in the future we would want video and tab streams to be compatible (i.e. use tab video with mic audio?), but that would take some refactoring since we branch in the browser process depending on what was requested. Added a check in GenerateStreamForDevice just to future-proof this a bit.
lgtm LGTM. BTW--I've been asked in the past to run the following tests/scripts when changing code surrounding MediaStreamManager to make sure nothing's broken, and I think that'd be a good idea here. For your convenience, the commands I used were: cd .../src/ && ninja -C out/Debug chrome content_unittests peerconnection_server pyautolib chromedriver && out/Debug/content_unittests --gtest_filter='WebRTCAudioDeviceTest.*:MediaStream*:VideoCapture*:CaptureVideo*:AudioInputDeviceManagerTest.*' && LD_LIBRARY_PATH=out/Debug/lib:$LD_LIBRARY_PATH PYTHONPATH=out/Debug chrome/test/functional/webrtc_call.py && LD_LIBRARY_PATH=out/Debug/lib:$LD_LIBRARY_PATH PYTHONPATH=out/Debug chrome/test/functional/webrtc_brutality_test.py https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_dispatcher_host.cc (right): https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... content/browser/renderer_host/media/media_stream_dispatcher_host.cc:178: // TODO(justinlin): Cleanup/get rid of GenerateStreamForDevice and merge Please remember to file a bug for this. https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:139: static void UpdateRequestState( Is there a reason this isn't just a method in DeviceRequest? https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:878: !requested_audio || request.getState(request.options.audio_type) == This is a little confusing to read. Can you use more lines/whitespace so each expression of the logical ORing starts on a separate line? https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:887: !requested_video || request.getState(request.options.video_type) == Ditto on readability. https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_... content/browser/renderer_host/media/video_capture_manager.cc:176: // TODO(justinlin): This is so we don't get a null device. Remove later. Ah! A race to see which of our changes will land first (this will cause a minor merge conflict with my pending change 11065004). https://codereview.chromium.org/10928043/diff/42003/content/common/media/medi... File content/common/media/media_stream_options.cc (right): https://codereview.chromium.org/10928043/diff/42003/content/common/media/medi... content/common/media/media_stream_options.cc:44: } Consider adding DCHECKs, like the constructor above, and also device_ids here should probably be non-empty strings. https://codereview.chromium.org/10928043/diff/42003/content/renderer/media/me... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/10928043/diff/42003/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:58: media_stream::kMediaStreamSourceTab) { nit: Indentation. Consider putting this 4 chars to the right of GetMandatoryStreamConstraint's indentation. https://codereview.chromium.org/10928043/diff/42003/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:68: media_stream::kMediaStreamSourceTab) { nit: ditto on indentation. https://codereview.chromium.org/10928043/diff/42003/content/renderer/media/me... content/renderer/media/media_stream_impl.cc:161: << (options.audio_type ? "audio" : "") nit: Consider replacing these with the following so the logging is more useful: << "audio=" << options.audio_type << ", video=" << options.video_type << "], "
LGTM. Nice work!
Thanks for the instructions for running the tests! Fixed a few things with those. http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_dispatcher_host.cc (right): http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_dispatcher_host.cc:178: // TODO(justinlin): Cleanup/get rid of GenerateStreamForDevice and merge On 2012/10/12 01:10:50, Yuri wrote: > Please remember to file a bug for this. Done. http://code.google.com/p/chromium/issues/detail?id=155444 http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:139: static void UpdateRequestState( On 2012/10/12 01:10:50, Yuri wrote: > Is there a reason this isn't just a method in DeviceRequest? Good question. Done. http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:878: !requested_audio || request.getState(request.options.audio_type) == On 2012/10/12 01:10:50, Yuri wrote: > This is a little confusing to read. Can you use more lines/whitespace so each > expression of the logical ORing starts on a separate line? Done. http://codereview.chromium.org/10928043/diff/42003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:887: !requested_video || request.getState(request.options.video_type) == On 2012/10/12 01:10:50, Yuri wrote: > Ditto on readability. Done. http://codereview.chromium.org/10928043/diff/42003/content/common/media/media... File content/common/media/media_stream_options.cc (right): http://codereview.chromium.org/10928043/diff/42003/content/common/media/media... content/common/media/media_stream_options.cc:44: } On 2012/10/12 01:10:50, Yuri wrote: > Consider adding DCHECKs, like the constructor above, and also device_ids here > should probably be non-empty strings. Done. http://codereview.chromium.org/10928043/diff/42003/content/renderer/media/med... File content/renderer/media/media_stream_impl.cc (right): http://codereview.chromium.org/10928043/diff/42003/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:58: media_stream::kMediaStreamSourceTab) { On 2012/10/12 01:10:50, Yuri wrote: > nit: Indentation. Consider putting this 4 chars to the right of > GetMandatoryStreamConstraint's indentation. Done. http://codereview.chromium.org/10928043/diff/42003/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:68: media_stream::kMediaStreamSourceTab) { On 2012/10/12 01:10:50, Yuri wrote: > nit: ditto on indentation. Done. http://codereview.chromium.org/10928043/diff/42003/content/renderer/media/med... content/renderer/media/media_stream_impl.cc:161: << (options.audio_type ? "audio" : "") On 2012/10/12 01:10:50, Yuri wrote: > nit: Consider replacing these with the following so the logging is more useful: > > << "audio=" << options.audio_type > << ", video=" << options.video_type > << "], " Done. Thanks.
lgtm. Please consider remove constructors of StreamOptions that are not used in MediaStreamImpl. http://codereview.chromium.org/10928043/diff/55008/content/common/media/media... File content/common/media/media_stream_options.cc (right): http://codereview.chromium.org/10928043/diff/55008/content/common/media/media... content/common/media/media_stream_options.cc:36: StreamOptions::StreamOptions(MediaStreamType audio_type, Can you remove the other constructor? http://codereview.chromium.org/10928043/diff/55008/content/common/media/media... content/common/media/media_stream_options.cc:49: DCHECK(!video_device_id.empty()); Isn't this empty in normal case. http://codereview.chromium.org/10928043/diff/55008/content/common/media/media... File content/common/media/media_stream_options.h (left): http://codereview.chromium.org/10928043/diff/55008/content/common/media/media... content/common/media/media_stream_options.h:24: StreamOptions(bool user_audio, bool user_video); unused? Remove this. http://codereview.chromium.org/10928043/diff/55008/content/common/media/media... content/common/media/media_stream_options.h:25: StreamOptions(MediaStreamType audio_type, MediaStreamType video_type); Is this one necessary? Seems like default constructor can be used instead if I only look in MediaStreamImpl. http://codereview.chromium.org/10928043/diff/55008/content/common/media/media... File content/common/media/media_stream_options.h (right): http://codereview.chromium.org/10928043/diff/55008/content/common/media/media... content/common/media/media_stream_options.h:31: StreamOptions(MediaStreamType audio_type, std::string audio_device_id, Not used in MediaStreamImpl. Remove?
Two more nits. lgmt if you address them. http://codereview.chromium.org/10928043/diff/55001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10928043/diff/55001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:148: (request->options.video_type != content::MEDIA_TAB_VIDEO_CAPTURE && What happen if either audio_type or video_type == TAB, but not both? Also, please separate these checks. move request->options.video_type != content::MEDIA_TAB_VIDEO_CAPTURE && 149 request->options.audio_type != content::MEDIA_TAB_AUDIO_CAPTURE)) above content::MediaObserver* media_observer = ... Then we don't access observer unless we need to. http://codereview.chromium.org/10928043/diff/55001/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.h (right): http://codereview.chromium.org/10928043/diff/55001/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.h:90: class DeviceRequest; why expose it to public, can we keep it to private?
(just looked at content/public which is what I assume you wanted me to look at. for future reference, please tell reviewers which parts you want them to look at) https://codereview.chromium.org/10928043/diff/55008/content/public/browser/me... File content/public/browser/media_observer.h (right): https://codereview.chromium.org/10928043/diff/55008/content/public/browser/me... content/public/browser/media_observer.h:56: const content::MediaStreamDevice& device, nit: here and below (and also throughout the file) don't add "content::" https://codereview.chromium.org/10928043/diff/55008/content/public/common/med... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/10928043/diff/55008/content/public/common/med... content/public/common/media_stream_request.h:66: enum RequestState { why is the enum in this struct, it's not used in it? this seems like it should be in its own file, i.e. content/public/browser/media_request_state.h. the values should be REQUEST_STATE_foo
Yes, sorry will do that next time. Is it better to ask for public content api changes before or after the review is done. PTAL. https://codereview.chromium.org/10928043/diff/55001/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://codereview.chromium.org/10928043/diff/55001/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.cc:148: (request->options.video_type != content::MEDIA_TAB_VIDEO_CAPTURE && On 2012/10/12 14:00:59, xians1 wrote: > What happen if either audio_type or video_type == TAB, but not both? > > Also, please separate these checks. > move request->options.video_type != content::MEDIA_TAB_VIDEO_CAPTURE && > 149 request->options.audio_type != content::MEDIA_TAB_AUDIO_CAPTURE)) > above content::MediaObserver* media_observer = ... > > Then we don't access observer unless we need to. Not both is OK. It checks (if (!tabVideo && !tabAudio)), so either will be false. Done. https://codereview.chromium.org/10928043/diff/55001/content/browser/renderer_... File content/browser/renderer_host/media/media_stream_manager.h (right): https://codereview.chromium.org/10928043/diff/55001/content/browser/renderer_... content/browser/renderer_host/media/media_stream_manager.h:90: class DeviceRequest; On 2012/10/12 14:00:59, xians1 wrote: > why expose it to public, can we keep it to private? Done. Can move it back now that the static is gone. https://codereview.chromium.org/10928043/diff/55008/content/common/media/medi... File content/common/media/media_stream_options.cc (right): https://codereview.chromium.org/10928043/diff/55008/content/common/media/medi... content/common/media/media_stream_options.cc:36: StreamOptions::StreamOptions(MediaStreamType audio_type, On 2012/10/12 13:22:05, perkj wrote: > Can you remove the other constructor? Done. https://codereview.chromium.org/10928043/diff/55008/content/common/media/medi... content/common/media/media_stream_options.cc:49: DCHECK(!video_device_id.empty()); On 2012/10/12 13:22:05, perkj wrote: > Isn't this empty in normal case. Done. https://codereview.chromium.org/10928043/diff/55008/content/common/media/medi... File content/common/media/media_stream_options.h (left): https://codereview.chromium.org/10928043/diff/55008/content/common/media/medi... content/common/media/media_stream_options.h:24: StreamOptions(bool user_audio, bool user_video); On 2012/10/12 13:22:05, perkj wrote: > unused? Remove this. There's too many tests to update. I'll do it in a follow-up CL. https://codereview.chromium.org/10928043/diff/55008/content/common/media/medi... content/common/media/media_stream_options.h:25: StreamOptions(MediaStreamType audio_type, MediaStreamType video_type); On 2012/10/12 13:22:05, perkj wrote: > Is this one necessary? Seems like default constructor can be used instead if I > only look in MediaStreamImpl. Done. https://codereview.chromium.org/10928043/diff/55008/content/common/media/medi... File content/common/media/media_stream_options.h (right): https://codereview.chromium.org/10928043/diff/55008/content/common/media/medi... content/common/media/media_stream_options.h:31: StreamOptions(MediaStreamType audio_type, std::string audio_device_id, On 2012/10/12 13:22:05, perkj wrote: > Not used in MediaStreamImpl. Remove? Done. https://codereview.chromium.org/10928043/diff/55008/content/public/browser/me... File content/public/browser/media_observer.h (right): https://codereview.chromium.org/10928043/diff/55008/content/public/browser/me... content/public/browser/media_observer.h:56: const content::MediaStreamDevice& device, On 2012/10/12 17:21:41, John Abd-El-Malek wrote: > nit: here and below (and also throughout the file) don't add "content::" Done. https://codereview.chromium.org/10928043/diff/55008/content/public/common/med... File content/public/common/media_stream_request.h (right): https://codereview.chromium.org/10928043/diff/55008/content/public/common/med... content/public/common/media_stream_request.h:66: enum RequestState { On 2012/10/12 17:21:41, John Abd-El-Malek wrote: > why is the enum in this struct, it's not used in it? > > this seems like it should be in its own file, i.e. > content/public/browser/media_request_state.h. the values should be > REQUEST_STATE_foo Done. Sounds good to me.
lgtm with nit http://codereview.chromium.org/10928043/diff/73001/content/public/browser/med... File content/public/browser/media_request_state.h (right): http://codereview.chromium.org/10928043/diff/73001/content/public/browser/med... content/public/browser/media_request_state.h:11: REQUEST_STATE_NOT_REQUESTED = 0, nit: sorry I meant to say MEDIA_REQUEST_foo
Thanks! http://codereview.chromium.org/10928043/diff/73001/content/public/browser/med... File content/public/browser/media_request_state.h (right): http://codereview.chromium.org/10928043/diff/73001/content/public/browser/med... content/public/browser/media_request_state.h:11: REQUEST_STATE_NOT_REQUESTED = 0, On 2012/10/15 06:38:57, John Abd-El-Malek wrote: > nit: sorry I meant to say MEDIA_REQUEST_foo Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justinlin@chromium.org/10928043/79003
Change committed as 161881
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/10928043/diff/79003/content/browser/re... File content/browser/renderer_host/media/media_stream_manager.cc (right): https://chromiumcodereview.appspot.com/10928043/diff/79003/content/browser/re... content/browser/renderer_host/media/media_stream_manager.cc:32: const char kExtensionScheme[] = "chrome-extension"; I just saw this while looking through the code. this a layering violation, since content shouldn't know about chrome |
