Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7002027:
VideoCaptureHost (Closed)
|
Created:
9 years, 7 months ago by Per K Modified:
9 years, 3 months ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., acolwell GONE FROM CHROMIUM, annacc, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), mflodman1 Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionVideoCaptureHost
This is the patch containing code necessary for communicating with the VideoCaptureMessageFilter from the browser process and transferring Transport Dibs filled with video frames in I420 color format. It also contain code for color converting video frames to I420.
Color conversion has been tested on Linux and Windows by using video_capture_host_unittest and dumping I420 frames to file. See
#define DUMP_VIDEO #define TEST_REAL_CAPTURE_DEVICE.
patch by perk@google.com
BUG=none
TEST=try bots
Patch Set 1 #
Total comments: 22
Patch Set 2 : '' #
Total comments: 36
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 110
Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : '' #
Total comments: 1
Messages
Total messages: 18 (0 generated)
I didn't look at coding style that much. The main concern is the interface between Host and Memory. see comments for more details. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... File content/browser/renderer_host/video_capture_host.cc (right): http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.cc:114: const IPC::Message& msg, int device_id, indent http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.cc:134: vc_entries_.insert(std::make_pair(device_id, memory_handler)); Some thoughts about the modules: 1. Host only needs to know device_id, routing_id and memory_handler. It doesn't need to know session_id. 2. memory_handler doesn't need to know routing_id and device_id. 3. the communication interface between Host and Memory is VideoCaptureMemory::EventHandler. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.cc:140: VideoCaptureMemory* memory_handler = LookupStartedDevicesById(device_id); would it be good to pass Stop command to memory_handler and let it handle all buffers? when memory_handler has received all DIB's, it's stopped and call a event handling function to notify Host. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.cc:226: media::VideoCapture::kError)); Is it needed for renderer process to distinguish different errors? If so, error code is needed. It's a TODO in renderer process. It would be great if you can add some error codes from device point of view. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... File content/browser/renderer_host/video_capture_host.h (right): http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:5: // VideoCaptureHost serves video capture related requests from VideCaptureImpl The counterpart of VideoCaptureHost is VideoCaptureMessageFilter in renderer process. VideoCaptureMessageFilter takes care of message routing and device_id, while VideoCaptureImpl presents video capture device to its clients. VideoCaptureImpl is the major horse to generate messages. The device_id is created by VCMessageFilter and assigned to a VCImpl. With this information, could you rephrase some comments (here and below) related to modules in renderer process? http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:99: void OnDeviceStoped(int device_id); OnDeviceStopped http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:104: void DoDeviceStoped(int device_id); ditto http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:132: VideoCaptureMemory* LookupStopedDevicesById(int device_id); ditto http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:139: VCEntryMap vc_stoped_entries_; ditto http://codereview.chromium.org/7002027/diff/1/media/video/capture/video_captu... File media/video/capture/video_capture_color_conversion.cc (right): http://codereview.chromium.org/7002027/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_color_conversion.cc:103: 128) >> 8) + 128; looks like downsampled U and V are located at [0,0] position in a 2x2 sub-region. is this due to consideration of computation? What about possible alias? any plan to have optimization, such as assembly? In media/base/, there are some files containing color conversion functions. any possibility to consolidate? http://codereview.chromium.org/7002027/diff/1/media/video/capture/video_captu... File media/video/capture/video_capture_color_conversion.h (right): http://codereview.chromium.org/7002027/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_color_conversion.h:14: class VideoCaptureColorConversion { is this color conversion only for video capture, or can be used for other cases?
http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... File content/browser/renderer_host/video_capture_host.cc (right): http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.cc:114: const IPC::Message& msg, int device_id, On 2011/05/11 23:23:31, wjia wrote: > indent Done. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.cc:134: vc_entries_.insert(std::make_pair(device_id, memory_handler)); On 2011/05/11 23:23:31, wjia wrote: > Some thoughts about the modules: > 1. Host only needs to know device_id, routing_id and memory_handler. It doesn't > need to know session_id. > 2. memory_handler doesn't need to know routing_id and device_id. > 3. the communication interface between Host and Memory is > VideoCaptureMemory::EventHandler. VCEntryId moved to VideoCaptureHost and called VideoCaptureEntry. VideoCaptureMemory still have a member called device_id but this is given to the VideoCaptureMemory when it is constructed. This is used for identification so that it can always be used in the entries_ map for quick lookup. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.cc:140: VideoCaptureMemory* memory_handler = LookupStartedDevicesById(device_id); On 2011/05/11 23:23:31, wjia wrote: > would it be good to pass Stop command to memory_handler and let it handle all > buffers? when memory_handler has received all DIB's, it's stopped and call a > event handling function to notify Host. Done. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.cc:226: media::VideoCapture::kError)); On 2011/05/11 23:23:31, wjia wrote: > Is it needed for renderer process to distinguish different errors? If so, error > code is needed. It's a TODO in renderer process. It would be great if you can > add some error codes from device point of view. I am not really sure we will be able to distinguish between errors. Most likely the error will occur when you start a capture device. Either because of a hardware problem or that the user has unplugged it after it was opened or another application is using it. I don't plan to add any codes at the moment. Most other errrors I would assume only happen if we have made something wrong in the code. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... File content/browser/renderer_host/video_capture_host.h (right): http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:5: // VideoCaptureHost serves video capture related requests from VideCaptureImpl On 2011/05/11 23:23:31, wjia wrote: > The counterpart of VideoCaptureHost is VideoCaptureMessageFilter in renderer > process. VideoCaptureMessageFilter takes care of message routing and device_id, > while VideoCaptureImpl presents video capture device to its clients. > VideoCaptureImpl is the major horse to generate messages. The device_id is > created by VCMessageFilter and assigned to a VCImpl. > > With this information, could you rephrase some comments (here and below) related > to modules in renderer process? Done. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:99: void OnDeviceStoped(int device_id); On 2011/05/11 23:23:31, wjia wrote: > OnDeviceStopped Done. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:104: void DoDeviceStoped(int device_id); On 2011/05/11 23:23:31, wjia wrote: > ditto Done. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:132: VideoCaptureMemory* LookupStopedDevicesById(int device_id); On 2011/05/11 23:23:31, wjia wrote: > ditto Done. http://codereview.chromium.org/7002027/diff/1/content/browser/renderer_host/v... content/browser/renderer_host/video_capture_host.h:139: VCEntryMap vc_stoped_entries_; On 2011/05/11 23:23:31, wjia wrote: > ditto Done. http://codereview.chromium.org/7002027/diff/1/media/video/capture/video_captu... File media/video/capture/video_capture_color_conversion.cc (right): http://codereview.chromium.org/7002027/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_color_conversion.cc:103: 128) >> 8) + 128; On 2011/05/11 23:23:31, wjia wrote: > looks like downsampled U and V are located at [0,0] position in a 2x2 > sub-region. is this due to consideration of computation? What about possible > alias? > > any plan to have optimization, such as assembly? > > In media/base/, there are some files containing color conversion functions. any > possibility to consolidate? Would you prefer if the conversion functions are put in yuv_converter.cc instead? I put them here so we would not need to change existing files. We have no plans to do any assembly optimizations at the moment. Added todo in the code and emailed hclam. http://codereview.chromium.org/7002027/diff/1/media/video/capture/video_captu... File media/video/capture/video_capture_color_conversion.h (right): http://codereview.chromium.org/7002027/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_color_conversion.h:14: class VideoCaptureColorConversion { On 2011/05/11 23:23:31, wjia wrote: > is this color conversion only for video capture, or can be used for other cases? It has been made private to VideoCapture by purpose. But sure- it is standard conversions to I420.
nice work!
nice work! http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host.cc (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:17: entries_.clear(); the dtor of entries_ will do clear() by default. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:141: new VideoCaptureMemory(memory_id, this); does Memory have to know memory_id? I think it's redundant since Host can always find memory_id with Memory pointer. It's up to you though. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:156: // It does not exist so it must have been stopped already. indent http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:175: device_id); indent http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:184: (VideoCaptureMemory::VideoCaptureMemoryId id) { the open "(" would go with function name. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:189: media::VideoCapture::kStopped)); "kStopped" could be sent multiple time for a device_id. right now, renderer process doesn't recycle device_id immediately (the range is 32 bits). this should work fine. In case renderer process re-uses device_id immediately after it gets "stopped" signal, this multiple "kStopped" message could be a problem. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host.h (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.h:75: scoped_refptr<VideoCaptureMemory> >EntryMap; move typedef to where it's used. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host_unittest.cc (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:106: need only one blank line. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:199: .WillByDefault(Return()); there is a gmock warning when unit test is run: GMOCK WARNING: Uninteresting mock function call - taking default action specified at: content/browser/renderer_host/video_capture_host_unittest.cc:200: Function call: OnStateChanged(200, 1, 2) http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_memory.cc (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:20: event_handler_(*event_handler), any reason to duplicate event_handler? http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:27: owned_dibs_.clear(); dtor of std::map will clear the objects. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:55: bool ready_to_delete = false; ready_to_delete will be set by line 58 anyway. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:108: #if defined(OS_WIN) // RGB on Windows start at the bottom line. all # should start at the beginning of the line. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:137: frame_info_.width, frame_info_.width / 2); indent http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:159: TransportDIB* dib = TransportDIB::Create(needed_size, i); is it good to move dib creation out of critical section? Only put code changing owned_dibs_ and free_dibs_ in critical section? Not sure how long it will take to create one DIB. it's always better to minimize critical section. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:179: // This will report to the event handler that it object is ready to delete "the object" or "this object" http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:182: bool ready_to_delete_now = false; ditto. this variable will set below anyway. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_memory.h (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.h:81: typedef std::list<TransportDIB::Handle> DIBHANDLEList; typedef precedes where it's used.
Thanks. Do you think I should change the name of VideoCaptureMemory to VideoCaptureController now since the class does not only manages DIBS? / Per http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host.cc (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:17: entries_.clear(); On 2011/05/19 04:24:37, wjia wrote: > the dtor of entries_ will do clear() by default. Removed. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:141: new VideoCaptureMemory(memory_id, this); On 2011/05/19 04:24:37, wjia wrote: > does Memory have to know memory_id? I think it's redundant since Host can always > find memory_id with Memory pointer. It's up to you though. True- but this way we don't need to iterate through the map to find the routing_id given a callback from VideoCapatureMemory. The key consist of a pair of routing id and device_id. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:156: // It does not exist so it must have been stopped already. On 2011/05/19 04:24:37, wjia wrote: > indent Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:175: device_id); On 2011/05/19 04:24:37, wjia wrote: > indent Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:184: (VideoCaptureMemory::VideoCaptureMemoryId id) { On 2011/05/19 04:24:37, wjia wrote: > the open "(" would go with function name. Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:189: media::VideoCapture::kStopped)); On 2011/05/19 04:24:37, wjia wrote: > "kStopped" could be sent multiple time for a device_id. right now, renderer > process doesn't recycle device_id immediately (the range is 32 bits). this > should work fine. In case renderer process re-uses device_id immediately after > it gets "stopped" signal, this multiple "kStopped" message could be a problem. This can happen if an Error occur and the message filter also call Stop. As I wrote in the email an error will trigger the state to change to first kError and then kStopped when all Dibs are returned even though the message filter have not requested the camera to stop. Is the client ok with this? http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host.h (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.h:75: scoped_refptr<VideoCaptureMemory> >EntryMap; On 2011/05/19 04:24:37, wjia wrote: > move typedef to where it's used. Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host_unittest.cc (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:106: On 2011/05/19 04:24:37, wjia wrote: > need only one blank line. Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:199: .WillByDefault(Return()); On 2011/05/19 04:24:37, wjia wrote: > there is a gmock warning when unit test is run: > > GMOCK WARNING: > Uninteresting mock function call - taking default action specified at: > content/browser/renderer_host/video_capture_host_unittest.cc:200: > Function call: OnStateChanged(200, 1, 2) Changed to Expect_Call Anynumber. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_memory.cc (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:20: event_handler_(*event_handler), On 2011/05/19 04:24:37, wjia wrote: > any reason to duplicate event_handler? Sorry I don't understand? event_handler_ is a reference? http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:27: owned_dibs_.clear(); On 2011/05/19 04:24:37, wjia wrote: > dtor of std::map will clear the objects. removed. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:55: bool ready_to_delete = false; On 2011/05/19 04:24:37, wjia wrote: > ready_to_delete will be set by line 58 anyway. Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:108: #if defined(OS_WIN) // RGB on Windows start at the bottom line. On 2011/05/19 04:24:37, wjia wrote: > all # should start at the beginning of the line. Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:137: frame_info_.width, frame_info_.width / 2); On 2011/05/19 04:24:37, wjia wrote: > indent Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:159: TransportDIB* dib = TransportDIB::Create(needed_size, i); On 2011/05/19 04:24:37, wjia wrote: > is it good to move dib creation out of critical section? Only put code changing > owned_dibs_ and free_dibs_ in critical section? Not sure how long it will take > to create one DIB. it's always better to minimize critical section. Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:179: // This will report to the event handler that it object is ready to delete On 2011/05/19 04:24:37, wjia wrote: > "the object" or "this object" Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.cc:182: bool ready_to_delete_now = false; On 2011/05/19 04:24:37, wjia wrote: > ditto. this variable will set below anyway. Done. http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_memory.h (right): http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_memory.h:81: typedef std::list<TransportDIB::Handle> DIBHANDLEList; On 2011/05/19 04:24:37, wjia wrote: > typedef precedes where it's used. Done.
Changed name of VideoCaptureMemory to VideoCaptureController to better fit the new behavior. Updated patch to svn cl 86049. Wei- can you help me find another reviewer if you think this patch is good that can help us land this when cl http://codereview.chromium.org/6946001 is landed? Thanks On 2011/05/19 08:55:31, Per K wrote: > Thanks. > Do you think I should change the name of VideoCaptureMemory to > VideoCaptureController now since the class does not only manages DIBS? > > / > Per > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > File content/browser/renderer_host/video_capture_host.cc (right): > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host.cc:17: entries_.clear(); > On 2011/05/19 04:24:37, wjia wrote: > > the dtor of entries_ will do clear() by default. > > Removed. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host.cc:141: new > VideoCaptureMemory(memory_id, this); > On 2011/05/19 04:24:37, wjia wrote: > > does Memory have to know memory_id? I think it's redundant since Host can > always > > find memory_id with Memory pointer. It's up to you though. > > True- but this way we don't need to iterate through the map to find the > routing_id given a callback from VideoCapatureMemory. > The key consist of a pair of routing id and device_id. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host.cc:156: // It does not exist so > it must have been stopped already. > On 2011/05/19 04:24:37, wjia wrote: > > indent > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host.cc:175: device_id); > On 2011/05/19 04:24:37, wjia wrote: > > indent > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host.cc:184: > (VideoCaptureMemory::VideoCaptureMemoryId id) { > On 2011/05/19 04:24:37, wjia wrote: > > the open "(" would go with function name. > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host.cc:189: > media::VideoCapture::kStopped)); > On 2011/05/19 04:24:37, wjia wrote: > > "kStopped" could be sent multiple time for a device_id. right now, renderer > > process doesn't recycle device_id immediately (the range is 32 bits). this > > should work fine. In case renderer process re-uses device_id immediately after > > it gets "stopped" signal, this multiple "kStopped" message could be a problem. > > This can happen if an Error occur and the message filter also call Stop. As I > wrote in the email an error will trigger the state to change to first kError and > then kStopped when all Dibs are returned even though the message filter have not > requested the camera to stop. Is the client ok with this? > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > File content/browser/renderer_host/video_capture_host.h (right): > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host.h:75: > scoped_refptr<VideoCaptureMemory> >EntryMap; > On 2011/05/19 04:24:37, wjia wrote: > > move typedef to where it's used. > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > File content/browser/renderer_host/video_capture_host_unittest.cc (right): > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host_unittest.cc:106: > On 2011/05/19 04:24:37, wjia wrote: > > need only one blank line. > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_host_unittest.cc:199: > .WillByDefault(Return()); > On 2011/05/19 04:24:37, wjia wrote: > > there is a gmock warning when unit test is run: > > > > GMOCK WARNING: > > Uninteresting mock function call - taking default action specified at: > > content/browser/renderer_host/video_capture_host_unittest.cc:200: > > Function call: OnStateChanged(200, 1, 2) > > Changed to Expect_Call Anynumber. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > File content/browser/renderer_host/video_capture_memory.cc (right): > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.cc:20: > event_handler_(*event_handler), > On 2011/05/19 04:24:37, wjia wrote: > > any reason to duplicate event_handler? > > Sorry I don't understand? event_handler_ is a reference? > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.cc:27: owned_dibs_.clear(); > On 2011/05/19 04:24:37, wjia wrote: > > dtor of std::map will clear the objects. > > removed. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.cc:55: bool ready_to_delete = > false; > On 2011/05/19 04:24:37, wjia wrote: > > ready_to_delete will be set by line 58 anyway. > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.cc:108: #if defined(OS_WIN) > // RGB on Windows start at the bottom line. > On 2011/05/19 04:24:37, wjia wrote: > > all # should start at the beginning of the line. > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.cc:137: frame_info_.width, > frame_info_.width / 2); > On 2011/05/19 04:24:37, wjia wrote: > > indent > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.cc:159: TransportDIB* dib = > TransportDIB::Create(needed_size, i); > On 2011/05/19 04:24:37, wjia wrote: > > is it good to move dib creation out of critical section? Only put code > changing > > owned_dibs_ and free_dibs_ in critical section? Not sure how long it will take > > to create one DIB. it's always better to minimize critical section. > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.cc:179: // This will report > to the event handler that it object is ready to delete > On 2011/05/19 04:24:37, wjia wrote: > > "the object" or "this object" > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.cc:182: bool > ready_to_delete_now = false; > On 2011/05/19 04:24:37, wjia wrote: > > ditto. this variable will set below anyway. > > Done. > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > File content/browser/renderer_host/video_capture_memory.h (right): > > http://codereview.chromium.org/7002027/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/video_capture_memory.h:81: typedef > std::list<TransportDIB::Handle> DIBHANDLEList; > On 2011/05/19 04:24:37, wjia wrote: > > typedef precedes where it's used. > > Done.
Aaron, Could you help to review this patch? Thanks, Wei
http://codereview.chromium.org/7002027/diff/29001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/7002027/diff/29001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:1926: '../content/browser/media_stream/video_capture_manager_unittest.cc', not seeing this file? http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_controller.cc (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:14: static const unsigned int kNoOfDIBS = 3; size_t ? http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:24: // Delete all TransportDIBs comments end w/ periods http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:53: // Check if we own this handle indent http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:57: lock_.Acquire(); nit: use AutoLock with a scope { AutoLock l(lock_); // do stuff } http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:69: void VideoCaptureController::OnIncomingCapturedFrame(const uint8* data, which thread is this executing on? we're doing colour conversion so don't want to clog up the wrong thread http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:75: lock_.Acquire(); nit: use AutoLock with a scope { AutoLock l(lock_); // do stuff } http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:88: uint8* target = static_cast<uint8*> (dib->memory()); no space between > and ( http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:89: CHECK(dib->size() >= (unsigned int) (frame_info_.width*frame_info_.height*3) / static_cast<> http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:102: uint8* uplane = target + frame_info_.width * frame_info_.height; nit: remove extra space after + http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:103: uint8* vplane = uplane + (frame_info_.width * frame_info_.height) / 4; nit: remove extra space after + http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:120: uint8* uplane = target + frame_info_.width * frame_info_.height; nit: remove extra space after + http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:121: uint8* vplane = uplane + (frame_info_.width * frame_info_.height) / 4; nit: remove extra space after + http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:134: uint8* uplane = target + frame_info_.width * frame_info_.height; nit: remove extra space after + http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:135: uint8* vplane = uplane + (frame_info_.width * frame_info_.height) / 4; nit: remove extra space after + http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:163: lock_.Acquire(); nit: use AutoLock with a scope { AutoLock l(lock_); // do stuff } http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:171: if (owned_dibs_.size() != kNoOfDIBS) { warning: accessing owned_dibs_ outside of lock http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:184: lock_.Acquire(); nit: use AutoLock with a scope { AutoLock l(lock_); // do stuff } http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:192: if (stopped_task) { who would call OnDeviceStopped() w/o a task? it looks like only callee is inside this class so perhaps this should be required http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_controller.h (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:6: // VideoCaptureManager and VideoCaptrurDevice. s/VideoCaptrurDevice/VideoCaptureDevice/ http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:32: // Id used for identifying an object of VideoCaptureController comments end w/ periods http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:38: // An TransportDIB have been filled with I420 video. space these methods out http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:55: VideoCaptureController::EventHandler* event_handler); nit: you don't need to prefix w/ VideoCaptureController:: http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:56: ~VideoCaptureController(); virtual http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:64: // Stop video capture. add a blank line to space these before comments http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:73: // Implement media::VideoCaptureDevice::EventHandler comments end w/ periods http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:84: // Lock to protect free_dibs_. what about owned_dibs_? it looks like you lock in order to modify that structure as well http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:87: typedef std::list<TransportDIB::Handle> DIBHANDLEList; DIBHANDLEList -> DIBHandleList http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:89: // Free DIBS that can be filled with video frames. please add a blank line before these comments to make it a bit more readable http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:93: EventHandler& event_handler_; no ref types -- use pointer please http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:99: }; DISALLOW_IMPLICIT_DESTRUCTORS http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host.cc (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:13: VideoCaptureHost::~VideoCaptureHost() { nit: collapse onto one line {} http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:117: IPC_MESSAGE_HANDLER(VideoCaptureHostMsg_Start, OnStartCapture) indent the HANDLER functions by 2 spaces http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:134: DCHECK(entries_.find(controller_id) == entries_.end()); DCHECK_EQ http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:152: // It does not exist so it must have been stopped already. indentation http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:186: entries_.erase(id); will this actually delete the VideoCaptureController? http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host_unittest.cc (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:50: void StartDump(int widht, int height) { s/widht/width http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:157: dumper_.StartDump(params.width, params.height); de-indent by 2 spaces http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:179: #ifndef TEST_REAL_CAPTURE_DEVICE nit: don't indent preprocessor statements http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:200: .Times(AnyNumber()); indentation http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:233: FROM_HERE, NewRunnableFunction(&PostQuitOnVideoCaptureManagerThread, I think you can replace this by calling message_loop_->RunAllPending(); http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:249: .Times(AnyNumber()) indentation (should be 4 spaces) http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:250: .WillOnce(ExitMessageLoop(message_loop_.get())); you usually don't need to do this kind of stuff and can call message_loop_->RunAllPending() instead (assuming everything executes on the single message_loop_ until work has completed) http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:275: .Times(AnyNumber()) indentation (should be 4 spaces) http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:276: .WillOnce(ExitMessageLoop(message_loop_.get())); ditto w/ RunAllPending() http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:294: .Times(AtLeast(1)); indentation (should be 4 spaces) http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:310: .Times(AnyNumber()) indentation (should be 4 spaces) http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:311: .WillOnce(ExitMessageLoop(message_loop_.get())) ditto w/ RunAllPending() http://codereview.chromium.org/7002027/diff/29001/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/7002027/diff/29001/content/content_browser.gyp... content/content_browser.gypi:168: 'browser/media_stream/media_stream_provider.h', not seeing these files? http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert.h File media/base/yuv_convert.h (right): http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert.h#ne... media/base/yuv_convert.h:80: void ConvertRGB24ToYUV(const uint8* rgbframe, unit test? http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert.h#ne... media/base/yuv_convert.h:90: void ConvertYUY2ToYUV(const uint8* src, unit test? http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert_c.cc File media/base/yuv_convert_c.cc (right): http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert_c.cc... media/base/yuv_convert_c.cc:52: uint8* yplane, indentation http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert_c.cc... media/base/yuv_convert_c.cc:65: pixel[0] * 25 + 128) >> 8) + 16); indent this line one more space http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert_c.cc... media/base/yuv_convert_c.cc:68: pixel[0] * 112 + 128) >> 8) + 128); indent this line one more space
Thanks. Here is the next round. Added unittests for color conversion. Regards Per http://codereview.chromium.org/7002027/diff/29001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/7002027/diff/29001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:1926: '../content/browser/media_stream/video_capture_manager_unittest.cc', On 2011/05/23 05:05:09, scherkus wrote: > not seeing this file? This patch is dependent on 6946001 where it will be included. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_controller.cc (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:14: static const unsigned int kNoOfDIBS = 3; On 2011/05/23 05:05:09, scherkus wrote: > size_t ? Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:24: // Delete all TransportDIBs On 2011/05/23 05:05:09, scherkus wrote: > comments end w/ periods Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:53: // Check if we own this handle On 2011/05/23 05:05:09, scherkus wrote: > indent Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:57: lock_.Acquire(); On 2011/05/23 05:05:09, scherkus wrote: > nit: use AutoLock with a scope > > { > AutoLock l(lock_); > // do stuff > } Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:69: void VideoCaptureController::OnIncomingCapturedFrame(const uint8* data, On 2011/05/23 05:05:09, scherkus wrote: > which thread is this executing on? we're doing colour conversion so don't want > to clog up the wrong thread This will be done by the thread running the capture device. Ie- direct show thread on windows and the v4l2_thread on Linux. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:75: lock_.Acquire(); On 2011/05/23 05:05:09, scherkus wrote: > nit: use AutoLock with a scope > > { > AutoLock l(lock_); > // do stuff > } Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:88: uint8* target = static_cast<uint8*> (dib->memory()); On 2011/05/23 05:05:09, scherkus wrote: > no space between > and ( Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:89: CHECK(dib->size() >= (unsigned int) (frame_info_.width*frame_info_.height*3) / On 2011/05/23 05:05:09, scherkus wrote: > static_cast<> Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:102: uint8* uplane = target + frame_info_.width * frame_info_.height; On 2011/05/23 05:05:09, scherkus wrote: > nit: remove extra space after + Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:103: uint8* vplane = uplane + (frame_info_.width * frame_info_.height) / 4; On 2011/05/23 05:05:09, scherkus wrote: > nit: remove extra space after + Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:120: uint8* uplane = target + frame_info_.width * frame_info_.height; On 2011/05/23 05:05:09, scherkus wrote: > nit: remove extra space after + Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:121: uint8* vplane = uplane + (frame_info_.width * frame_info_.height) / 4; On 2011/05/23 05:05:09, scherkus wrote: > nit: remove extra space after + Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:134: uint8* uplane = target + frame_info_.width * frame_info_.height; On 2011/05/23 05:05:09, scherkus wrote: > nit: remove extra space after + Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:135: uint8* vplane = uplane + (frame_info_.width * frame_info_.height) / 4; On 2011/05/23 05:05:09, scherkus wrote: > nit: remove extra space after + Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:163: lock_.Acquire(); On 2011/05/23 05:05:09, scherkus wrote: > nit: use AutoLock with a scope > > { > AutoLock l(lock_); > // do stuff > } Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:171: if (owned_dibs_.size() != kNoOfDIBS) { On 2011/05/23 05:05:09, scherkus wrote: > warning: accessing owned_dibs_ outside of lock Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:184: lock_.Acquire(); On 2011/05/23 05:05:09, scherkus wrote: > nit: use AutoLock with a scope > > { > AutoLock l(lock_); > // do stuff > } Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:192: if (stopped_task) { On 2011/05/23 05:05:09, scherkus wrote: > who would call OnDeviceStopped() w/o a task? > > it looks like only callee is inside this class so perhaps this should be > required VideoCaptureController::StopCapture called from VideoCaptureHost::OnStopCapture and void VideoCaptureHost::OnChannelClosing(). The reason I added the task is due to that VideoCaptureController needs to be deleted before VideoCaptureHost. Normally VideoCaptureHost::OnStopCapture is called via IPC and then we are not interested in when the capture device have been stopped. We are only interested in when the capturedevice have been stopped and all Transport DIBs have been returned and therefore controller->StopCapture(NULL) is called. Should I rather create a Dummy task? http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_controller.h (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:6: // VideoCaptureManager and VideoCaptrurDevice. On 2011/05/23 05:05:09, scherkus wrote: > s/VideoCaptrurDevice/VideoCaptureDevice/ Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:32: // Id used for identifying an object of VideoCaptureController On 2011/05/23 05:05:09, scherkus wrote: > comments end w/ periods Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:38: // An TransportDIB have been filled with I420 video. On 2011/05/23 05:05:09, scherkus wrote: > space these methods out Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:55: VideoCaptureController::EventHandler* event_handler); On 2011/05/23 05:05:09, scherkus wrote: > nit: you don't need to prefix w/ VideoCaptureController:: Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:56: ~VideoCaptureController(); On 2011/05/23 05:05:09, scherkus wrote: > virtual Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:64: // Stop video capture. On 2011/05/23 05:05:09, scherkus wrote: > add a blank line to space these before comments Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:73: // Implement media::VideoCaptureDevice::EventHandler On 2011/05/23 05:05:09, scherkus wrote: > comments end w/ periods Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:84: // Lock to protect free_dibs_. On 2011/05/23 05:05:09, scherkus wrote: > what about owned_dibs_? > > it looks like you lock in order to modify that structure as well Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:87: typedef std::list<TransportDIB::Handle> DIBHANDLEList; On 2011/05/23 05:05:09, scherkus wrote: > DIBHANDLEList -> DIBHandleList Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:89: // Free DIBS that can be filled with video frames. On 2011/05/23 05:05:09, scherkus wrote: > please add a blank line before these comments to make it a bit more readable Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:93: EventHandler& event_handler_; On 2011/05/23 05:05:09, scherkus wrote: > no ref types -- use pointer please Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.h:99: }; On 2011/05/23 05:05:09, scherkus wrote: > DISALLOW_IMPLICIT_DESTRUCTORS DISALLOW_IMPLICIT_CONSTRUCTORS? http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host.cc (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:13: VideoCaptureHost::~VideoCaptureHost() { On 2011/05/23 05:05:09, scherkus wrote: > nit: collapse onto one line {} Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:117: IPC_MESSAGE_HANDLER(VideoCaptureHostMsg_Start, OnStartCapture) On 2011/05/23 05:05:09, scherkus wrote: > indent the HANDLER functions by 2 spaces Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:134: DCHECK(entries_.find(controller_id) == entries_.end()); On 2011/05/23 05:05:09, scherkus wrote: > DCHECK_EQ Can I really do that with complex types? I get the following compile error when I change to DCHECK_EQ(entries_.find(controller_id), entries_.end());: In file included from ./base/memory/weak_ptr.h:56, from ./base/task.h:11, from ./content/browser/browser_thread.h:11, from ./content/browser/browser_message_filter.h:10, from ./content/browser/renderer_host/video_capture_host.h:42, from content/browser/renderer_host/video_capture_host.cc:5: ./base/logging.h: In function ‘std::string* logging::MakeCheckOpString(const t1&, const t2&, const char*) [with t1 = std::_Rb_tree_iterator<std::pair<const std::pair<int, int>, scoped_refptr<VideoCaptureController> > >, t2 = std::_Rb_tree_iterator<std::pair<const std::pair<int, int>, scoped_refptr<VideoCaptureController> > >]’: ./base/logging.h:512: instantiated from ‘std::string* logging::CheckEQImpl(const t1&, const t2&, const char*) [with t1 = std::_Rb_tree_iterator<std::pair<const std::pair<int, int>, scoped_refptr<VideoCaptureController> > >, t2 = std::_Rb_tree_iterator<std::pair<const std::pair<int, int>, scoped_refptr<VideoCaptureController> > >]’ content/browser/renderer_host/video_capture_host.cc:133: instantiated from here ./base/logging.h:465: error: no match for ‘operator<<’ in ‘std::operator<< [with _Traits = std::char_traits<char>](((std::basic_ostream<char, std::char_traits<char> >&)((std::basic_ostream<char, std::char_traits<char> >*)std::operator<< [with _Traits = std::char_traits<char>](((std::basic_ostream<char, std::char_traits<char> >&)(& ss.std::basic_ostringstream<char, std::char_traits<char>, std::allocator<char> >::<anonymous>)), names))), ((const char*)" (")) << v1’ /usr/include/c++/4.4/ostream:108: note: candidates are: std::basic_ostream<_CharT, _Traits>& std::basic_ostream<_CharT, _Traits>::operator<<(std::basic_ostream<_CharT, _Traits>& (*) http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:152: // It does not exist so it must have been stopped already. On 2011/05/23 05:05:09, scherkus wrote: > indentation Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:186: entries_.erase(id); On 2011/05/23 05:05:09, scherkus wrote: > will this actually delete the VideoCaptureController? Yes- since it is a ref_counted pointer. VideoCaptureController::~VideoCaptureController() at video_capture_controller.cc:26 0x3c044a3 base::RefCountedThreadSafe<VideoCaptureController, base::DefaultRefCountedThreadSafeTraits<VideoCaptureController> >::DeleteInternal() at ref_counted.h:149 0x3b6d6f2 base::DefaultRefCountedThreadSafeTraits<VideoCaptureController>::Destruct() at ref_counted.h:114 0x3b6d182 base::RefCountedThreadSafe<VideoCaptureController, base::DefaultRefCountedThreadSafeTraits<VideoCaptureController> >::Release() at ref_counted.h:143 0x3b6ca5c scoped_refptr<VideoCaptureController>::~scoped_refptr() at ref_counted.h:241 0x3b6c3a5 std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> >::~pair() at stl_pair.h:68 0x3b6bda2 __gnu_cxx::new_allocator<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > >::destroy() at new_allocator.h:115 0x3b6d68a std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> >, std::_Select1st<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > >, std::less<std::pair<int, int> >, std::allocator<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > > >::_M_destroy_node() at stl_tree.h:383 0x3b6ce3d std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> >, std::_Select1st<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > >, std::less<std::pair<int, int> >, std::allocator<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > > >::_M_erase() at stl_tree.h:972 0x3b6c561 std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> >, std::_Select1st<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > >, std::less<std::pair<int, int> >, std::allocator<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > > >::clear() at stl_tree.h:726 0x3b6d857 std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> >, std::_Select1st<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > >, std::less<std::pair<int, int> >, std::allocator<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > > >::erase() at stl_tree.h:1,385 0x3b6d57e std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> >, std::_Select1st<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > >, std::less<std::pair<int, int> >, std::allocator<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > > >::erase() at stl_tree.h:1,374 0x3b6cd56 std::map<std::pair<int, int>, scoped_refptr<VideoCaptureController>, std::less<std::pair<int, int> >, std::allocator<std::pair<std::pair<int, int> const, scoped_refptr<VideoCaptureController> > > >::erase() at stl_map.h:582 0x3b6c4d7 VideoCaptureHost::DoDeleteVideoCaptureController() at video_capture_host.cc:186 0x3b6b9e2 http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host_unittest.cc (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:50: void StartDump(int widht, int height) { On 2011/05/23 05:05:09, scherkus wrote: > s/widht/width Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:157: dumper_.StartDump(params.width, params.height); On 2011/05/23 05:05:09, scherkus wrote: > de-indent by 2 spaces Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:179: #ifndef TEST_REAL_CAPTURE_DEVICE On 2011/05/23 05:05:09, scherkus wrote: > nit: don't indent preprocessor statements Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:200: .Times(AnyNumber()); On 2011/05/23 05:05:09, scherkus wrote: > indentation Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:233: FROM_HERE, NewRunnableFunction(&PostQuitOnVideoCaptureManagerThread, On 2011/05/23 05:05:09, scherkus wrote: > I think you can replace this by calling message_loop_->RunAllPending(); Please see below. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:249: .Times(AnyNumber()) On 2011/05/23 05:05:09, scherkus wrote: > indentation (should be 4 spaces) Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:250: .WillOnce(ExitMessageLoop(message_loop_.get())); On 2011/05/23 05:05:09, scherkus wrote: > you usually don't need to do this kind of stuff and can call > message_loop_->RunAllPending() instead (assuming everything executes on the > single message_loop_ until work has completed) Video Capture uses 3 different threads : 1. Browser IO thread for IPC 2. VideoCaptureManager owns a thread for starting and stopping video_capture devices. It runs in its own thread because it can take a while. 3. The thread that do the actual capturing. DirectShow on windows and the v4l2_thread_ in VideoCaptureDeviceLinux.h on Linux. So in this test: host_->OnStartCapture is called on the IO thread. The implementation call VideoCaptureManager::OnStart on vc_device_thread_ in VideoCaptureManager.cc When a capture device is started the video frames are delivered to VideoCaptureController::OnIncomingCapturedFrame on the v4l2_thread (or DirectShow). The dib for the video frame is then posted back to the IO thread in void VideoCaptureHost::OnBufferReady to be sent on IPC. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:275: .Times(AnyNumber()) On 2011/05/23 05:05:09, scherkus wrote: > indentation (should be 4 spaces) Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:276: .WillOnce(ExitMessageLoop(message_loop_.get())); On 2011/05/23 05:05:09, scherkus wrote: > ditto w/ RunAllPending() please see comment above. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:294: .Times(AtLeast(1)); On 2011/05/23 05:05:09, scherkus wrote: > indentation (should be 4 spaces) Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:310: .Times(AnyNumber()) On 2011/05/23 05:05:09, scherkus wrote: > indentation (should be 4 spaces) Done. http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:311: .WillOnce(ExitMessageLoop(message_loop_.get())) On 2011/05/23 05:05:09, scherkus wrote: > ditto w/ RunAllPending() please see comment above. http://codereview.chromium.org/7002027/diff/29001/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/7002027/diff/29001/content/content_browser.gyp... content/content_browser.gypi:168: 'browser/media_stream/media_stream_provider.h', On 2011/05/23 05:05:09, scherkus wrote: > not seeing these files? This patch is dependent on 6946001 where it will be included. http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert.h File media/base/yuv_convert.h (right): http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert.h#ne... media/base/yuv_convert.h:80: void ConvertRGB24ToYUV(const uint8* rgbframe, On 2011/05/23 05:05:09, scherkus wrote: > unit test? Done. http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert.h#ne... media/base/yuv_convert.h:90: void ConvertYUY2ToYUV(const uint8* src, On 2011/05/23 05:05:09, scherkus wrote: > unit test? Done. http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert_c.cc File media/base/yuv_convert_c.cc (right): http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert_c.cc... media/base/yuv_convert_c.cc:52: uint8* yplane, On 2011/05/23 05:05:09, scherkus wrote: > indentation Done. http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert_c.cc... media/base/yuv_convert_c.cc:65: pixel[0] * 25 + 128) >> 8) + 16); On 2011/05/23 05:05:09, scherkus wrote: > indent this line one more space Done. http://codereview.chromium.org/7002027/diff/29001/media/base/yuv_convert_c.cc... media/base/yuv_convert_c.cc:68: pixel[0] * 112 + 128) >> 8) + 128); On 2011/05/23 05:05:09, scherkus wrote: > indent this line one more space Done.
almost there! not seeing the unittests.. did you forget to include them? thanks http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host.cc (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host.cc:134: DCHECK(entries_.find(controller_id) == entries_.end()); On 2011/05/23 12:05:47, Per K wrote: > On 2011/05/23 05:05:09, scherkus wrote: > > DCHECK_EQ > > Can I really do that with complex types? I get the following compile error when > I change to DCHECK_EQ(entries_.find(controller_id), entries_.end());: > In file included from ./base/memory/weak_ptr.h:56, > from ./base/task.h:11, > from ./content/browser/browser_thread.h:11, > from ./content/browser/browser_message_filter.h:10, > from ./content/browser/renderer_host/video_capture_host.h:42, > from content/browser/renderer_host/video_capture_host.cc:5: > ./base/logging.h: In function ‘std::string* logging::MakeCheckOpString(const > t1&, const t2&, const char*) [with t1 = std::_Rb_tree_iterator<std::pair<const > std::pair<int, int>, scoped_refptr<VideoCaptureController> > >, t2 = > std::_Rb_tree_iterator<std::pair<const std::pair<int, int>, > scoped_refptr<VideoCaptureController> > >]’: > ./base/logging.h:512: instantiated from ‘std::string* > logging::CheckEQImpl(const t1&, const t2&, const char*) [with t1 = > std::_Rb_tree_iterator<std::pair<const std::pair<int, int>, > scoped_refptr<VideoCaptureController> > >, t2 = > std::_Rb_tree_iterator<std::pair<const std::pair<int, int>, > scoped_refptr<VideoCaptureController> > >]’ > content/browser/renderer_host/video_capture_host.cc:133: instantiated from > here > ./base/logging.h:465: error: no match for ‘operator<<’ in ‘std::operator<< [with > _Traits = std::char_traits<char>](((std::basic_ostream<char, > std::char_traits<char> >&)((std::basic_ostream<char, std::char_traits<char> > >*)std::operator<< [with _Traits = > std::char_traits<char>](((std::basic_ostream<char, std::char_traits<char> >&)(& > ss.std::basic_ostringstream<char, std::char_traits<char>, std::allocator<char> > >::<anonymous>)), names))), ((const char*)" (")) << v1’ > /usr/include/c++/4.4/ostream:108: note: candidates are: > std::basic_ostream<_CharT, _Traits>& std::basic_ostream<_CharT, > _Traits>::operator<<(std::basic_ostream<_CharT, _Traits>& (*) ah I thought it was a basic type! my bad! http://codereview.chromium.org/7002027/diff/17003/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_controller.cc (right): http://codereview.chromium.org/7002027/diff/17003/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:99: frame_info_.height*3) / spaces around *
Updated VideoCaptureHost to work on Windows as well. Sorry about the late change. Unit test for yuv conversion is in http://codereview.chromium.org/7065021/. Regards Per http://codereview.chromium.org/7002027/diff/17003/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_controller.cc (right): http://codereview.chromium.org/7002027/diff/17003/content/browser/renderer_ho... content/browser/renderer_host/video_capture_controller.cc:99: frame_info_.height*3) / On 2011/05/23 21:19:17, scherkus wrote: > spaces around * Done.
LGTM thanks per!
+jam for content/browser OWNERS review
just have a comment about class design + threading... I've found it very helpful to have classes that are provided threads to run on since it leaves it up to the application/embedder (i.e,. chrome or a unit test) to decide whether multiple threads should be used. in general if you're using post task for synchronization (which I heartily recommend!) there is no difference to the code whether it's running on multiple threads or a single thread http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host_unittest.cc (right): http://codereview.chromium.org/7002027/diff/29001/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:250: .WillOnce(ExitMessageLoop(message_loop_.get())); On 2011/05/23 12:05:47, Per K wrote: > On 2011/05/23 05:05:09, scherkus wrote: > > you usually don't need to do this kind of stuff and can call > > message_loop_->RunAllPending() instead (assuming everything executes on the > > single message_loop_ until work has completed) > > Video Capture uses 3 different threads : > 1. Browser IO thread for IPC > 2. VideoCaptureManager owns a thread for starting and stopping video_capture > devices. It runs in its own thread because it can take a while. > 3. The thread that do the actual capturing. DirectShow on windows and the > v4l2_thread_ in VideoCaptureDeviceLinux.h on Linux. > > So in this test: > host_->OnStartCapture is called on the IO thread. > The implementation call > VideoCaptureManager::OnStart on vc_device_thread_ in VideoCaptureManager.cc > When a capture device is started the video frames are delivered to > VideoCaptureController::OnIncomingCapturedFrame > on the v4l2_thread (or DirectShow). > The dib for the video frame is then posted back to the IO thread in void > VideoCaptureHost::OnBufferReady to be sent on IPC. Hrmm... if you design your classes to accept message loops to run on you can inject a single message loop everywhere inside your unittests -- effectively making them single threaded and avoiding such code. For example if VideoCaptureManager is _given_ a thread, then in the unit test you wouldn't create a new thread where as in chrome it would be sensible to create a new thread.
http://codereview.chromium.org/7002027/diff/44014/content/browser/renderer_ho... File content/browser/renderer_host/video_capture_host_unittest.cc (right): http://codereview.chromium.org/7002027/diff/44014/content/browser/renderer_ho... content/browser/renderer_host/video_capture_host_unittest.cc:94: while (handle != 0) { FYI: during the process of landing your patch the try bots caught a platform specific bug... since the value of TransportDIB::Handle is platform-specific, you should use TransportDIB::DefaultHandleValue() (represents an invalid/default handle) as well as TransportDIB::is_valid_handle() to check for validity I've updated your patch locally and once it passes trybots will land it for you
Committed as r86927.
CRAP I jumped the gun! jam sorry for committing -- let me know if you spot any issues and Per & I will fix it up
On 2011/05/26 23:43:50, scherkus wrote: > CRAP I jumped the gun! > > jam sorry for committing -- let me know if you spot any issues and Per & I will > fix it up lgtm (only glanced at content files). sorry for delay, i missed this (but please don't --force a change again, as that negates the whole point of OWNERS file). note that there are a bunch of other owners for content\browser now as well :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
