Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8417046:
refactor video capture in browser process to support device sharing across multiple renderer proc... (Closed)
|
Created:
9 years, 1 month ago by wjia(left Chromium) Modified:
9 years, 1 month ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
Descriptionrefactor video capture in browser process to support device sharing across multiple renderer processes
This is a split of http://codereview.chromium.org/8304017/
1. VideoCaptureController is owned by VideoCaptureManager.
2. VideoCaptureHost obtains an instance of controller from manager.
3. The largest resolution is used for the device capturing. Clients should expect frame buffers with larger resolution that they requsted. All cropping/resampling is done by clients.
4. Frame buffers are shared between renderer processes and browser process. The buffer received by clients are shared buffers. Clients are recommended to copy pixel data into their local storage. A client can slow down capture frame rate if it returns buffers slowly.
5. This patch only supported the default device. More change in media stream is needed to support user selected device. That will be a different patch.
BUG=none
TEST=trybots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108533
Patch Set 1 #
Total comments: 32
Patch Set 2 : code review #
Total comments: 29
Patch Set 3 : rebase #Patch Set 4 : code review #Patch Set 5 : '' #
Total comments: 16
Patch Set 6 : code review #Messages
Total messages: 12 (0 generated)
This is a split of http://codereview.chromium.org/8304017/ to facilitate review with smaller patch. It's same as last patch set in http://codereview.chromium.org/8304017/.
http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_controller.cc (right): http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:35: // Handle to the render process that will receive the DIBs. nit: add blank lines before // comments http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:40: bool report_ready_to_delete; this is a pretty important field -- I'd encourage some documentation here http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:128: cit = FindClient(id, event_handler, pending_clients_); I'd strongly encourage you to have FindClient() return the client pointer itself (or NULL if not found) this is an extremely hard to read function and "cit" and "dit" don't make for very good names when you could have something like "client" you may have to do another look-up in order to erase the client (perhaps introduce a DeleteClient()?) from the map, but again we're talking about a very small list of clients so I'd pick code clarity over avoiding a second look-up http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:179: if (controller_clients_.size() == 0) { .empty() http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:204: if ((*cit)->report_ready_to_delete && (*cit)->buffers.size() == 0) { .empty() http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:221: // some clients call StopCapture before returning all buffers. is it valid to call StopCapture() before returning all buffers? can you require all buffers to be returned before calling StopCapture()? it might help simplify the implementation http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:251: if (!buffer_exist) { nit: you could do without buffer_exist by checking dib instead if (!dib) { return; } http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:283: int rgb_stride = - 3 * frame_info_.width; nit: - 3 -> -3 also the side of this #if could be reduced as the uint8* lines of code are duplicated http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:302: uint8* uplane = target + frame_info_.width * frame_info_.height; nit: remove extra spaces here http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:427: for (DIBMap::iterator dit = owned_dibs_.begin(); do you need lock_ here for accessing owned_dibs_? if this function is expected to be called while holding the lock (as I think it's supposed to be), I'd recommend making it stand out a little bit more instead of "WithLock" try "_Locked" you may also want to put a lock_.AssertAcquired() at the top of the function http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:455: free_dibs_.clear(); do you need lock_ here? http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:459: if (controller_clients_.size() + pending_clients_.size() == 0) { .empty() http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_controller.h (right): http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.h:124: ClientSideDIBCount client_side_dib_count_; I find the buffer_id system awfully confusing why go through the trouble with multiple lists/maps and buffer_ids when kNoOfDIBS is 3? I doubt you gaining any sort of performance advantage using a map when you only have three elements. what about the following: struct SharedDIB { base::SharedMemory* shared_memory; int references; }; SharedDIB shared_dibs[kNoOfDIBS]; Then inside of functions like OnIncomingCapturedFrame() you'd have much simpler code, such as: for (int i = 0; i < kNoOfDIBS; ++i) { if (shared_dibs[i].references == 0) { // found an available DIB } } It'd also do away with some of the hard to read iterator/map-based code, such as the following: if (--dit->second > 0) continue; http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:249: if (cit->second->handlers.size() == 0) { .empty() http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:407: ctrller = new VideoCaptureController(this); try to avoid abbreviations -- can we use |controller| insteadl?
On 2011/10/31 03:20:18, scherkus wrote: > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:35: // Handle to > the render process that will receive the DIBs. > nit: add blank lines before // comments > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:40: bool > report_ready_to_delete; > this is a pretty important field -- I'd encourage some documentation here > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:128: cit = > FindClient(id, event_handler, pending_clients_); > I'd strongly encourage you to have FindClient() return the client pointer itself > (or NULL if not found) > > this is an extremely hard to read function and "cit" and "dit" don't make for > very good names when you could have something like "client" > > you may have to do another look-up in order to erase the client (perhaps > introduce a DeleteClient()?) from the map, but again we're talking about a very > small list of clients so I'd pick code clarity over avoiding a second look-up > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:179: if > (controller_clients_.size() == 0) { > .empty() > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:204: if > ((*cit)->report_ready_to_delete && (*cit)->buffers.size() == 0) { > .empty() > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:221: // some > clients call StopCapture before returning all buffers. > is it valid to call StopCapture() before returning all buffers? > > can you require all buffers to be returned before calling StopCapture()? it > might help simplify the implementation > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:251: if > (!buffer_exist) { > nit: you could do without buffer_exist by checking dib instead > > if (!dib) { > return; > } > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:283: int > rgb_stride = - 3 * frame_info_.width; > nit: - 3 -> -3 > > also the side of this #if could be reduced as the uint8* lines of code are > duplicated > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:302: uint8* > uplane = target + frame_info_.width * frame_info_.height; > nit: remove extra spaces here > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:427: for > (DIBMap::iterator dit = owned_dibs_.begin(); > do you need lock_ here for accessing owned_dibs_? > > if this function is expected to be called while holding the lock (as I think > it's supposed to be), I'd recommend making it stand out a little bit more > > instead of "WithLock" try "_Locked" > > you may also want to put a lock_.AssertAcquired() at the top of the function > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:455: > free_dibs_.clear(); > do you need lock_ here? > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.cc:459: if > (controller_clients_.size() + pending_clients_.size() == 0) { > .empty() > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > File content/browser/renderer_host/media/video_capture_controller.h (right): > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_controller.h:124: > ClientSideDIBCount client_side_dib_count_; > I find the buffer_id system awfully confusing > > why go through the trouble with multiple lists/maps and buffer_ids when > kNoOfDIBS is 3? I doubt you gaining any sort of performance advantage using a > map when you only have three elements. > > what about the following: > struct SharedDIB { > base::SharedMemory* shared_memory; > int references; > }; > SharedDIB shared_dibs[kNoOfDIBS]; > > Then inside of functions like OnIncomingCapturedFrame() you'd have much simpler > code, such as: > > for (int i = 0; i < kNoOfDIBS; ++i) { > if (shared_dibs[i].references == 0) { > // found an available DIB > } > } > > It'd also do away with some of the hard to read iterator/map-based code, such as > the following: > if (--dit->second > 0) > continue; > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > File content/browser/renderer_host/media/video_capture_manager.cc (right): > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_manager.cc:249: if > (cit->second->handlers.size() == 0) { > .empty() > > http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... > content/browser/renderer_host/media/video_capture_manager.cc:407: ctrller = new > VideoCaptureController(this); > try to avoid abbreviations -- can we use |controller| insteadl? I will wait with next review until you have addressed sherkus comments about the cit/dit etc.
scherkus is on it, so removing myself from reviewers list. Let me know if you want me to look at this anyway.
Thanks, Andrew! Patch has been updated. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_controller.cc (right): http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:35: // Handle to the render process that will receive the DIBs. On 2011/10/31 03:20:18, scherkus wrote: > nit: add blank lines before // comments Done. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:40: bool report_ready_to_delete; On 2011/10/31 03:20:18, scherkus wrote: > this is a pretty important field -- I'd encourage some documentation here Done. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:128: cit = FindClient(id, event_handler, pending_clients_); On 2011/10/31 03:20:18, scherkus wrote: > I'd strongly encourage you to have FindClient() return the client pointer itself > (or NULL if not found) > > this is an extremely hard to read function and "cit" and "dit" don't make for > very good names when you could have something like "client" > > you may have to do another look-up in order to erase the client (perhaps > introduce a DeleteClient()?) from the map, but again we're talking about a very > small list of clients so I'd pick code clarity over avoiding a second look-up Done. std::list has remove(). So no need to have another function to remove element. cit and dit have been renamed to client_it and dib_it, respectively. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:179: if (controller_clients_.size() == 0) { On 2011/10/31 03:20:18, scherkus wrote: > .empty() Done. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:204: if ((*cit)->report_ready_to_delete && (*cit)->buffers.size() == 0) { On 2011/10/31 03:20:18, scherkus wrote: > .empty() Done. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:221: // some clients call StopCapture before returning all buffers. On 2011/10/31 03:20:18, scherkus wrote: > is it valid to call StopCapture() before returning all buffers? > > can you require all buffers to be returned before calling StopCapture()? it > might help simplify the implementation It's possible that renderer process side sends StopCapture message before all buffers are returned because some buffers are sent to renderer process before Host receives StopCapture message, but after renderer process sends StopCapture. Currently, host doesn't track buffers. It just relay the message to controller. So it's valid controller gets StopCapture() before that client (i.e., host) returns all buffers. We can require host to call StopCapture only after returning all buffers by moving some logic into VCHost. My take is that the total complexity will be pretty much same. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:251: if (!buffer_exist) { On 2011/10/31 03:20:18, scherkus wrote: > nit: you could do without buffer_exist by checking dib instead > > if (!dib) { > return; > } Done. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:283: int rgb_stride = - 3 * frame_info_.width; On 2011/10/31 03:20:18, scherkus wrote: > nit: - 3 -> -3 > > also the side of this #if could be reduced as the uint8* lines of code are > duplicated Done and more cleanup. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:302: uint8* uplane = target + frame_info_.width * frame_info_.height; On 2011/10/31 03:20:18, scherkus wrote: > nit: remove extra spaces here Done. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:427: for (DIBMap::iterator dit = owned_dibs_.begin(); On 2011/10/31 03:20:18, scherkus wrote: > do you need lock_ here for accessing owned_dibs_? > > if this function is expected to be called while holding the lock (as I think > it's supposed to be), I'd recommend making it stand out a little bit more > > instead of "WithLock" try "_Locked" > > you may also want to put a lock_.AssertAcquired() at the top of the function lock_ is not needed here. changed correspondingly. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:455: free_dibs_.clear(); On 2011/10/31 03:20:18, scherkus wrote: > do you need lock_ here? lock_ is not needed here. At this point, the device has been fully stopped and it will not send any buffer to controller, which means free_dibs_ will not access on device thread. Furthermore, all buffers have been returned to controller. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.cc:459: if (controller_clients_.size() + pending_clients_.size() == 0) { On 2011/10/31 03:20:18, scherkus wrote: > .empty() Done. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_controller.h (right): http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.h:124: ClientSideDIBCount client_side_dib_count_; On 2011/10/31 03:20:18, scherkus wrote: > I find the buffer_id system awfully confusing > > why go through the trouble with multiple lists/maps and buffer_ids when > kNoOfDIBS is 3? I doubt you gaining any sort of performance advantage using a > map when you only have three elements. > > what about the following: > struct SharedDIB { > base::SharedMemory* shared_memory; > int references; > }; > SharedDIB shared_dibs[kNoOfDIBS]; > > Then inside of functions like OnIncomingCapturedFrame() you'd have much simpler > code, such as: > > for (int i = 0; i < kNoOfDIBS; ++i) { > if (shared_dibs[i].references == 0) { > // found an available DIB > } > } > > It'd also do away with some of the hard to read iterator/map-based code, such as > the following: > if (--dit->second > 0) > continue; Added SharedDIB and removed ClientSideDIBCount. Using map for owned_dibs_ allows controller to use any numbers for buffer_id, instead of just 0, 1, 2. Let me know if code clarity/simplicity is more important than flexibility here. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:249: if (cit->second->handlers.size() == 0) { On 2011/10/31 03:20:18, scherkus wrote: > .empty() Done. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_manager.cc:407: ctrller = new VideoCaptureController(this); On 2011/10/31 03:20:18, scherkus wrote: > try to avoid abbreviations -- can we use |controller| insteadl? Done.
http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_controller.h (right): http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.h:124: ClientSideDIBCount client_side_dib_count_; On 2011/10/31 22:02:44, wjia wrote: > On 2011/10/31 03:20:18, scherkus wrote: > > I find the buffer_id system awfully confusing > > > > why go through the trouble with multiple lists/maps and buffer_ids when > > kNoOfDIBS is 3? I doubt you gaining any sort of performance advantage using a > > map when you only have three elements. > > > > what about the following: > > struct SharedDIB { > > base::SharedMemory* shared_memory; > > int references; > > }; > > SharedDIB shared_dibs[kNoOfDIBS]; > > > > Then inside of functions like OnIncomingCapturedFrame() you'd have much > simpler > > code, such as: > > > > for (int i = 0; i < kNoOfDIBS; ++i) { > > if (shared_dibs[i].references == 0) { > > // found an available DIB > > } > > } > > > > It'd also do away with some of the hard to read iterator/map-based code, such > as > > the following: > > if (--dit->second > 0) > > continue; > Added SharedDIB and removed ClientSideDIBCount. > > Using map for owned_dibs_ allows controller to use any numbers for buffer_id, > instead of just 0, 1, 2. > > Let me know if code clarity/simplicity is more important than flexibility here. I guess I don't fully understand. What is the flexibility giving you? Do you plan on switching to opaque handles or the pointer value of SharedDIB? It's your call -- using a map makes some things easier (i.e., bounds checking is replaced by map lookup), but it's also a bit more verbose. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_controller.cc (right): http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:57: int references; documentation for these fields http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:153: DCHECK(client); what about making the comment into a string? DCHECK(client) << "Client should have called StartCapture()"; http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:168: if (--dib_it->second->references > 0) from a quick glance this isn't very clear -- what about: dib_it->second->references -= 1; if (dib_it->second->references > 0) // ... code ... http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:174: free_dibs_.push_back(buffer_id); if you didn't have free_dibs_ this code would go away http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:208: controller_clients_); indentation http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:223: if (--dib_it->second->references > 0) ditto http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:229: free_dibs_.push_back(buffer_id); if you didn't have free_dibs_ this code would go away http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:251: if (!free_dibs_.empty()) { using free_dibs_ isn't really any more efficient but it does look like it comes in handy for when the device thread is executed faster than dibs are consumed by clients how about: for (each element in owned_dibs) { if (owned_dibs_[i]->references == 0) { buffer_id = i; dib = owned_dibs[i]; dib->references = 1; // reference is added so buffer can be passed to DoIncomingCapturedFrameOnIOThread() break; } } http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:337: if (state_ != media::VideoCapture::kStarted) { without free_dibs_ you could rewrite this whole function like so: int count = 0; if (state_ == kStarted) { // iterate over clients and increment count } base::AutoLock lock(lock_); DCHECK(buffer has reference of 1); owned_dibs_[buffer_id]->references = count; This would handle all cases and requires less bookkeeping with free_dibs_ http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:393: free_dibs_.push_back(i); if you didn't have free_dibs_ this code would go away http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:449: free_dibs_.clear(); if you didn't have free_dibs_ this code would go away http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_controller.h (right): http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.h:81: struct SharedDIB; nit: move this down to where SharedDIB is used http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.h:114: DIBHandleList free_dibs_; why do you still need free_dibs_? if I follow the code correctly it looks like it comes in handy during VideoCaptureController::OnIncomingCapturedFrame() but I think it could be simplified I've documented places in the .cc where you shouldn't need free_dibs_ -- would like to hear your thoughts. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:9: #include "content/public/browser/browser_thread.h" a->z ordering http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:403: scoped_refptr<VideoCaptureController> controller = NULL; nit: scoped_refptr doesn't need to be initialized to NULL
Thanks, Andrew! Patch has been updated. Patch Set 3 is just rebase. Patch Set 4 is the real change. http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/video_capture_controller.h (right): http://codereview.chromium.org/8417046/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/video_capture_controller.h:124: ClientSideDIBCount client_side_dib_count_; On 2011/11/01 02:18:38, scherkus wrote: > On 2011/10/31 22:02:44, wjia wrote: > > On 2011/10/31 03:20:18, scherkus wrote: > > > I find the buffer_id system awfully confusing > > > > > > why go through the trouble with multiple lists/maps and buffer_ids when > > > kNoOfDIBS is 3? I doubt you gaining any sort of performance advantage using > a > > > map when you only have three elements. > > > > > > what about the following: > > > struct SharedDIB { > > > base::SharedMemory* shared_memory; > > > int references; > > > }; > > > SharedDIB shared_dibs[kNoOfDIBS]; > > > > > > Then inside of functions like OnIncomingCapturedFrame() you'd have much > > simpler > > > code, such as: > > > > > > for (int i = 0; i < kNoOfDIBS; ++i) { > > > if (shared_dibs[i].references == 0) { > > > // found an available DIB > > > } > > > } > > > > > > It'd also do away with some of the hard to read iterator/map-based code, > such > > as > > > the following: > > > if (--dit->second > 0) > > > continue; > > Added SharedDIB and removed ClientSideDIBCount. > > > > Using map for owned_dibs_ allows controller to use any numbers for buffer_id, > > instead of just 0, 1, 2. > > > > Let me know if code clarity/simplicity is more important than flexibility > here. > > I guess I don't fully understand. What is the flexibility giving you? Do you > plan on switching to opaque handles or the pointer value of SharedDIB? > > It's your call -- using a map makes some things easier (i.e., bounds checking is > replaced by map lookup), but it's also a bit more verbose. By "flexibility", I mean we can improve the capture later. For example, current design doesn't allow controller to restart device till it has received all shared buffers and the device is fully stopped. This can be improved in such a way that controller doesn't have to wait for all shared buffers returned. As long as device has been fully stopped, controller can restart capturing with another series of buffer_id's. The new buffer_id's don't overlap with previous ones. This new design can speed up resolution change by allowing buffer return and new capturing in parallel. Another advantage of using map is that buffer_id can be other values, maybe scrambled ones. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_controller.cc (right): http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:57: int references; On 2011/11/01 02:18:38, scherkus wrote: > documentation for these fields Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:153: DCHECK(client); On 2011/11/01 02:18:38, scherkus wrote: > what about making the comment into a string? > > DCHECK(client) << "Client should have called StartCapture()"; Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:168: if (--dib_it->second->references > 0) On 2011/11/01 02:18:38, scherkus wrote: > from a quick glance this isn't very clear -- what about: > dib_it->second->references -= 1; > if (dib_it->second->references > 0) > // ... code ... Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:174: free_dibs_.push_back(buffer_id); On 2011/11/01 02:18:38, scherkus wrote: > if you didn't have free_dibs_ this code would go away Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:208: controller_clients_); On 2011/11/01 02:18:38, scherkus wrote: > indentation Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:223: if (--dib_it->second->references > 0) On 2011/11/01 02:18:38, scherkus wrote: > ditto Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:229: free_dibs_.push_back(buffer_id); On 2011/11/01 02:18:38, scherkus wrote: > if you didn't have free_dibs_ this code would go away Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:251: if (!free_dibs_.empty()) { On 2011/11/01 02:18:38, scherkus wrote: > using free_dibs_ isn't really any more efficient but it does look like it comes > in handy for when the device thread is executed faster than dibs are consumed by > clients > > how about: > for (each element in owned_dibs) { > if (owned_dibs_[i]->references == 0) { > buffer_id = i; > dib = owned_dibs[i]; > dib->references = 1; // reference is added so buffer can be passed to > DoIncomingCapturedFrameOnIOThread() > break; > } > } removed free_dibss_. I have to use a special value "-1" here. Otherwise, it will be treated as buffer at renderer side. And lock on all occurrence of "references". http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:337: if (state_ != media::VideoCapture::kStarted) { On 2011/11/01 02:18:38, scherkus wrote: > without free_dibs_ you could rewrite this whole function like so: > > int count = 0; > if (state_ == kStarted) { > // iterate over clients and increment count > } > > base::AutoLock lock(lock_); > DCHECK(buffer has reference of 1); > owned_dibs_[buffer_id]->references = count; > > > > This would handle all cases and requires less bookkeeping with free_dibs_ Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:393: free_dibs_.push_back(i); On 2011/11/01 02:18:38, scherkus wrote: > if you didn't have free_dibs_ this code would go away Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.cc:449: free_dibs_.clear(); On 2011/11/01 02:18:38, scherkus wrote: > if you didn't have free_dibs_ this code would go away Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_controller.h (right): http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_controller.h:81: struct SharedDIB; On 2011/11/01 02:18:38, scherkus wrote: > nit: move this down to where SharedDIB is used Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:9: #include "content/public/browser/browser_thread.h" On 2011/11/01 02:18:38, scherkus wrote: > a->z ordering Done. http://codereview.chromium.org/8417046/diff/10001/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:403: scoped_refptr<VideoCaptureController> controller = NULL; On 2011/11/01 02:18:38, scherkus wrote: > nit: scoped_refptr doesn't need to be initialized to NULL Done.
Hi- A suggestion to refactor this a bit to simplify it. You have moved some of the VideoCaptrueCOntroller responsibilities from VIdeoCaptureHost to VideoCaptureManager but not all. I suggest you move the rest as well. That way you should be able to remove some redundant code. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_controller.h (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.h:99: ControllerClients& clients); const ControllerClients& - not allowed to have none const ref. Does this have to be in the class since you have all inputs as arguments. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_controller_event_handler.h (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller_event_handler.h:47: virtual void OnReadyToDelete(const VideoCaptureControllerID& id) = 0; Should you put OnReadyToDelete in a separate interface or use an event if you make sure VideoCaptureManager creates VideoCaptureControllers? VideoCaptureHost should not have to care about the lifetime of a controller if VideoCaptureManager creates and owns it. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_manager.cc:221: Controllers::iterator cit = controllers_.find(video_capture_device); You can skip this lookup by changing media::VideoCaptureDevice::EventHandler* video_capture_receiver to http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_manager.h (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_manager.h:77: const media::VideoCaptureParams& capture_params, Why is capture_params needed both here and in void Start? Should only need them once. Even better: Remove AddController and RemoveController. They can be combined with Start and Stop above. VideoCaptureHost call Start(capture_params, handler) VideoCapureManager::Start looks up the handler and creates a new controller if needed. The controller implements media::VideoCaptureDevice::EventHander so there is no longer any need for the VideoCaptureHost to pass it in since VideoCaptureManager now owns / creates it. This way you should be able to simplify things a bit.
http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_controller.cc (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.cc:173: DCHECK_GT(dib_it->second->references, 0) << "The buffer is not used by " nit: drop the << to 4-space indent on next line http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.cc:226: DCHECK_GT(dib_it->second->references, 0) << "The buffer is not used by " ditto http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.cc:258: dib_it->second->references = -1; does setting it as 1 really cause things to go wrong? if not, then keeping reference count non-negative would certainly be clearer but if so could we make this a constant? I also wonder whether it makes sense to make SharedDIB a class as opposed to a struct. For example you could have AddRef() and Release() methods, DCHECKs, etc... Your call. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.cc:377: DCHECK(owned_dibs_.empty()) << "Device is restarted without releasing shared " ditto
http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_controller.cc (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.cc:173: DCHECK_GT(dib_it->second->references, 0) << "The buffer is not used by " On 2011/11/02 23:08:26, scherkus wrote: > nit: drop the << to 4-space indent on next line Done. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.cc:226: DCHECK_GT(dib_it->second->references, 0) << "The buffer is not used by " On 2011/11/02 23:08:26, scherkus wrote: > ditto Done. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.cc:258: dib_it->second->references = -1; On 2011/11/02 23:08:26, scherkus wrote: > does setting it as 1 really cause things to go wrong? > > if not, then keeping reference count non-negative would certainly be clearer but > if so could we make this a constant? > > I also wonder whether it makes sense to make SharedDIB a class as opposed to a > struct. For example you could have AddRef() and Release() methods, DCHECKs, > etc... > > Your call. yes, setting it as 1 indeed confuses ClientHasDIB(). I will keep an eye on SharedDIB for future improvement. Right now, controller has exclusive ownership of this struct. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.cc:377: DCHECK(owned_dibs_.empty()) << "Device is restarted without releasing shared " On 2011/11/02 23:08:26, scherkus wrote: > ditto Done. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_controller.h (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller.h:99: ControllerClients& clients); On 2011/11/02 09:18:13, perkj wrote: > const ControllerClients& - not allowed to have none const ref. > Does this have to be in the class since you have all inputs as arguments. Added "const". Those classes/structs are private to VCController. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_controller_event_handler.h (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_controller_event_handler.h:47: virtual void OnReadyToDelete(const VideoCaptureControllerID& id) = 0; On 2011/11/02 09:18:13, perkj wrote: > Should you put OnReadyToDelete in a separate interface or use an event if you > make sure VideoCaptureManager creates VideoCaptureControllers? VideoCaptureHost > should not have to care about the lifetime of a controller if > VideoCaptureManager creates and owns it. I didn't change the function name. It should be OnReadyToRemove or OnControllerDone. It's a signal telling Host that this controller has got all buffers back, and Host is free to remove it. This event is always needed. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_manager.cc (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_manager.cc:221: Controllers::iterator cit = controllers_.find(video_capture_device); On 2011/11/02 09:18:13, perkj wrote: > You can skip this lookup by changing media::VideoCaptureDevice::EventHandler* > video_capture_receiver > to The second parameter is for device to send event to. It doesn't have to be VideoCaptureController. In the VCManager_unittest, another subclass of media::VideoCaptureDevice::EventHandler is used. http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_manager.h (right): http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_manager.h:77: const media::VideoCaptureParams& capture_params, On 2011/11/02 09:18:13, perkj wrote: > Why is capture_params needed both here and in void Start? Should only need them > once. > > Even better: Remove AddController and RemoveController. They can be combined > with Start and Stop above. > VideoCaptureHost call Start(capture_params, handler) > VideoCapureManager::Start looks up the handler and creates a new controller if > needed. The controller implements media::VideoCaptureDevice::EventHander so > there is no longer any need for the VideoCaptureHost to pass it in since > VideoCaptureManager now owns / creates it. > > This way you should be able to simplify things a bit. AddController needs session_id in capture_params (I may just use session_id directly), while Start() needs it for capture configurations. I have thought about moving most of the logic into Manager. It indeed simplifies code a bit. But it makes more sense for Host to deal with Controller directly when it returns buffers (calling controller->ReturnBuffer()). Manager is mainly for open/close/start/stop of device, while Controller is for other operations.
Right , but you can let VideoCaptureManager:Start return the the VideoCaptureController instead of in VideoCaptureManager::AddController. On 2011/11/03 07:02:02, wjia wrote: > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > content/browser/renderer_host/media/video_capture_controller.cc:173: > DCHECK_GT(dib_it->second->references, 0) << "The buffer is not used by " > On 2011/11/02 23:08:26, scherkus wrote: > > nit: drop the << to 4-space indent on next line > > Done. > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > content/browser/renderer_host/media/video_capture_controller.cc:226: > DCHECK_GT(dib_it->second->references, 0) << "The buffer is not used by " > On 2011/11/02 23:08:26, scherkus wrote: > > ditto > > Done. > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > content/browser/renderer_host/media/video_capture_controller.cc:258: > dib_it->second->references = -1; > On 2011/11/02 23:08:26, scherkus wrote: > > does setting it as 1 really cause things to go wrong? > > > > if not, then keeping reference count non-negative would certainly be clearer > but > > if so could we make this a constant? > > > > I also wonder whether it makes sense to make SharedDIB a class as opposed to a > > struct. For example you could have AddRef() and Release() methods, DCHECKs, > > etc... > > > > Your call. > > yes, setting it as 1 indeed confuses ClientHasDIB(). > > I will keep an eye on SharedDIB for future improvement. Right now, controller > has exclusive ownership of this struct. > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > content/browser/renderer_host/media/video_capture_controller.cc:377: > DCHECK(owned_dibs_.empty()) << "Device is restarted without releasing shared " > On 2011/11/02 23:08:26, scherkus wrote: > > ditto > > Done. > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > File content/browser/renderer_host/media/video_capture_controller.h (right): > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > content/browser/renderer_host/media/video_capture_controller.h:99: > ControllerClients& clients); > On 2011/11/02 09:18:13, perkj wrote: > > const ControllerClients& - not allowed to have none const ref. > > Does this have to be in the class since you have all inputs as arguments. > > Added "const". Those classes/structs are private to VCController. > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > File > content/browser/renderer_host/media/video_capture_controller_event_handler.h > (right): > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > content/browser/renderer_host/media/video_capture_controller_event_handler.h:47: > virtual void OnReadyToDelete(const VideoCaptureControllerID& id) = 0; > On 2011/11/02 09:18:13, perkj wrote: > > Should you put OnReadyToDelete in a separate interface or use an event if you > > make sure VideoCaptureManager creates VideoCaptureControllers? > VideoCaptureHost > > should not have to care about the lifetime of a controller if > > VideoCaptureManager creates and owns it. > > I didn't change the function name. It should be OnReadyToRemove or > OnControllerDone. It's a signal telling Host that this controller has got all > buffers back, and Host is free to remove it. This event is always needed. > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > File content/browser/renderer_host/media/video_capture_manager.cc (right): > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > content/browser/renderer_host/media/video_capture_manager.cc:221: > Controllers::iterator cit = controllers_.find(video_capture_device); > On 2011/11/02 09:18:13, perkj wrote: > > You can skip this lookup by changing media::VideoCaptureDevice::EventHandler* > > video_capture_receiver > > to > > The second parameter is for device to send event to. It doesn't have to be > VideoCaptureController. In the VCManager_unittest, another subclass of > media::VideoCaptureDevice::EventHandler is used. > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > File content/browser/renderer_host/media/video_capture_manager.h (right): > > http://codereview.chromium.org/8417046/diff/7005/content/browser/renderer_hos... > content/browser/renderer_host/media/video_capture_manager.h:77: const > media::VideoCaptureParams& capture_params, > On 2011/11/02 09:18:13, perkj wrote: > > Why is capture_params needed both here and in void Start? Should only need > them > > once. > > > > Even better: Remove AddController and RemoveController. They can be combined > > with Start and Stop above. > > VideoCaptureHost call Start(capture_params, handler) > > VideoCapureManager::Start looks up the handler and creates a new controller if > > needed. The controller implements media::VideoCaptureDevice::EventHander so > > there is no longer any need for the VideoCaptureHost to pass it in since > > VideoCaptureManager now owns / creates it. > > > > This way you should be able to simplify things a bit. > > AddController needs session_id in capture_params (I may just use session_id > directly), while Start() needs it for capture configurations. > > I have thought about moving most of the logic into Manager. It indeed simplifies > code a bit. But it makes more sense for Host to deal with Controller directly > when it returns buffers (calling controller->ReturnBuffer()). Manager is mainly > for open/close/start/stop of device, while Controller is for other operations.
LGTM |
