Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7192007:
Adding error signalling from device to VideocaptureManager to relay up to MediaStream and WebKit. (Closed)
|
Created:
9 years, 6 months ago by mflodman1 Modified:
9 years, 6 months ago Reviewers:
commit-bot: I haz the power, Leandro Graciá Gil, Per K, John Knottenbelt, scherkus (not reviewing) CC:
chromium-reviews, hclam+watch_chromium.org, sjl, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., jam, joi+watch-content_chromium.org, acolwell GONE FROM CHROMIUM, annacc, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdding error signalling from device to VideocaptureManager to relay up to MediaStream and WebKit.
Moving struct out of media_stream_provider.h to media_stream_options.h. The struct will later be used in MediaStream IPC and MediaStreamManager.
BUG=none
TEST=try bots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89852
Patch Set 1 #
Total comments: 71
Patch Set 2 : Review changes based on feedback from jknotten, scherkus and leandrogracia. #
Total comments: 6
Patch Set 3 : '' #Patch Set 4 : Struct/enum change in media_stream_options.h. #Patch Set 5 : Adding the gyp-files. #
Total comments: 11
Messages
Total messages: 13 (0 generated)
Adding jknotten and perkj as reviewers.
A few nits and questions :) http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.cc (left): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:41: DCHECK(!listener_); Why is this check is no longer appropriate? If it is still appropriate, would it also be appropriate to add a DCHECK in VideoCaptureManager::Unregister? http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:206: GetAvailableDevices(device_names.get()); Is it possible for device_names to be empty after this call? http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.h (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.h:58: // A capture device error has occurred for |capture_sessions_id|. The device nit: capture_session_id http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.h:77: void OnOpen(int capture_session_id, const StreamDeviceInfo device); Is it possible to pass const references here for StreamDeviceInfo, and also below for PostOnDevicesEnumerated? http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager_unittest.cc:256: device_id, false); nit: indentation http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... File content/common/media/media_stream_options.cc (right): http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.cc:11: name(), is it necessary to list name and device_id here (they will be invoked with default constructors anyway) http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... File content/common/media/media_stream_options.h (right): http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:1: // Copyright (c) 20101 The Chromium Authors. All rights reserved. nit: 20101 => 2011
Here are some comments. I hope I'm not repeating John's. Also, I made a few comments to the existing code. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:30: devices_(), The default constructor will be automatically called. I don't think this is needed. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:35: VideoCaptureManager::~VideoCaptureManager() { You should delete the entries of your devices_ map, if any. Or switch to scoped_refptr (so that they can be copied) instead of raw pointers which may make the code simpler. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:128: scoped_ptr<media::VideoCaptureDevice::Names> device_names( If I'm not wrong this is a scoped_ptr to a pointer of an STL list of structures containing strings. Are you sure you need to do this? Since you're not passing the device_names list anywhere else and you're not allocating any initial list size I think maybe this would be simpler and enough: media::VideoCaptureDevice::Names device_names; GetAvailableDevices(&device_names); // The only reason you need a pointer is because of style conventions we pass pointers instead of non-const references. This is just to fill the list as you know. It should also simplify the code of the iterators and avoid cleaning up it later. One more thing: is there any special reason you're using STL lists instead of STL vectors? Unless you're expecting to insert elements frequently or in positions different to the end I think a vector would be a better choice. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:143: devices.clear(); I think this happens automatically with the destructor. Am I wrong? http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:144: device_names.get()->clear(); Independently of the comment about the type, if you use scoped_ptr objects you should be able to use the overloaded -> operator without calling .get(). http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:219: video_capture_receiver->OnError(); Since you're using pointers for this a DCHECK would not harm. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:248: delete stopped_task; I have no idea of how this works but it looks a bit dangerous. Are you sure this is the right way to go? http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:278: if (listener_ == NULL) { I think style says we should use if (!listener_) in these situations, but I'm not completely sure. Can you check it? http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:314: StreamDeviceInfoArray devices) { Looks like this could be a const reference to me. You are passing an STL vector from the same thread when calling this. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:372: if (device_info.device_id == it->second->device_name().unique_id) { Looks like this if doesn't need braces according to the style guidelines. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.h (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.h:114: VideoCaptureDevices devices_; It's probably worth to make a comment saying that this class has the ownership and takes care of the lifetime of every single VideoCaptureDevice pointer in this map.
driveby! http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/media_stream_provider.h (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/media_stream_provider.h:86: nit: get rid of blank line http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_controller.cc (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:212: nit: get rid of blank line http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:380: nit: get rid of blank line http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.h (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.h:60: void Error(const media::VideoCaptureSessionId capture_session_id); const-ref ? http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... File content/common/media/media_stream_options.cc (right): http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.cc:27: nit: get rid of blank line http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... File content/common/media/media_stream_options.h (right): http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:5: #ifndef CONTENT_COMMON_MEDIA_MEDIA_STREAM_OPTIONS_H_ sanity check... does this have to be inside of content/common/ ? I don't think I see any code inside of content/renderer yet but perhaps there will be soon :) in any case, could you add an OWNERS file w/ myself listed for the time being? http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:11: namespace media_stream { nit: content/ doesn't typically use namespaces, but up to you folks you don't have to make this change now http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:16: struct GenerateStreamOptions { nit: the name of this struct is rather confusing as it reads as a verb and not a noun is that intentional? http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:18: kVideoNoCamera = 0, nit: "Video" prefix is redundant considering this is defined inside of a struct http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:20: kVideoFacingEnvironment = 1 << 1, considering the amount of flags is relatively small, what about enforcing type safety by having all 4 options: kNone kFacingUser kFacingEnvironment kFacingBoth ...or something like that then you do w/o the unsigned typedef and things are self describing, etc.. also in the kFacingBoth case... are you requesting that you want both cameras or that you want either camera (i.e,. whatever we can satisfy). http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:25: GenerateStreamOptions() : audio(false), video_options(kVideoNoCamera) {} does having a default ctor make sense or will all callees be calling the other ctor? http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:45: StreamDeviceInfo(); nit: in general if you all callees properly initialize the struct then you can do without this ctor otherwise you get stuck with an ill-defined ctor (i.e., what does a default StreamDeviceInfo represent?) http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:47: const std::string name_param, make const-ref http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:48: const std::string device_param, ditto http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:51: enum { kNoId = -1}; nit: space before } http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:59: std::string device_id; these two parameters seem similar to what's defined under media/audio/audio_device_name.h is that intentional? http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:71: nit: get rid of blank line
Thanks for all the quick comments! And drivebys are very welcome. I'll work on this Monday morning and I'll upload a new patch then.
Thanks for your feedback! Most of that feedback is addressed. I tried to change media_stream_options.h based on your feedback Andrew, but ran into issues in the IPC code for the next patch. See comments in that file. I'll look at that tomorrow. content/common/media/OWNERS added. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/media_stream_provider.h (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/media_stream_provider.h:86: On 2011/06/17 03:03:52, scherkus wrote: > nit: get rid of blank line Done. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_controller.cc (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:212: On 2011/06/17 03:03:52, scherkus wrote: > nit: get rid of blank line Done. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.cc (left): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:41: DCHECK(!listener_); On 2011/06/16 15:20:43, John Knottenbelt wrote: > Why is this check is no longer appropriate? > If it is still appropriate, would it also be appropriate to add a DCHECK in > VideoCaptureManager::Unregister? Added DCHECK in both places to track possible user errors. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:30: devices_(), Done. This was mainly to point out it's initialized and not forgotten. I'm used to this, but seems to not be the Chrome style :) On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > The default constructor will be automatically called. I don't think this is > needed. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:35: VideoCaptureManager::~VideoCaptureManager() { This should never happen, classes "above" should take care of this. I added a DCHECK to show that is the case. OK? On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > You should delete the entries of your devices_ map, if any. Or switch to > scoped_refptr (so that they can be copied) instead of raw pointers which may > make the code simpler. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:128: scoped_ptr<media::VideoCaptureDevice::Names> device_names( Changed to not using scoped_ptr. This was left after some other rewriting, thanks! On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > If I'm not wrong this is a scoped_ptr to a pointer of an STL list of structures > containing strings. Are you sure you need to do this? Since you're not passing > the device_names list anywhere else and you're not allocating any initial list > size I think maybe this would be simpler and enough: > > media::VideoCaptureDevice::Names device_names; > GetAvailableDevices(&device_names); // The only reason you need a pointer is > because of style conventions we pass pointers instead of non-const references. > This is just to fill the list as you know. > > It should also simplify the code of the iterators and avoid cleaning up it > later. > > One more thing: is there any special reason you're using STL lists instead of > STL vectors? Unless you're expecting to insert elements frequently or in > positions different to the end I think a vector would be a better choice. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:143: devices.clear(); On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > I think this happens automatically with the destructor. Am I wrong? Done. You're of course right about elements being deleted by the destructor. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:144: device_names.get()->clear(); On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > Independently of the comment about the type, if you use scoped_ptr objects you > should be able to use the overloaded -> operator without calling .get(). Removed. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:206: GetAvailableDevices(device_names.get()); On 2011/06/16 15:20:43, John Knottenbelt wrote: > Is it possible for device_names to be empty after this call? Good point. Rewritten based on feedback above and calling error function if the device list is empty. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:219: video_capture_receiver->OnError(); On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > Since you're using pointers for this a DCHECK would not harm. Done, but at top of functions since the pointer is used at other places as well. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:248: delete stopped_task; On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > I have no idea of how this works but it looks a bit dangerous. Are you sure this > is the right way to go? This was introduced when implementing OnChannelClosing in VideoCaptureHost to signal back asap that VideoCaptureDevice has been stopped and no more captured buffers will be sent to VideoCaptureController/VideoCaptureHost. The alternative is to signal this in VideoCaptureDevice::EventHandler, but this way we can later add more than one listener for every VideoCaptureDevice. The manager will then be respomsible for stopping the device when it's really needed. Makes sense? The reason for putting it this wayinstead of VideoCaptureDevice signalling an event is to have the opportunity for VideoCaptureManager to attach multiple users of the same VideoCaptureDevice. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:278: if (listener_ == NULL) { On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > I think style says we should use if (!listener_) in these situations, but I'm > not completely sure. Can you check it? Done. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:314: StreamDeviceInfoArray devices) { On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > Looks like this could be a const reference to me. You are passing an STL vector > from the same thread when calling this. Done. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:372: if (device_info.device_id == it->second->device_name().unique_id) { On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > Looks like this if doesn't need braces according to the style guidelines. We've received comments regarding this that either way is fine, as long as it's consistent. We chose this way in, to encapsulate it in case of comments like in the function above. I'll keep it this way if there are no strong opinions about it, but I can change if that is preferred :) http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:380: On 2011/06/17 03:03:52, scherkus wrote: > nit: get rid of blank line Done. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.h (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.h:58: // A capture device error has occurred for |capture_sessions_id|. The device On 2011/06/16 15:20:43, John Knottenbelt wrote: > nit: capture_session_id Done. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.h:60: void Error(const media::VideoCaptureSessionId capture_session_id); On 2011/06/17 03:03:52, scherkus wrote: > const-ref ? Done. I'd prefer to remove this typedef and just use an int, but I'll discuss with Wei and that will be another patch if agreed. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.h:77: void OnOpen(int capture_session_id, const StreamDeviceInfo device); On 2011/06/16 15:20:43, John Knottenbelt wrote: > Is it possible to pass const references here for StreamDeviceInfo, and also > below for PostOnDevicesEnumerated? Done. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.h:114: VideoCaptureDevices devices_; On 2011/06/16 17:39:53, Leandro Graciá Gil wrote: > It's probably worth to make a comment saying that this class has the ownership > and takes care of the lifetime of every single VideoCaptureDevice pointer in > this map. Done. http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): http://codereview.chromium.org/7192007/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager_unittest.cc:256: device_id, false); On 2011/06/16 15:20:43, John Knottenbelt wrote: > nit: indentation Done. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... File content/common/media/media_stream_options.cc (right): http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.cc:11: name(), On 2011/06/16 15:20:43, John Knottenbelt wrote: > is it necessary to list name and device_id here (they will be invoked with > default constructors anyway) Done. I'm used to do it that way to indicate it's not forgotten and I didn't see any style guide about it. It doesn't seem to be the Chrome style though, so changed. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.cc:27: On 2011/06/17 03:03:52, scherkus wrote: > nit: get rid of blank line Done. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... File content/common/media/media_stream_options.h (right): http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:1: // Copyright (c) 20101 The Chromium Authors. All rights reserved. On 2011/06/16 15:20:43, John Knottenbelt wrote: > nit: 20101 => 2011 Done. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:5: #ifndef CONTENT_COMMON_MEDIA_MEDIA_STREAM_OPTIONS_H_ On 2011/06/17 03:03:52, scherkus wrote: > sanity check... does this have to be inside of content/common/ ? > > I don't think I see any code inside of content/renderer yet but perhaps there > will be soon :) > > in any case, could you add an OWNERS file w/ myself listed for the time being? The structs in this file will be used to requesting capture device usage and to signal the result of the requests. OWNERS added. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:11: namespace media_stream { On 2011/06/17 03:03:52, scherkus wrote: > nit: content/ doesn't typically use namespaces, but up to you folks > > you don't have to make this change now Good to know, noted and we'll look at this later. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:16: struct GenerateStreamOptions { On 2011/06/17 03:03:52, scherkus wrote: > nit: the name of this struct is rather confusing as it reads as a verb and not a > noun > > is that intentional? Changed to only 'StreamOptions'. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:18: kVideoNoCamera = 0, On 2011/06/17 03:03:52, scherkus wrote: > nit: "Video" prefix is redundant considering this is defined inside of a struct Done. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:20: kVideoFacingEnvironment = 1 << 1, On 2011/06/17 03:03:52, scherkus wrote: > considering the amount of flags is relatively small, what about enforcing type > safety by having all 4 options: > kNone > kFacingUser > kFacingEnvironment > kFacingBoth > > ...or something like that > > then you do w/o the unsigned typedef and things are self describing, etc.. > > also in the kFacingBoth case... are you requesting that you want both cameras or > that you want either camera (i.e,. whatever we can satisfy). I tried changing but ran into IPC issues when building with the code that will be uploaded after this is done. We, i.e. Per and I, looked at the IPC messages and defined what we thought is correct, but obviously something was missing. I'll have another look at this tomorrow. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:25: GenerateStreamOptions() : audio(false), video_options(kVideoNoCamera) {} On 2011/06/17 03:03:52, scherkus wrote: > does having a default ctor make sense or will all callees be calling the other > ctor? We don't explicitly call this ctor and I tried to remove it, resulting in errors for no matching function to the remove ctor (this is from the IPC part of media stream, which will be uploaded after this patch): In file included from ./ipc/ipc_message_utils.h:19, from ./ipc/ipc_message_macros.h:179, from ./content/common/child_process_messages.h:9, from ./content/common/content_message_generator.h:7, from content/common/content_message_generator.cc:7: ./base/tuple.h: In constructor ‘Tuple4<A, B, C, D>::Tuple4() [with A = int, B = int, C = media_stream::StreamOptions, D = std::basic_string<char, std::char_traits<char>, std::allocator<char> >]’: ./content/common/media_stream_messages.h:60: instantiated from here ./base/tuple.h:136: error: no matching function for call to ‘media_stream::StreamOptions::StreamOptions()’ Any ideas? I'm far from an IPC expert. Thanks! http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:45: StreamDeviceInfo(); On 2011/06/17 03:03:52, scherkus wrote: > nit: in general if you all callees properly initialize the struct then you can > do without this ctor > > otherwise you get stuck with an ill-defined ctor (i.e., what does a default > StreamDeviceInfo represent?) Same as for removing StreamOptions ctor above, I'll have another look at this tomorrow. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:47: const std::string name_param, On 2011/06/17 03:03:52, scherkus wrote: > make const-ref Done. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:48: const std::string device_param, On 2011/06/17 03:03:52, scherkus wrote: > ditto Done. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:51: enum { kNoId = -1}; On 2011/06/17 03:03:52, scherkus wrote: > nit: space before } Done. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:59: std::string device_id; On 2011/06/17 03:03:52, scherkus wrote: > these two parameters seem similar to what's defined under > media/audio/audio_device_name.h > > is that intentional? Yes. This struct will be used to signal name/id for both audio capture devices and video capture devices to the WebKit glue to keep track of open devices. http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:71: On 2011/06/17 03:03:52, scherkus wrote: > nit: get rid of blank line Done.
nits but LGTM, you'll have to rebase to tip-of-tree as I think there may be some conflicts http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... File content/common/media/media_stream_options.h (right): http://codereview.chromium.org/7192007/diff/1/content/common/media/media_stre... content/common/media/media_stream_options.h:25: GenerateStreamOptions() : audio(false), video_options(kVideoNoCamera) {} On 2011/06/20 19:48:03, Magnus Flodman wrote: > On 2011/06/17 03:03:52, scherkus wrote: > > does having a default ctor make sense or will all callees be calling the other > > ctor? > We don't explicitly call this ctor and I tried to remove it, resulting in > errors for no matching function to the remove ctor (this is from the IPC part of > media stream, which will be uploaded after this patch): > In file included from ./ipc/ipc_message_utils.h:19, > from ./ipc/ipc_message_macros.h:179, > from ./content/common/child_process_messages.h:9, > from ./content/common/content_message_generator.h:7, > from content/common/content_message_generator.cc:7: > ./base/tuple.h: In constructor ‘Tuple4<A, B, C, D>::Tuple4() [with A = int, B = > int, C = media_stream::StreamOptions, D = std::basic_string<char, > std::char_traits<char>, std::allocator<char> >]’: > ./content/common/media_stream_messages.h:60: instantiated from here > ./base/tuple.h:136: error: no matching function for call to > ‘media_stream::StreamOptions::StreamOptions()’ > > Any ideas? I'm far from an IPC expert. > > Thanks! Ahh I wasn't aware this ended up getting used in IPC messages... in which I don't think we can avoid it. The alternative would be to use a define a different struct in the IPC layer, but I don't think it's strictly necessary. http://codereview.chromium.org/7192007/diff/8002/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/8002/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_manager.cc:35: DCHECK_EQ(devices_.size(), size_t(0)); nit: devices_.empty() http://codereview.chromium.org/7192007/diff/8002/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_manager.cc:204: if (device_names.size() == 0) { .empty() http://codereview.chromium.org/7192007/diff/8002/content/common/media/OWNERS File content/common/media/OWNERS (right): http://codereview.chromium.org/7192007/diff/8002/content/common/media/OWNERS#... content/common/media/OWNERS:2: scherkus@chromium.org I think you'll have to rebase to tip of tree as I ended up checking in content/common/media/OWNERS last week
Changes based on Andrew's feedbak and merged with LKGR, r89645. http://codereview.chromium.org/7192007/diff/8002/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/8002/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_manager.cc:35: DCHECK_EQ(devices_.size(), size_t(0)); On 2011/06/21 00:27:46, scherkus wrote: > nit: devices_.empty() Done. http://codereview.chromium.org/7192007/diff/8002/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_manager.cc:204: if (device_names.size() == 0) { On 2011/06/21 00:27:46, scherkus wrote: > .empty() Done. http://codereview.chromium.org/7192007/diff/8002/content/common/media/OWNERS File content/common/media/OWNERS (right): http://codereview.chromium.org/7192007/diff/8002/content/common/media/OWNERS#... content/common/media/OWNERS:2: scherkus@chromium.org On 2011/06/21 00:27:46, scherkus wrote: > I think you'll have to rebase to tip of tree as I ended up checking in > content/common/media/OWNERS last week Done.
Changed enum/typedef in media_stream_options.h.
Change committed as 89852
Looks like I got late by a few minutes. I'm sorry. I have only a few minor nits. Please do continue with your patch and fix them at some point later as they're not important at all. http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:260: if (listener_ == NULL) { I've tried to find exactly where the style guidelines said that if (!listener_) is preferred over this, but I couldn't. I'm sorry if I was wrong on this, I'm pretty sure some reviewer told me about this when I started working in Chrome. Andrew, do you know something about this little detail? It's not really important. http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:261: // Listener has been removed Style says comments should end with a period. Please check many others in the patch. http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:380: } // namespace media The namespace is media_stream. http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.h (right): http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.h:55: void Stop(const media::VideoCaptureSessionId capture_session_id, Comparing to Error, I think you're missing a & here. http://codereview.chromium.org/7192007/diff/15011/content/common/media/media_... File content/common/media/media_stream_options.h (right): http://codereview.chromium.org/7192007/diff/15011/content/common/media/media_... content/common/media/media_stream_options.h:13: // StreamOptions is a Chromium representation of WebKits WebKit's
thanks for the catches leandro! magnus: send out a follow up patch and we'll get it reviewed+landed http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:260: if (listener_ == NULL) { On 2011/06/21 18:00:26, Leandro Graciá Gil wrote: > I've tried to find exactly where the style guidelines said that if (!listener_) > is preferred over this, but I couldn't. I'm sorry if I was wrong on this, I'm > pretty sure some reviewer told me about this when I started working in Chrome. > > Andrew, do you know something about this little detail? It's not really > important. there's no hard-and-fast rule about it... so it boils down to consistency if a lot of the surrounding code prefers w/o == NULL -- then stick with that I lean slightly towards (!listener_) since it's more compact
New patch created containing changes based on review by Leandro: http://codereview.chromium.org/7217018/ http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:260: if (listener_ == NULL) { On 2011/06/21 18:03:42, scherkus wrote: > On 2011/06/21 18:00:26, Leandro Graciá Gil wrote: > > I've tried to find exactly where the style guidelines said that if > (!listener_) > > is preferred over this, but I couldn't. I'm sorry if I was wrong on this, I'm > > pretty sure some reviewer told me about this when I started working in Chrome. > > > > Andrew, do you know something about this little detail? It's not really > > important. > > there's no hard-and-fast rule about it... so it boils down to consistency > > if a lot of the surrounding code prefers w/o == NULL -- then stick with that > > I lean slightly towards (!listener_) since it's more compact I agree. Done. http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:261: // Listener has been removed On 2011/06/21 18:00:26, Leandro Graciá Gil wrote: > Style says comments should end with a period. Please check many others in the > patch. Done. http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:380: } // namespace media On 2011/06/21 18:00:26, Leandro Graciá Gil wrote: > The namespace is media_stream. Done. http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.h (right): http://codereview.chromium.org/7192007/diff/15011/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.h:55: void Stop(const media::VideoCaptureSessionId capture_session_id, On 2011/06/21 18:00:26, Leandro Graciá Gil wrote: > Comparing to Error, I think you're missing a & here. Done. http://codereview.chromium.org/7192007/diff/15011/content/common/media/media_... File content/common/media/media_stream_options.h (right): http://codereview.chromium.org/7192007/diff/15011/content/common/media/media_... content/common/media/media_stream_options.h:13: // StreamOptions is a Chromium representation of WebKits On 2011/06/21 18:00:26, Leandro Graciá Gil wrote: > WebKit's Done. |
