Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24079003:
Add VideoCaptureDevice::GetDeviceSupportedFormats to interface + implementation for Linux and Fake (Closed)
|
Created:
7 years, 3 months ago by mcasas Modified:
7 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd VideoCaptureDevice::GetDeviceSupportedFormats to interface + implementation for Linux and Fake
BUG=289494
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228904
Patch Set 1 #
Total comments: 1
Patch Set 2 : Removed parameter device_name from GetDeviceSupportedFormats. Added explanation to VCD interface. #
Total comments: 47
Patch Set 3 : Addressed ncarter@, scherkus@ and perkj@ comments #Patch Set 4 : Removed inexistent video_capture_device_dummy; also removed from media.gyp targets. #
Total comments: 12
Patch Set 5 : Addressed perkj@'s comments. #Patch Set 6 : Rebase. #
Total comments: 13
Patch Set 7 : Manual rebase + changed GetDeviceSupportedFormats to static, to be called right after GetDeviceName… #
Total comments: 10
Patch Set 8 : Addressed perkj@ comments #
Total comments: 6
Patch Set 9 : More perkj@'s comments addressed. #
Total comments: 1
Patch Set 10 : Disconnected v4l2->pixel format translation for those formats _not_ supported (== revert). #
Total comments: 2
Patch Set 11 : perkj@'s nits #Messages
Total messages: 32 (0 generated)
Hi perkj@ - following our split of crrev.com/16841011 - could you PTAL? thanks!
hi scherkus@, could you PTAL? In particular to
video_capture_device_linux.{h,cc}.
https://codereview.chromium.org/24079003/diff/1/media/video/capture/video_cap... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/1/media/video/capture/video_cap... media/video/capture/video_capture_device.h:226: // driver, for a particular video capture device identified by device_name. does this method need to be part of this interface? looking at the implementation of VideoCaptureDeviceLinux::GetDeviceSupportedFormats() it doesn't use any members of the class -- it's pretty much a static method this is also requires clients to create a VideoCaptureDevice for some device before being able to query this method
On 2013/09/11 17:30:46, scherkus wrote: > https://codereview.chromium.org/24079003/diff/1/media/video/capture/video_cap... > File media/video/capture/video_capture_device.h (right): > > https://codereview.chromium.org/24079003/diff/1/media/video/capture/video_cap... > media/video/capture/video_capture_device.h:226: // driver, for a particular > video capture device identified by device_name. > does this method need to be part of this interface? > > looking at the implementation of > VideoCaptureDeviceLinux::GetDeviceSupportedFormats() it doesn't use any members > of the class -- it's pretty much a static method > > this is also requires clients to create a VideoCaptureDevice for some device > before being able to query this method Well, Linux implementation does not use any VideoCaptureDevice members because it opens the device to query the formats, in turn because the device_id in Linux is the filesystem name (/dev/video1) but in other platforms (win, android), the device has to be opened and the reference kept before the capabilities can be enumerated; but this is indeed not obvious from the CL. On second terms, if the device is already opened _and_ we don't allow reopening in a larger resolution, f.i., the GetDeviceCapabilities would truncate the list, etc., and for this the current device state has to be accessed, e.g. a member method.
On 2013/09/11 19:56:44, miguelao wrote: > On 2013/09/11 17:30:46, scherkus wrote: > > > https://codereview.chromium.org/24079003/diff/1/media/video/capture/video_cap... > > File media/video/capture/video_capture_device.h (right): > > > > > https://codereview.chromium.org/24079003/diff/1/media/video/capture/video_cap... > > media/video/capture/video_capture_device.h:226: // driver, for a particular > > video capture device identified by device_name. > > does this method need to be part of this interface? > > > > looking at the implementation of > > VideoCaptureDeviceLinux::GetDeviceSupportedFormats() it doesn't use any > members > > of the class -- it's pretty much a static method > > > > this is also requires clients to create a VideoCaptureDevice for some device > > before being able to query this method > > Well, Linux implementation does not use any VideoCaptureDevice members because > it opens the device to query the formats, in turn because the device_id in Linux > is the filesystem name (/dev/video1) but in other platforms (win, android), the > device has to be opened and the reference kept before the capabilities can be > enumerated; but this is indeed not obvious from the CL. > > On second terms, if the device is already opened _and_ we don't allow reopening > in a larger resolution, f.i., the GetDeviceCapabilities would truncate the list, > etc., and for this the current device state has to be accessed, e.g. a member > method. Do you always call GetDeviceSupportedFormats() with the same device_name as the one you pass into Create()? If so, could we drop the device_name parameter? That would make things a bit clearer.
Removed parameter "device_name" from GetDeviceSupportedFormats, following scherkus@ review, and added info on VCD interface.
https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_device.h:230: virtual void GetDeviceSupportedFormats( how do I know if this method succeeded or not? what happens to capture_formats when it does fail? https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_device.h:231: VideoCaptureFormats* capture_formats) = 0; this CL could be dramatically simplified if you provided a default implementation in the .cc
https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:257: remove blank line https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:260: int fd = open(device_name_.id().c_str(), O_RDONLY); use file_util::ScopedFD https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:260: int fd = open(device_name_.id().c_str(), O_RDONLY); this should be wrapped with HANDLE_EINTR() https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:262: return; DPLOG(ERROR) << "Couldn't open " << device_name_.id(); DPLOG will provide the posix last error message https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:264: media::VideoCaptureCapability capture_format; remove media:: https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:271: // Retrieve the caps one by one, first get colorspace, then sizes, then instead of having this large block of code indented, check for the error condition and return early https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:276: capture_format.color = media::V4l2ColorToVideoCaptureColorFormat( remove media:: https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:276: capture_format.color = media::V4l2ColorToVideoCaptureColorFormat( do we have to cast here? it seems we don't have to on line 418 and as far as I can tell the types are the same https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:288: // TODO(mcasas) see crbug.com/249953 nit: we format TODOs with a : after the ) also can you elaborate / provide a summary of what to do in addition to linking to the bug? https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:288: // TODO(mcasas) see crbug.com/249953 nit: use http:// in front of crbug.com (many text editors 'n web browsers will link-ify the result when viewing the source) https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:291: // Same here as the case before: TODO(mcasas) see crbug.com/249953 ditto https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:292: NOTIMPLEMENTED(); if these are unimplemented, do you mean to continue executing and insert a result? is there a use case for inserting a bunch of VideoCaptureCapabilities with 0 width/height? https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:301: (frame_interval.discrete.numerator != 0) I'd just use an if () statement here -- the ?: operator isn't making things easy to read in this case https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:307: // Same here as the case above: TODO(mcasas) see crbug.com/249953 nit: try to start TODO comments at the beginning of a comment // TODO(mcasa): Same here as above, see http://crbug.com/249953 https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:312: NOTIMPLEMENTED(); ditto... is there a use case for inserting VideoCaptureCapabilities with an undefined frame rate? https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:315: capture_formats->insert(capture_formats->end(), capture_format); push_back() ? https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_types.h:80: typedef std::vector<media::VideoCaptureCapability> VideoCaptureFormats; remove media:: https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_types.h:80: typedef std::vector<media::VideoCaptureCapability> VideoCaptureFormats; it seems unfortunate that we have to use VideoCaptureCapability considering we only set color, width, height, and frame_rate what about expected_capture_delay, interlaced? session_id also seems very out of place
https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:256: VideoCaptureFormats* capture_formats) { To be threadsafe - all v4l2 calls in this file is done on a separate thread |v4l2_thread_| - this is since v4l2 require us to pull frames from a camera. You will need to decide on if you want to call this before or after Allocate- Allocate open the driver so you should not do it again. Would it make more sence to move this code to OnAllocate and let the supported formats be returned in the observer interface? Also consider moving the requirement for a capture format to the Start method? https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_device.h:227: // after the device being opened with Allocate(), and it may take into account It seems a bit weird that we require Allocate to be called before this function. Before calling this function- there is no way of knowing what |capture_format| to Allocate. Please also make sure that you use the same "capability" structure for both Allocate and this new method. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_types.h:82: } // namespace media yes- session_id seems wrong to have at all. Can you help checkout and write a comment what VideoCaptureResolutionType frame_size_type is? Is interlaced used by anyone? Is there even a reason to separate VideoCaptureFormat and VideoCaptureCapability? Wouldn't it make more sence to combine them?
https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_types.h:82: } // namespace media On 2013/09/13 10:33:05, perkj wrote: > yes- session_id seems wrong to have at all. > > Can you help checkout and write a comment what VideoCaptureResolutionType > frame_size_type is? > Is interlaced used by anyone? Is there even a reason to separate > VideoCaptureFormat and VideoCaptureCapability? Wouldn't it make more sence to > combine them? Here's the information needs I see: VCDevice <-> VCDevice::EventHandler (needs the info: width, height, colorspace, stride etc. Does NOT have a session_id because a device can serve multiple sessions.) VCDevice::EventHandler <-> VCController (needs width and height, but colorspace is implicitly I420. Does not have a session_id, because that's per-client state.) VCController/VCManager <-> VCHost <-> VideoCapture <-> VideoCaptureImpl <-> delegates (needs width and height. The session_id can be included but is redundant unless it is in the context of the first (Start) request, in which case it's a required parameter. Pixel format is always implicitly I420, and should never be included.) As far as I can tell, |interlaced| serves no purpose anywhere. It's not clear to me (because I haven't looked) how important it is that we send frame_rate in responses everywhere, but I don't see it as hurting anything and can be bundled with width and height. I don't think that VideoCaptureResolutionType as currently formulated is altogether coherent. It's not clear to me whether it's a property of the device, a mode of the device, or a constraint of the receiver. It seems like it's actually a mix. If I've read the code correctly, the PPAPI clients can't deal with variable resolutions, but the webrtc clients can? So in that sense it's a constraint of the client. But also, only one of the devices is geared up to support the variable resolution mode, so it's also a capability of the device.
https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_device.h:227: // after the device being opened with Allocate(), and it may take into account On 2013/09/13 10:33:05, perkj wrote: > It seems a bit weird that we require Allocate to be called before this function. > Before calling this function- there is no way of knowing what |capture_format| > to Allocate. > Please also make sure that you use the same "capability" structure for both > Allocate and this new method. > I have a CL out that merges Start+Allocate, and the proposed semantic here is at odds with that. If you have a look at https://codereview.chromium.org/24133002/ I think you'll quickly see how they're in conflict. Are there notes or an email thread somewhere that describes your plan for how GetDeviceSupportedFormats will operate end-to-end? What's not clear to me is how this new function is intended to be called in practice from VideoCaptureManager. Today a device is always constructed, Allocate()'d and Start()'ed in response to a HostMsg_Start IPC event, and that's why I'm moving to merge those phases. (see the recently-revamped code at https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... ) At what point exactly in the lifetime of the stream do we expect to enumerate the supported formats? Does the result need to make it all the way back to the renderer process, or is it handled in the controller? My gut feeling right now is that this functionality is more about enumeration of devices, rather than operation of a device. If so then it probably doesn't belong tied to an instance of VideoCaptureDevice* at all. It should probably be more like how device enumeration works, using static functions. In my thinking, once a VideoCaptureDevice exists you ought to know what you want to do with it.
On 2013/09/13 19:21:08, ncarter wrote: > https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... > File media/video/capture/video_capture_device.h (right): > > https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... > media/video/capture/video_capture_device.h:227: // after the device being opened > with Allocate(), and it may take into account > On 2013/09/13 10:33:05, perkj wrote: > > It seems a bit weird that we require Allocate to be called before this > function. > > Before calling this function- there is no way of knowing what |capture_format| > > to Allocate. > > Please also make sure that you use the same "capability" structure for both > > Allocate and this new method. > > > > I have a CL out that merges Start+Allocate, and the proposed semantic here is at > odds with that. If you have a look at https://codereview.chromium.org/24133002/ > I think you'll quickly see how they're in conflict. Are there notes or an email > thread somewhere that describes your plan for how GetDeviceSupportedFormats will > operate end-to-end? > > What's not clear to me is how this new function is intended to be called in > practice from VideoCaptureManager. Today a device is always constructed, > Allocate()'d and Start()'ed in response to a HostMsg_Start IPC event, and that's > why I'm moving to merge those phases. (see the recently-revamped code at > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > ) At what point exactly in the lifetime of the stream do we expect to enumerate > the supported formats? Does the result need to make it all the way back to the > renderer process, or is it handled in the controller? > > My gut feeling right now is that this functionality is more about enumeration of > devices, rather than operation of a device. If so then it probably doesn't > belong tied to an instance of VideoCaptureDevice* at all. It should probably be > more like how device enumeration works, using static functions. In my thinking, > once a VideoCaptureDevice exists you ought to know what you want to do with it. Thanks for the info ncarter. Great to see your cl is trying to cleanup a bit. There has been a lot of people doing small changes here and there to get capture to work for different use cases. The WebRtc has for once changed the spec way to many times. The latest spec regarding media capture and getusermedia is from August 30 and can be found here. http://dev.w3.org/2011/webrtc/editor/getusermedia.html. What I think Miguels cl is a start for is to be able to let JS decide what resolution the camera should be using when it starts based on what it actually supports. In JS this is constraints in the call to getUserMedia(). It should also be possible to query a camera what it supports (after it has been opened and started) in order to reconfigure it. In JS this is a call to MediaStreamTrack::capabilities (). To reconfigure the resolutions you should call MediaStreamTrack::applyConstraints(MediaTrackConstraints constraints) In order to support these use cases I think it should be possible to query or be notified of the camera capabilities after the camera driver has been opened but before the camera is started from the render process. Since pepper required that a camera can be shared between tabs and also WebRtc really like this for testing etc - we might want to change the supported capabilities if the camera is used in multiple tabs. That said- I like your refactoring of VideoCaptureDevice but we might want to allow queering or getting the capabilities before calling AllocateAndStart.
https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_device.h:229: // supported formats limited etc. The current contract of this function seems to expect synchronous operation. Are you sure this will work well on all device implementations? The threading story of various device impls is all over the place. MFWin and the desktop capture devices are two cases where it's not clear if we can get what we need. Should the response be sent via some method of the VCD::EH interface instead? That would make it much easier to communicate this info back down the VideoCaptureController -> VideoCaptureHost -> VIdeoCaptureImpl chain. Or does this information need to flow back via MediaStreamManager?
On 2013/09/16 08:48:09, perkj wrote: > On 2013/09/13 19:21:08, ncarter wrote: > > > https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... > > File media/video/capture/video_capture_device.h (right): > > > > > https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... > > media/video/capture/video_capture_device.h:227: // after the device being > opened > > with Allocate(), and it may take into account > > On 2013/09/13 10:33:05, perkj wrote: > > > It seems a bit weird that we require Allocate to be called before this > > function. > > > Before calling this function- there is no way of knowing what > |capture_format| > > > to Allocate. > > > Please also make sure that you use the same "capability" structure for both > > > Allocate and this new method. > > > > > > > I have a CL out that merges Start+Allocate, and the proposed semantic here is > at > > odds with that. If you have a look at > https://codereview.chromium.org/24133002/ > > I think you'll quickly see how they're in conflict. Are there notes or an > email > > thread somewhere that describes your plan for how GetDeviceSupportedFormats > will > > operate end-to-end? > > > > What's not clear to me is how this new function is intended to be called in > > practice from VideoCaptureManager. Today a device is always constructed, > > Allocate()'d and Start()'ed in response to a HostMsg_Start IPC event, and > that's > > why I'm moving to merge those phases. (see the recently-revamped code at > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > ) At what point exactly in the lifetime of the stream do we expect to > enumerate > > the supported formats? Does the result need to make it all the way back to the > > renderer process, or is it handled in the controller? > > > > My gut feeling right now is that this functionality is more about enumeration > of > > devices, rather than operation of a device. If so then it probably doesn't > > belong tied to an instance of VideoCaptureDevice* at all. It should probably > be > > more like how device enumeration works, using static functions. In my > thinking, > > once a VideoCaptureDevice exists you ought to know what you want to do with > it. > > Thanks for the info ncarter. > Great to see your cl is trying to cleanup a bit. There has been a lot of people > doing small changes here and there to get capture to work for different use > cases. > The WebRtc has for once changed the spec way to many times. The latest spec > regarding media capture and getusermedia is from August 30 and can be found > here. http://dev.w3.org/2011/webrtc/editor/getusermedia.html. > > > What I think Miguels cl is a start for is to be able to let JS decide what > resolution the camera should be using when it starts based on what it actually > supports. In JS this is constraints in the call to getUserMedia(). > It should also be possible to query a camera what it supports (after it has been > opened and started) in order to reconfigure it. In JS this is a call to > MediaStreamTrack::capabilities (). > To reconfigure the resolutions you should call > MediaStreamTrack::applyConstraints(MediaTrackConstraints constraints) > > In order to support these use cases I think it should be possible to query or be > notified of the camera capabilities after the camera driver has been opened but > before the camera is started from the render process. > Since pepper required that a camera can be shared between tabs and also WebRtc > really like this for testing etc - we might want to change the supported > capabilities if the camera is used in multiple tabs. > > That said- I like your refactoring of VideoCaptureDevice but we might want to > allow queering or getting the capabilities before calling AllocateAndStart. Thanks for the background and the spec explanation, that's an area where my understanding is relatively weak. What I understand now is that constraints are passed into getUserMedia, but the spec also requires the constraints of the source to be communicated back via the |capabilities| member. At what point in the lifetime of the track is the capabilities() member expected to be populated? Is it only after the onstarted() event fires? Which IPC is this info going to be sent back in? The two options that seem plausible to me are MediaStreamMsg_StreamGenerated or VideoCaptureMsg_DeviceInfo . Currently the Device does not exist at all at the time the StreamGenerated message is being crafted.
Hi perkj@ scherkus@ ncarter@, first apologies for the long delay, I have been rethinking a bit this CL and guess now it'll be clearer. Some general comments: 1) the general working of VCD::GetDeviceSupportedFormats has changed according to conversations with perkj@: It is now reporting the supported formats asynchronously via a VCD::EventHandler::OnDeviceSupportedFormatsEnumerated (long huh?). The returned capabilities vector is allocated inside the device driver, like the other callbacks. Hence the call signature is now much simpler: void GetDeviceSupportedFormats(scoped_ptr<EventHandler> client); 2) ATM, the getUserMedia works on zero knowledge of the underlying device capabilities. Getting any information out of the device is better than nothing, right? This CL is a first step in a series to address this issue. Hence it is not clear when or how this method will be called; in my view it should be called before AllocateAndStart(), but I see no trouble in some second client calling the same method with the device running, and receiving a filtered list of capabilities... 3) Attending to ncarter@ suggestion, I provided an empty implementation of the method and the callback, hence the CL is less daunting now :) 4) There is clean up to do in video_capture_types like you all said --> http://crbug.com/297597 5) About the Linux implementation, due to the 2 threads internally present (IOThread and V4L2), on GetDeviceSupportedFormats, a PostTask is used to jump to V4L2 thread, more concretely to OnGetDeviceSupportedFormats(). Plus, I think I addressed all your comments :D BIG thanks! https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:256: VideoCaptureFormats* capture_formats) { On 2013/09/13 10:33:05, perkj wrote: > To be threadsafe - all v4l2 calls in this file is done on a separate thread > |v4l2_thread_| - this is since v4l2 require us to pull frames from a camera. > > You will need to decide on if you want to call this before or after Allocate- > Allocate open the driver so you should not do it again. > > Would it make more sence to move this code to OnAllocate and let the supported > formats be returned in the observer interface? > Also consider moving the requirement for a capture format to the Start method? Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:257: On 2013/09/12 17:56:00, scherkus wrote: > remove blank line Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:260: int fd = open(device_name_.id().c_str(), O_RDONLY); On 2013/09/12 17:56:00, scherkus wrote: > this should be wrapped with HANDLE_EINTR() Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:262: return; On 2013/09/12 17:56:00, scherkus wrote: > DPLOG(ERROR) << "Couldn't open " << device_name_.id(); > > DPLOG will provide the posix last error message Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:264: media::VideoCaptureCapability capture_format; On 2013/09/12 17:56:00, scherkus wrote: > remove media:: Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:271: // Retrieve the caps one by one, first get colorspace, then sizes, then On 2013/09/12 17:56:00, scherkus wrote: > instead of having this large block of code indented, check for the error > condition and return early Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:276: capture_format.color = media::V4l2ColorToVideoCaptureColorFormat( On 2013/09/12 17:56:00, scherkus wrote: > remove media:: Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:276: capture_format.color = media::V4l2ColorToVideoCaptureColorFormat( On 2013/09/12 17:56:00, scherkus wrote: > do we have to cast here? > > it seems we don't have to on line 418 and as far as I can tell the types are the > same Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:288: // TODO(mcasas) see crbug.com/249953 On 2013/09/12 17:56:00, scherkus wrote: > nit: use http:// in front of http://crbug.com (many text editors 'n web browsers will > link-ify the result when viewing the source) Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:288: // TODO(mcasas) see crbug.com/249953 Is an easy elaborate: support these devices :) https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:291: // Same here as the case before: TODO(mcasas) see crbug.com/249953 On 2013/09/12 17:56:00, scherkus wrote: > ditto Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:292: NOTIMPLEMENTED(); I don't know this particular case. The proposed use case for this function as a whole is to add the getUserMedia call to decide if a particular resolution (and frame rate etc) as requested by the user, is possible. Right now there is no information at all coming from the device to support this decision. So if we get any information out of it, it would be already an improvement. That was my rationale for all these NOTIMPLEMENTED and let it continue, deferring the semantics to higher levels... https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:301: (frame_interval.discrete.numerator != 0) On 2013/09/12 17:56:00, scherkus wrote: > I'd just use an if () statement here -- the ?: operator isn't making things easy > to read in this case Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:307: // Same here as the case above: TODO(mcasas) see crbug.com/249953 On 2013/09/12 17:56:00, scherkus wrote: > nit: try to start TODO comments at the beginning of a comment > > // TODO(mcasa): Same here as above, see http://crbug.com/249953 Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:312: NOTIMPLEMENTED(); This is an easier case: yes, some devices might not be willing to share the video frame rates (think auto exposure types), but if width+height are present, that'd be an improvement. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:315: capture_formats->insert(capture_formats->end(), capture_format); On 2013/09/12 17:56:00, scherkus wrote: > push_back() ? Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_device.h:229: // supported formats limited etc. Asynchronous reply would be the answer, let me write some notes in the next iteration. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_device.h:230: virtual void GetDeviceSupportedFormats( On the asynchronous reply, it would be empty. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_device.h:231: VideoCaptureFormats* capture_formats) = 0; On 2013/09/12 17:29:09, scherkus wrote: > this CL could be dramatically simplified if you provided a default > implementation in the .cc Done. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_types.h:80: typedef std::vector<media::VideoCaptureCapability> VideoCaptureFormats; On 2013/09/12 17:56:00, scherkus wrote: > it seems unfortunate that we have to use VideoCaptureCapability considering we > only set color, width, height, and frame_rate > > what about expected_capture_delay, interlaced? > > session_id also seems very out of place TODO() check if those parameters are still in use anywhere. https://codereview.chromium.org/24079003/diff/15001/media/video/capture/video... media/video/capture/video_capture_types.h:82: } // namespace media On 2013/09/13 19:02:19, ncarter wrote: > On 2013/09/13 10:33:05, perkj wrote: > > yes- session_id seems wrong to have at all. > > > > Can you help checkout and write a comment what VideoCaptureResolutionType > > frame_size_type is? Associated enum names are self explanatory IMHO, either a constant resolution device or one that changes according to its own volition. > > Is interlaced used by anyone? Is there even a reason to separate > > VideoCaptureFormat and VideoCaptureCapability? Wouldn't it make more sence to > > combine them? Perhaps. I should fill a TODO/crbug on this, but is beyond the scope of this CL. > > Here's the information needs I see: > > VCDevice <-> VCDevice::EventHandler (needs the info: width, height, colorspace, > stride etc. Does NOT have a session_id because a device can serve multiple > sessions.) > > VCDevice::EventHandler <-> VCController (needs width and height, but colorspace > is implicitly I420. Does not have a session_id, because that's per-client > state.) > > VCController/VCManager <-> VCHost <-> VideoCapture <-> VideoCaptureImpl <-> > delegates (needs width and height. The session_id can be included but is > redundant unless it is in the context of the first (Start) request, in which > case it's a required parameter. Pixel format is always implicitly I420, and > should never be included.) > > As far as I can tell, |interlaced| serves no purpose anywhere. It's not clear to > me (because I haven't looked) how important it is that we send frame_rate in > responses everywhere, but I don't see it as hurting anything and can be bundled > with width and height. See above. TODO to further consolidate this file's classes. > > I don't think that VideoCaptureResolutionType as currently formulated is > altogether coherent. It's not clear to me whether it's a property of the device, > a mode of the device, or a constraint of the receiver. It seems like it's > actually a mix. If I've read the code correctly, the PPAPI clients can't deal > with variable resolutions, but the webrtc clients can? So in that sense it's a > constraint of the client. But also, only one of the devices is geared up to > support the variable resolution mode, so it's also a capability of the device. Is both a command to the device to be configured so and so and also a reply of the device.
Hej Some more comments. Main thing is the callback- you can not use the EventHandler as it is used now. It would probably be easier and maybe better to use base::CallBack instead for this. Especially if we are not sure that the call to retrieve the capabilities will come from the VCC. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/fake_... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/54001/media/video/capture/fake_... media/video/capture/fake_video_capture_device.h:39: int GetFrameRate() const; What is GetFrameRate and why? Make private or just put it in the cc file if it does not need to be visible. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:277: if (v4l2_thread_.IsRunning()) { |v4l2_thread| is started in Allocate - so this still means you can not call GetDeviceSupportedFormats before the camera has been started. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:455: int fd; please put the code for opening the driver in one method and reuse in both cases. Also add a method for checking if it has already been opened and use it in both places. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:470: // Test if this is a V4L2 capture device. No need to do this again. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:531: close(fd); What if we are already capturing? Does it work to enumerate while capturing? Do not close the driver in that case..... https://codereview.chromium.org/24079003/diff/54001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/54001/media/video/capture/video... media/video/capture/video_capture_device.h:224: virtual void GetDeviceSupportedFormats(scoped_ptr<EventHandler> client) {}; Instead of |client| beeing an EventHandler - it might be easier to just let it be of type base::CallBack, see MediaRequestResponseCallback for an example. Otherwise you will have to rethink what a |client| is and when it is provided - ie - before AllocateAndStart. You don't want to delete the VCC just because you wanted to get the available formats. You should also make sure GetDevicesSupportedFormats always return- even if an error occur or the function is not implemented. It will be very hard to use unless you know that you always get a result. Have you thought about how this will be used?
Hi perkj@, addressed smaller things and then some slightly larger changes: - Changed the parameter of GetDeviceSupportedFormats to be a Callback function, which is called in all cases, in case of internal error, and empty vector is returned as the function parameter. - On Linux implementation, I added checks for |v4l2_thread_| being running and |device_fd_| being opened. If the GetDeviceSupportedFormats is called and the device file is not open, it is opened and closed before the callback. If the thread is not running, it is launched exclusively for the query, and to prevent race conditions with other thread launching/killing operations, it's protected with an autolock. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/fake_... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/54001/media/video/capture/fake_... media/video/capture/fake_video_capture_device.h:39: int GetFrameRate() const; Calculates the frame rate of the fake device. We need a method because it needs a calculation. I'll put it as private. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:277: if (v4l2_thread_.IsRunning()) { If |v4l2_thread_| is not running, I'll switch it on for the query and post a Stop() call immediately afterwards. To prevent race conditions, all individual Start and Stop operations on the thread are lock protected; these 3 just mentioned are protected in bulk, IOW Start-Query-Stop are executed in critical section as well. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:455: int fd; On 2013/09/25 07:58:41, perkj wrote: > please put the code for opening the driver in one method and reuse in both > cases. > Also add a method for checking if it has already been opened and use it in both > places. Done. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:470: // Test if this is a V4L2 capture device. If this method is called _before_ Allocate+Init, we need to check. Made a static method bool IsDeviceTypeCaptureOrOutput(int fd) . https://codereview.chromium.org/24079003/diff/54001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:531: close(fd); Device file is only close if it was closed whenever the method was called. https://codereview.chromium.org/24079003/diff/54001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/54001/media/video/capture/video... media/video/capture/video_capture_device.h:224: virtual void GetDeviceSupportedFormats(scoped_ptr<EventHandler> client) {}; On 2013/09/25 07:58:41, perkj wrote: > Instead of |client| beeing an EventHandler - it might be easier to just let it > be of type base::CallBack, see MediaRequestResponseCallback for an example. > Otherwise you will have to rethink what a |client| is and when it is provided - > ie - before AllocateAndStart. You don't want to delete the VCC just because you > wanted to get the available formats. > > You should also make sure GetDevicesSupportedFormats always return- even if an > error occur or the function is not implemented. It will be very hard to use > unless you know that you always get a result. > > Have you thought about how this will be used? Done.
Regarding the patch overall, it's tough to decide whether this is the right interface because it seems like we don't know how the underlying functionality will be used. The questions that need to be answered are: - What entity in the stack decides to request this information? - At what stage in the lifetime of the stream? Has the device already been opened at this point? - How does the request get from the requester, to the Device, and back? It sounds like perkj might have some specific ideas about this (having expressed a strong opinion about using EventHandler versus callbacks). I really think that in general the VCD ought to be in the business of a conversation with its controller -- having callbacks that go elsewhere are something we should avoid. Big picture, with today's architecture, I see two broad options: - Option 1: we do the work of GetDeviceSupportedFormats as part of VideoCaptureDevice::GetDeviceNames, and mediastreammanager caches/forwards the result when the stream is opened. - Option 2: We do the work of GetDeviceSupportedFormats after the Device is created. This is going to require reworking the VCM/VCC/VCD architecture somewhat, either creating the device much earlier than we do today, or else adding a new phase to device startup where we negotiate constraints. https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:282: base::AutoLock lock(lock_); Does this lock provide any actual sequentialization? It seems like it's only ever called from the public methods of VideoCaptureDevice, which are only ever called from the device thread. Moreover, if there were contention for this lock, then wouldn't calling Stop() while holding the lock be a recipe for deadlock? https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:298: if (!v4l2_thread_.IsRunning()) { In practice are we really going to exercise both sides of this branch ? I would be surprised if the answer is yes. https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:311: v4l2_thread_.Stop(); This code appears to be starting a thread, blocking for the thread to complete, then joining the thread. This seems like a very expensive way of calling a function. https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:357: ++fmtdesc.index; Why? https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:514: ++timeout_count_; What was wrong with postincrement? https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.h:66: int OpenDeviceFile(int oflag = O_RDONLY); http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments
Nick is right- we should figure out how this will be used. The reason that I suggested a callback was because I hoped that would mean that regardless of who would request the capabilities - it would still be usefull. I was originally sure that this should be used through the VCC and VCM and be available before the capturer is started. That is were I think this logically belong. But Tommi said that we might need and an extension API for queering devices and its capabilities for other purposes than MediaStreams. Also Ben suggested said that the current constraint language can be interpreted as that we should match the constraints with all available devices and pick the device that best matches the constraint.... That means we need to now the capabilities before we select a camera. So these two last use cases suggest that GetDeviceSupportedFormats will be used similarly as GetDeviceNames but not necessarily combined as one method.
@nick let me complement what perkj@ is saying The requester of the device capabilities(a.k.a caps) might be an entity different from VCC, in particular thinking about the VCManager which, IINM, creates the VCD, way before the VCD::Allocate+Start process. The idea would be that this VCM or a similar pre-data-pushing-activity entity would get the caps, indeed near in time to GetDeviceNames (whenever the user has given permission etc. of course). This is the reason of the locks and the literal callback (as opposed to interface-callback). I see another scenario in which a second tab via gUM requests caps and the same VCD would return an _reduced_ version of the caps, i.e. smaller resolutions and/or slower framerates, in a not-reopen-device world. perkj@ thinks this is a job for some VCM or similar, caching the original caps and filtering them, but I would say this is the device's responsability. This would be the scenario in which GetDeviceSupportedFormats does not launch the |v4l2_thread_|, as you mention in your comments.
Comments addressed. Next patch coming soon, got a bit stuck in the rebase though. https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:282: base::AutoLock lock(lock_); See larger reply, there might be two entities calling methods in VCD. > Moreover, if there were contention for this lock, then wouldn't calling Stop() > while holding the lock be a recipe for deadlock? I don't see it yet, the lock belongs to the VCD thread and the stop is an asynchronous operation on the |v4l2_thread_|, right? https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:298: if (!v4l2_thread_.IsRunning()) { On 2013/10/02 22:15:15, ncarter wrote: > In practice are we really going to exercise both sides of this branch ? I would > be surprised if the answer is yes. See larger explanation. https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:310: // Note that Stop Oops my bad! https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:311: v4l2_thread_.Stop(); On 2013/10/02 22:15:15, ncarter wrote: > This code appears to be starting a thread, blocking for the thread to complete, > then joining the thread. This seems like a very expensive way of calling a > function. I also think that launching this thread just for retrieving the caps is a bit overkill, but it does not break the code orthogonality (all device operations in the same |v4l2_thread_| thread). https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:357: ++fmtdesc.index; Preincrements are preferred by Google C++ for objects [1] and complex entities - not enforced for simple types, and someone raised up the topic far in the past, so I change them dutifully. [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a... https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:514: ++timeout_count_; See previous comment on line 357. https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.h (right): https://codereview.chromium.org/24079003/diff/66001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.h:66: int OpenDeviceFile(int oflag = O_RDONLY); On 2013/10/02 22:15:15, ncarter wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments Done.
Ok, after a lenghty conversation with perkj@, this is the new mechanisms :) VideoCaptureManager will query the device caps via a static method in VideoCaptureDevice, the same way it does with GetDeviceNames. This way, there would be no need for locking and no need for opening the device (Linux, in this CL). VCM will cache the capabilities and keep them, potentially filtering them for later clients. There would _not_ be any queries of the caps after this original moment of the system, except when new devices are added, same as now. This means ncarter@ option 1 :) I'll update the code, until then no need for review :)
perkj@, ncarter@, PTAL!
This I think make sense. But I would prefer to filter the result of GetCaptureCapabilites in the implementation. I don't see the point in returning a list of all supported color formats and the individual resolutions and frame rate. Pick the most efficient color format and the max frame rate per resolution. https://codereview.chromium.org/24079003/diff/82001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/82001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:158: // Test if this is a V4L2 capture device. no need for this - the |device| must come from GetDeviceNames. https://codereview.chromium.org/24079003/diff/82001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:173: while (ioctl(fd, VIDIOC_ENUM_FMT, &pixel_format) == 0) { I don't think this function should return duplicate resolutions. - If we return duplicates we need to filter them before returning them to JS I think. It does not matter for the user of this method what color space it is as long as we support color conversion to I420. Why not let this method pick the most efficient color space and then enumerate the supported resolutions? https://codereview.chromium.org/24079003/diff/82001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:194: while (ioctl(fd, VIDIOC_ENUM_FRAMEINTERVALS, &frame_interval) == 0) { I don't think we care about all frame rates either- just the highest frame rate for a certain resolution. https://codereview.chromium.org/24079003/diff/82001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/82001/media/video/capture/video... media/video/capture/video_capture_device.h:198: VideoCaptureFormats* const formats); remove const - please see GetDeviceNAmes. https://codereview.chromium.org/24079003/diff/82001/media/video/capture/video... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/24079003/diff/82001/media/video/capture/video... media/video/capture/video_capture_types.h:82: typedef std::vector<VideoCaptureCapability> VideoCaptureFormats; VideoCaptureCapabilities? does this typedef already exist somewhere maybe?
Hi perkj@, PTAL. A general comment is that IMHO the GetDeviceCapabilities should reply a unfiltered list of those, passing all the raw info to some higher entity where the algorithm gUM-requested-caps-versus-device-possibilities will be carried out. The higher entity can then filter them as seen fit. Enumerating all linux caps (56) of C910 takes less than 8ms (unittests). https://codereview.chromium.org/24079003/diff/82001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/82001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:158: // Test if this is a V4L2 capture device. On 2013/10/13 12:12:48, perkj wrote: > no need for this - the |device| must come from GetDeviceNames. Done. https://codereview.chromium.org/24079003/diff/82001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:173: while (ioctl(fd, VIDIOC_ENUM_FMT, &pixel_format) == 0) { On 2013/10/13 12:12:48, perkj wrote: > I don't think this function should return duplicate resolutions. - If we return > duplicates we need to filter them before returning them to JS I think. > It does not matter for the user of this method what color space it is as long as > we support color conversion to I420. > Why not let this method pick the most efficient color space and then enumerate > the supported resolutions? This information is not for the user, but for the getUserMedia matching algorithm. Following our conversations, IIRC we agreed that the intelligence of it would be somewhere else but in the device. The device lists and send _once_ all those capabilities; if it would prefilter them, we'd effectively be dividing the algorithm in several places, and there being so many device implementations, that would be brittle. A factual example: hardware encoders and VP9 support more than I420. We'll need to expand the current infra to support RGBA. Screen capture works in RGBA, and now there's a hardcoded csc to I420. https://codereview.chromium.org/24079003/diff/82001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:194: while (ioctl(fd, VIDIOC_ENUM_FRAMEINTERVALS, &frame_interval) == 0) { On 2013/10/13 12:12:49, perkj wrote: > I don't think we care about all frame rates either- just the highest frame rate > for a certain resolution. One of the bugs related to this is asking for a frame rate that the device cannot provide because is too low. Again, my comments before, I think we should send all the capabilities. https://codereview.chromium.org/24079003/diff/82001/media/video/capture/video... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/82001/media/video/capture/video... media/video/capture/video_capture_device.h:198: VideoCaptureFormats* const formats); On 2013/10/13 12:12:49, perkj wrote: > remove const - please see GetDeviceNAmes. Done. https://codereview.chromium.org/24079003/diff/82001/media/video/capture/video... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/24079003/diff/82001/media/video/capture/video... media/video/capture/video_capture_types.h:82: typedef std::vector<VideoCaptureCapability> VideoCaptureFormats; On 2013/10/13 12:12:49, perkj wrote: > VideoCaptureCapabilities? does this typedef already exist somewhere maybe? Yes, it exists in webrtc device_info_impl.h, but that's webrtc @ third_party. I chose VideoCaptureFormats because, in time, VCC could/should/ought to be removed and all simplified to just VCFormat, like ncarter@ and you pointed out.
ok - I think we should land this and hook it up to JS and getusermedia to learn more about if/where the formats should be filtered. Please fix the below comments first though. https://codereview.chromium.org/24079003/diff/89001/media/video/capture/fake_... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/89001/media/video/capture/fake_... media/video/capture/fake_video_capture_device.h:29: VideoCaptureFormats* const formats); VideoCaptureFormats* const -> VideoCaptureFormats* everywhere. https://codereview.chromium.org/24079003/diff/89001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/89001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:166: V4l2ColorToVideoCaptureColorFormat((int32)pixel_format.pixelformat); If you look at V4l2ColorToVideoCaptureColorFormat - it only support a very limited set for color spaces. So this will DCHECK on a number of devices. Please change this to just print out the formats we don't support and make sure they are not added to |formats|. (interestingly I don't see this function being used at all previously). https://codereview.chromium.org/24079003/diff/89001/media/video/capture/video... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/24079003/diff/89001/media/video/capture/video... media/video/capture/video_capture_types.h:82: typedef std::vector<VideoCaptureCapability> VideoCaptureFormats; I saw your response but since this is a VideoCaptureCapability and it is being used elsewere - please call this VideoCaptureCapabilities and change the name if we later change the rest of the implementation to use VideoCaptureFormat. I think this naming just adds to the confusion at the moment.
hi perkj@ could you PTAL? Thanks! ncarter@ seems like the CL is getting more stable and narrow in scope, could you PTAL? https://codereview.chromium.org/24079003/diff/89001/media/video/capture/fake_... File media/video/capture/fake_video_capture_device.h (right): https://codereview.chromium.org/24079003/diff/89001/media/video/capture/fake_... media/video/capture/fake_video_capture_device.h:29: VideoCaptureFormats* const formats); On 2013/10/15 08:42:54, perkj wrote: > VideoCaptureFormats* const -> VideoCaptureFormats* everywhere. Done. https://codereview.chromium.org/24079003/diff/89001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/89001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:166: V4l2ColorToVideoCaptureColorFormat((int32)pixel_format.pixelformat); Added the remaining colourspaces to V4l2...(); added a check in this line to skip unknown colorspaces. https://codereview.chromium.org/24079003/diff/89001/media/video/capture/video... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/24079003/diff/89001/media/video/capture/video... media/video/capture/video_capture_types.h:82: typedef std::vector<VideoCaptureCapability> VideoCaptureFormats; On 2013/10/15 08:42:54, perkj wrote: > I saw your response but since this is a VideoCaptureCapability and it is being > used elsewere - please call this VideoCaptureCapabilities and change the name if > we later change the rest of the implementation to use VideoCaptureFormat. I > think this naming just adds to the confusion at the moment. Done.
Sorry - I missed the existence of GetListOfUsableFourCCs and how it is used previously. https://codereview.chromium.org/24079003/diff/97001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/97001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:180: capture_format.color = Sorry, my fault - I just saw GetListOfUsableFourCCs that is being used to decide if you can open the camera with a certain color space. You do not want to change this list in this cl.... GetDeviceSupportedFormats should of course only return those formats that can be used in AllocateAndStart. So please remove V4l2ColorToVideoCaptureColorFormat all together and use GetDeviceSupportedFormats to match the color space with.
perkj@: reverted changes in V4l2ColorToVideoCaptureColorFormat() (except debug output).
lgtm if the below is addressed https://codereview.chromium.org/24079003/diff/98001/media/video/capture/linux... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/24079003/diff/98001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:73: DCHECK_NE(result, PIXEL_FORMAT_UNKNOWN) << std::hex << v4l2_fourcc; replace this dcheck with a log. Otherwise your function will crash on debug builds if it is not one of these formats. So the camera you tested with don't support any other formats? https://codereview.chromium.org/24079003/diff/98001/media/video/capture/linux... media/video/capture/linux/video_capture_device_linux.cc:166: V4l2ColorToVideoCaptureColorFormat((int32)pixel_format.pixelformat); ok if you do it this way- can you just add a comment in V4l2ColorToVideoCaptureColorFormat that it must use the same color spaces as in GetDeviceSupportedFormats.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/24079003/114001
Message was sent while issue was closed.
Change committed as 228904 |
