Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6946001:
VideoCaptureManager opens/closes, starts/stops and enumerates video capture devices. VideoCapture... (Closed)
|
Created:
9 years, 7 months ago by mflodman1 Modified:
9 years, 6 months ago Reviewers:
wjia(left Chromium) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionVideoCaptureManager opens/closes, starts/stops and enumerates video capture devices. VideoCaptureManager Open/Close/EnumerateDevices will be used by MediaStreamManager (will be added later) to implement parts of WhatWG peer connection API. VideoCaptureHost (will also be added later) will call Start/Stop to controll the media flow.
A unit test is provided and added to chrome/chrome_tests.gypi.
content/browser/media_stream/media_stream_provider.h will later be inherited by an audio capture manager as well.
patch by mflodman@google.com
BUG=none
TEST=try bots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86511
Patch Set 1 #
Total comments: 48
Patch Set 2 : '' #Patch Set 3 : VideoCaptureManager.cc error check changes #
Total comments: 52
Patch Set 4 : '' #Patch Set 5 : Small change to unit test mock version of listener. #
Total comments: 4
Patch Set 6 : Unit test changs based on review by wjia. #Patch Set 7 : Removing unnecessary checks in unittest that might fail on try server #
Total comments: 12
Messages
Total messages: 14 (0 generated)
Commented on lint errors. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... File content/browser/media_stream/media_stream_provider.h (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... content/browser/media_stream/media_stream_provider.h:92: #endif // CONTENT_BROWSER_MEDIA_STREAM_MEDIA_STREAM_PROVIDER_H_ Will add newline in next upload http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager.cc (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:460: } // namespace media Will add newline in next upload. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager.h (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:21: #include "media/video/capture/video_capture_device.h" Will change include order or _types.h and device.h in next upload. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:142: #endif // CONTENT_BROWSER_MEDIA_STREAM_VIDEO_CAPTURE_MANAGER_H_ Will add newline in next upload. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager_unittest.cc:10: #include "base/memory/scoped_ptr.h" Will change include order of message_loop.h and meomry/scoped_ptr.h in next upload. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager_unittest.cc:278: } Will add newline character in next upload.
I will take another look at the unit test later. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... File content/browser/media_stream/media_stream_provider.h (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... content/browser/media_stream/media_stream_provider.h:13: typedef int MediaCaptureSessionId; Is this the same SessionId received by VideoCapture? We'd consolidate it in some way. Another one is in media/video/capture/video_capture_types.h http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... content/browser/media_stream/media_stream_provider.h:52: class MediaStreamProviderListener { Could you add comments on those API's? http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... content/browser/media_stream/media_stream_provider.h:89: }; one empty line here. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager.cc (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:163: device_names.get()->begin(); it != device_names.get()->end(); ++it) { one more space indent http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:189: // device is supported. looks like you can use DeviceOpened() here. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:194: if (vc_device != NULL) { if vc_device == NULL here, is it an error? http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:266: device_names.get()->clear(); clear() is not needed. scoped_ptr should take care of this. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:277: // Possible errors are signaled to host comment doesn't match. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:293: // Possible errors are signaled by video_capture_device comment doesn't match. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:362: capture_session_id)); there is no need for On*Proxy. You can use: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, NewRunnableMethod(this, &VideoCaptureManager::OnOpened, capture_session_id)); http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager.h (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:12: #pragma once pragma is not needed. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:29: // not been called. Temporary(?) before MediaStreamManager is submitted. Will this enum stay even after MediaStreamManager is added? If not, could you add TODO here since it will be changed (removed?) after MediaStreamManager is submitted? http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:36: // Register, Unregister, Open, Close, EnumerateDevices no need to have comments for each function. These comments would be in base class. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:38: virtual int Register(MediaStreamType service_type, what's the meaning of returned value? If there are only 0 and non-zero, bool is recommended. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:53: MediaCaptureSessionId capture_session_id); How many modules will call Open/Close? If there are multiple, is it needed to have policy such as only whoever opens the device can close the device? http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:61: // 'stopped_task' will be called when a device has been stopped and no more would it good to rephrase this to: Stop device capture referenced by |capture_session_id|. No more frames will be delivered to the frame receiver, and |stopped_task| will be called. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager_unittest.cc:68: }; // class MockMediaStreamProviderListener one blank line here. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager_unittest.cc:180: remove one blank line.
http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... File content/browser/media_stream/media_stream_provider.h (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... content/browser/media_stream/media_stream_provider.h:13: typedef int MediaCaptureSessionId; Yes it is. We need to discuss where to put the typedef since it will be used by video capture classes in render process, browser process as well as by media stream. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... content/browser/media_stream/media_stream_provider.h:52: class MediaStreamProviderListener { On 2011/05/06 00:11:31, wjia wrote: > Could you add comments on those API's? Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... content/browser/media_stream/media_stream_provider.h:89: }; On 2011/05/06 00:11:31, wjia wrote: > one empty line here. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/me... content/browser/media_stream/media_stream_provider.h:92: #endif // CONTENT_BROWSER_MEDIA_STREAM_MEDIA_STREAM_PROVIDER_H_ On 2011/05/05 11:11:22, Magnus Flodman wrote: > Will add newline in next upload Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager.cc (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:163: device_names.get()->begin(); it != device_names.get()->end(); ++it) { What is the indent-rule here? I thought it was 4 spaces, should it match the first char inside after '('? I couldn't find this case in the style guide... http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:189: // device is supported. DeviceOpened has a different input type, but I created another version of DeviceOpened to have a similar look. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:194: if (vc_device != NULL) { Removed. vc_device == null only if a device can't be created earlier, but the it should never be added to the map. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:266: device_names.get()->clear(); On 2011/05/06 00:11:31, wjia wrote: > clear() is not needed. scoped_ptr should take care of this. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:277: // Possible errors are signaled to host Comment updated. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:293: // Possible errors are signaled by video_capture_device Comment updated. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:362: capture_session_id)); On 2011/05/06 00:11:31, wjia wrote: > there is no need for On*Proxy. You can use: > > BrowserThread::PostTask(BrowserThread::IO, > FROM_HERE, > NewRunnableMethod(this, > &VideoCaptureManager::OnOpened, > capture_session_id)); Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.cc:460: } // namespace media On 2011/05/05 11:11:22, Magnus Flodman wrote: > Will add newline in next upload. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager.h (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:12: #pragma once On 2011/05/06 00:11:31, wjia wrote: > pragma is not needed. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:21: #include "media/video/capture/video_capture_device.h" On 2011/05/05 11:11:22, Magnus Flodman wrote: > Will change include order or _types.h and device.h in next upload. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:29: // not been called. Temporary(?) before MediaStreamManager is submitted. Comment updated, removed last part. I don't know. This will still be there if it is needed by some other team than WebRtc that won't use MediaStreamManager, e.g. Pepper. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:36: // Register, Unregister, Open, Close, EnumerateDevices On 2011/05/06 00:11:31, wjia wrote: > no need to have comments for each function. These comments would be in base > class. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:38: virtual int Register(MediaStreamType service_type, On 2011/05/06 00:11:31, wjia wrote: > what's the meaning of returned value? If there are only 0 and non-zero, bool is > recommended. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:53: MediaCaptureSessionId capture_session_id); Only one module will call Open/Close, only one listener is allowed. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:61: // 'stopped_task' will be called when a device has been stopped and no more Comment updated. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager.h:142: #endif // CONTENT_BROWSER_MEDIA_STREAM_VIDEO_CAPTURE_MANAGER_H_ On 2011/05/05 11:11:22, Magnus Flodman wrote: > Will add newline in next upload. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... File content/browser/media_stream/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager_unittest.cc:10: #include "base/memory/scoped_ptr.h" On 2011/05/05 11:11:22, Magnus Flodman wrote: > Will change include order of message_loop.h and meomry/scoped_ptr.h in next > upload. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager_unittest.cc:68: }; // class MockMediaStreamProviderListener On 2011/05/06 00:11:31, wjia wrote: > one blank line here. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager_unittest.cc:180: On 2011/05/06 00:11:31, wjia wrote: > remove one blank line. Done. http://codereview.chromium.org/6946001/diff/1/content/browser/media_stream/vi... content/browser/media_stream/video_capture_manager_unittest.cc:278: } On 2011/05/05 11:11:22, Magnus Flodman wrote: > Will add newline character in next upload. Done.
Updated two error checks for video_capture_manager.cc and synced with Chrome LKGR, r84920.
Thanks for doing this guys, much appreciated! http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... File content/browser/media_stream/media_stream_provider.h (right): http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:53: // Feedback class used by MediaStreamProvider. Optional: perhaps "Callback class" might better indicate the purpose than "Feedback class" http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:57: virtual void Opened(MediaStreamType stream_type, Question: Since the MediaCaptureSessionId is being passed in, is it necessary to also pass in the stream type? Question: Also, I get the feeling that these interfaces are intended to only ever deal with one stream type. Does it complicate things if you drop it as an argument from all the methods in this class and also MediaStreamProvider? http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:80: // Registers a listener, only one listener is allowed. Please clarify: is this one listener per service type, or one listener in total? http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager.cc (right): http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:20: bool VideoCaptureManager::use_fake_device_ = false; Question: Can you make use_fake_device_ non-static (i.e. an ordinary member)? http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:26: void VideoCaptureManager::CreateTestManager() { Suggestion: Remove this method and make UseFakeDevice a public method, perhaps called UseFakeDeviceForTests. Test code could then use VideoCaptureManager::Get()->UseFakeDeviceForTests() http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:41: devices_.clear(); devices_ will be cleared by virtue of its destruction, so no need to call it explicitly here. The same goes for vc_device_thread.Stop(), but I think it is a good idea to call it explicitly here because you don't want the thread to be running after the destructor's body exits and the member fields are destroyed. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:47: if (service_type != kVideoCapture) { Nit (here, and elsewhere): Single line body should remove { } http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:51: if (listener_) { Question (here, and elsewhere): Is it a logic error to Register when there is already a listener? If so add a DCHECK: DCHECK(!listener_); Same question for the service_type checks. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:66: return; Nit (here and elsewhere): Unnecessary final return statement. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:75: PostOnError(-1, kInvalidMediaStreamType); Suggestion: Introduce a constant, e.g. kInvalidMediaCaptureSessionId instead of using -1 directly. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:182: if (devices_.find(capture_session_id) != devices_.end()) { Question: Is it appropriate to add more DCHECKs into the error checking here? http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:190: if (DeviceOpened(device) == true) { Nit: Remove == true http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:241: if (capture_params.session_id == kStartOpenSessionId) { Question: This seems like a convenience feature. If it was possible to move it outside of the implementation of VideoCaptureManager, I think it make it easier to check the correctness of this file. Where is this convenience used? http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:385: error)); Nit: indentation http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:412: if (device_name.unique_id.compare(it->second->device_name().unique_id) == I think we can use a directy comparison (==) instead of .compare? Same below. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:421: bool VideoCaptureManager::DeviceOpened( Add DCHECK to ensure/document what thread this method should be running on. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager.h (right): http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:23: class VideoCaptureManager Please provide a class level comment describing the purpose of this class. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:24: : public MediaStreamProvider { Join with the previous line. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:50: void Start(const media::VideoCaptureParams& capture_params, Question: Can the VideoCaptureSessionId specified in capture_params (and also as the first argument of Stop()) ever be a value other than kStartOpenSessionId? Suggestion: Move session_id out of VideoCaptureParams. Perhaps VideoCaptureSessionId type in video_capture_types is then not necessary? http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:61: // video capture device, due to time requirements. The function must be called Nit: device, due => device. Due time => timing requirements. The => requirements, the http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:63: static void CreateTestManager(); Nit: Move to just below static VideoCaptureManager* Get() to match order of implementation in .cc file. In general, the text order of the methods in the .cc file should match the text order of the methods in the .h. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:67: // Used for testing to mock platform dependent device code. Nit: to => with http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:111: // Only accessed on IO thread Nit: IO => Browser::IO (for consistency with above comments) http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:116: typedef std::map<int, media::VideoCaptureDevice*> VideoCaptureDevices; Question: Could we use MediaCaptureSessionId instead of int for the key type of map? I think this would be clearer. In the implementation, I see that the devices_ is keyed on both media::VideoCaptureSessionId and MediaCaptureSessionId, which would indicate that they are the same concept and perhaps should be merged. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager_unittest.cc:73: class MockFrameObserver: public media::VideoCaptureDevice::EventHandler { Do you intend any of these classes to be reused outside of this file? If not, put the rest of the file in an anonymous namespace. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager_unittest.cc:186: // Open the same device twice, should faild. Nit: faild => fail
Thanks for the comments John! Most of them are applied to the code, a few have only comments. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... File content/browser/media_stream/media_stream_provider.h (right): http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:53: // Feedback class used by MediaStreamProvider. On 2011/05/11 11:36:35, John Knottenbelt wrote: > Optional: perhaps "Callback class" might better indicate the purpose than > "Feedback class" Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:57: virtual void Opened(MediaStreamType stream_type, The plan is to have the MediaStreamManager instance registered as listener for both VideoCaptureManager and the audio manager. That's why the stream_type is an argument for the callbacks, to know which MediaStreamProvider is calling that callback. I think the stream type can be removed in MediaStreamProvider though, since there will be one MediaStreamProvider per media type. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:80: // Registers a listener, only one listener is allowed. There can be only one listener registered per MediaSteamProvider instance, but the plan is to use the single MediaStreamManager instance as listener for all MediaStreamProviders. I added a comment at the top of this file that hopefully explains this. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager.cc (right): http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:20: bool VideoCaptureManager::use_fake_device_ = false; On 2011/05/11 11:36:35, John Knottenbelt wrote: > Question: Can you make use_fake_device_ non-static (i.e. an ordinary member)? Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:26: void VideoCaptureManager::CreateTestManager() { On 2011/05/11 11:36:35, John Knottenbelt wrote: > Suggestion: Remove this method and make UseFakeDevice a public method, perhaps > called UseFakeDeviceForTests. > > Test code could then use VideoCaptureManager::Get()->UseFakeDeviceForTests() Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:41: devices_.clear(); On 2011/05/11 11:36:35, John Knottenbelt wrote: > devices_ will be cleared by virtue of its destruction, so no need to call it > explicitly here. > > The same goes for vc_device_thread.Stop(), but I think it is a good idea to call > it explicitly here because you don't want the thread to be running after the > destructor's body exits and the member fields are destroyed. Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:47: if (service_type != kVideoCapture) { Earlier code review (for VideoCaptureDeviceLinux) gave feedback to use one of the styles, but not mix styles in the same cc-file. After that we've used {} in all our code for all if-statements, so I'll keep it that way if you don't disagree and have a strong opinion about it. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:51: if (listener_) { On 2011/05/11 11:36:35, John Knottenbelt wrote: > Question (here, and elsewhere): Is it a logic error to Register when there is > already a listener? If so add a DCHECK: > > DCHECK(!listener_); > > Same question for the service_type checks. Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:66: return; On 2011/05/11 11:36:35, John Knottenbelt wrote: > Nit (here and elsewhere): Unnecessary final return statement. Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:75: PostOnError(-1, kInvalidMediaStreamType); servive_type removed, but invalid id added as return valur from Open(). http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:182: if (devices_.find(capture_session_id) != devices_.end()) { Yes, changed. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:190: if (DeviceOpened(device) == true) { On 2011/05/11 11:36:35, John Knottenbelt wrote: > Nit: Remove == true Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:241: if (capture_params.session_id == kStartOpenSessionId) { It is used to be able to open capture devices in Chrome without using MediaStreamManager. This will be used until MediaStreamManager is committed and also depending on Pepper and their use case. I see the point of the comment, but I think this is the easiest solution in Chrome and moving this outside of this class will probably make the rest of the code harder to check than this is. Moving this to another class, which would be VideoCaptureHost, will also require changes to error checks and other functions in this class. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:385: error)); On 2011/05/11 11:36:35, John Knottenbelt wrote: > Nit: indentation Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:412: if (device_name.unique_id.compare(it->second->device_name().unique_id) == On 2011/05/11 11:36:35, John Knottenbelt wrote: > I think we can use a directy comparison (==) instead of .compare? > > Same below. Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:421: bool VideoCaptureManager::DeviceOpened( On 2011/05/11 11:36:35, John Knottenbelt wrote: > Add DCHECK to ensure/document what thread this method should be running on. Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager.h (right): http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:23: class VideoCaptureManager On 2011/05/11 11:36:35, John Knottenbelt wrote: > Please provide a class level comment describing the purpose of this class. I wrote a description at the top of this file. Do you think that one should be moved to here, shall the explanation be extended and more detailed or shall there just be a small explanatino as added? http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:24: : public MediaStreamProvider { On 2011/05/11 11:36:35, John Knottenbelt wrote: > Join with the previous line. Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:50: void Start(const media::VideoCaptureParams& capture_params, Yes. kStartOpenSessionId will only be used when MediaStreamManager is not used. As a start this will be used in our tests to be able to capture video before MediaStream has been commited to Chrome and WebKit. We're looking at this at another patch and we're not sure how to solve this yet. The first step is to merge the two typedefs into one: http://codereview.chromium.org/7001017/ http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:61: // video capture device, due to time requirements. The function must be called On 2011/05/11 11:36:35, John Knottenbelt wrote: > Nit: > device, due => device. Due > time => timing > requirements. The => requirements, the Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:63: static void CreateTestManager(); On 2011/05/11 11:36:35, John Knottenbelt wrote: > Nit: Move to just below static VideoCaptureManager* Get() to match order of > implementation in .cc file. > > In general, the text order of the methods in the .cc file should match the text > order of the methods in the .h. Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:67: // Used for testing to mock platform dependent device code. On 2011/05/11 11:36:35, John Knottenbelt wrote: > Nit: to => with Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:111: // Only accessed on IO thread On 2011/05/11 11:36:35, John Knottenbelt wrote: > Nit: IO => Browser::IO > (for consistency with above comments) Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.h:116: typedef std::map<int, media::VideoCaptureDevice*> VideoCaptureDevices; I've added a TODO(mflodman) for changing key type in the map when the new patch is in and we have one type instead of two different as of today. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager_unittest.cc:73: class MockFrameObserver: public media::VideoCaptureDevice::EventHandler { On 2011/05/11 11:36:35, John Knottenbelt wrote: > Do you intend any of these classes to be reused outside of this file? If not, > put the rest of the file in an anonymous namespace. Done. http://codereview.chromium.org/6946001/diff/11001/content/browser/media_strea... content/browser/media_stream/video_capture_manager_unittest.cc:186: // Open the same device twice, should faild. On 2011/05/11 11:36:35, John Knottenbelt wrote: > Nit: faild => fail Done.
LGTM Thanks, Magnus.
This round is for unit test. In general, you can simplify those interfacing classes with gmock if you don't really care about the data content which are passed in or returned back. It's your call. Otherwise, LGTM. http://codereview.chromium.org/6946001/diff/23001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/6946001/diff/23001/content/browser/media_strea... content/browser/media_stream/video_capture_manager_unittest.cc:41: MediaCaptureSessionId capture_session_id) { the virtual method can be mocked directly: MOCK_METHOD2(Opened, void(MediaStreamType, const MediaCaptureSessionId)) without another function. http://codereview.chromium.org/6946001/diff/23001/content/browser/media_strea... content/browser/media_stream/video_capture_manager_unittest.cc:228: .Times(1); these 2 can be consolidated to .Times(2)
Unit test changed based on wjia's comments. http://codereview.chromium.org/6946001/diff/23001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/6946001/diff/23001/content/browser/media_strea... content/browser/media_stream/video_capture_manager_unittest.cc:41: MediaCaptureSessionId capture_session_id) { On 2011/05/18 22:45:25, wjia wrote: > the virtual method can be mocked directly: > MOCK_METHOD2(Opened, void(MediaStreamType, const MediaCaptureSessionId)) > without another function. Done. http://codereview.chromium.org/6946001/diff/23001/content/browser/media_strea... content/browser/media_stream/video_capture_manager_unittest.cc:228: .Times(1); On 2011/05/18 22:45:25, wjia wrote: > these 2 can be consolidated to .Times(2) Done.
Removed unnecessary test from the unit test. The test checked if we received data from the fake device, which is timing dependent and the device code itself is not in the scope of this test. The test runs fine on both the Windows desktop and Linux desktops tried, but it failed on the try server.
+brettw Brett, could you take a look?
Just some style nits, LGTM with this. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... File content/browser/media_stream/media_stream_provider.h (right): http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:43: MediaStreamType stream_type; Data should come after the functions. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:48: MediaCaptureDeviceInfo() : stream_type(kNoService), name(), device_id(), If initializers don't fit all one one line, format them like you do the second constructor (with each one on separate lines). http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager.cc (right): http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:14: // Starting id for the first capture session All of your comments should be complete sentences with periods at the end. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:15: enum { kFirstSessionId = 2}; This value is a bit mysterious. The comment should probably say why you picked this value or use 1. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:130: device_names.get()->begin(); it != device_names.get()->end(); ++it) { Wrapped lines should be indented to inside the (. Normalyy we would indent this an extra 4 spaces to differentiate it from the beginning of the next "argument" (so under the "a"). http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:136: // Let the listener know the result This comment and the next one don't add any value, I'd remove them.
Thanks Brett, John and Wei for all feedback and help!
send out pending comments to fully close this issue on review server. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... File content/browser/media_stream/media_stream_provider.h (right): http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:43: MediaStreamType stream_type; On 2011/05/24 20:49:30, brettw wrote: > Data should come after the functions. Done. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/media_stream_provider.h:48: MediaCaptureDeviceInfo() : stream_type(kNoService), name(), device_id(), On 2011/05/24 20:49:30, brettw wrote: > If initializers don't fit all one one line, format them like you do the second > constructor (with each one on separate lines). Done. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... File content/browser/media_stream/video_capture_manager.cc (right): http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:14: // Starting id for the first capture session On 2011/05/24 20:49:30, brettw wrote: > All of your comments should be complete sentences with periods at the end. Done. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:15: enum { kFirstSessionId = 2}; On 2011/05/24 20:49:30, brettw wrote: > This value is a bit mysterious. The comment should probably say why you picked > this value or use 1. change it to kFirstSessionId = VideoCaptureManager::kStartOpenSessionId + 1. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:130: device_names.get()->begin(); it != device_names.get()->end(); ++it) { On 2011/05/24 20:49:30, brettw wrote: > Wrapped lines should be indented to inside the (. Normalyy we would indent this > an extra 4 spaces to differentiate it from the beginning of the next "argument" > (so under the "a"). Done. http://codereview.chromium.org/6946001/diff/34001/content/browser/media_strea... content/browser/media_stream/video_capture_manager.cc:136: // Let the listener know the result On 2011/05/24 20:49:30, brettw wrote: > This comment and the next one don't add any value, I'd remove them. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
