Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
801363002:
Queue commands to the Os to start a video device. (Closed)
|
Created:
6 years ago by perkj_chrome Modified:
5 years, 11 months ago Reviewers:
tommi (sloooow) - chröme CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionQueue commands to the Os to start a video device.
Since it might take 1-2 seconds to start a device and the OS calls are blocking, a device should not be attempted to be started until the device thread is ready to process the request.
Instead, the requests are queued in VideoCaptureManager on the IO-thread.
BUG=428891
Committed: https://crrev.com/e770b238860b8907f054cecd8be9681922c2e3a0
Cr-Commit-Position: refs/heads/master@{#312081}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : Change to have DeviceEntry only touched on IO-thread. #
Total comments: 1
Patch Set 4 : Updated comments. #
Total comments: 38
Patch Set 5 : Addressed comments. #
Total comments: 2
Patch Set 6 : Fix potential race when calling VideoCapture::Start->Pause #
Total comments: 6
Patch Set 7 : Addressed more comments. #
Total comments: 2
Patch Set 8 : Changed to use DeviceEntry start_id as identifier. #Patch Set 9 : Fix lint + include algorithm. #Patch Set 10 : std::find_if #
Total comments: 8
Patch Set 11 : Adressed comments. #
Messages
Total messages: 17 (3 generated)
perkj@chromium.org changed reviewers: + tommi@chromium.org
Can you please review?
https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:222: CaptureDeviceStartRequest::CaptureDeviceStartRequest( no need to wrap? https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:259: delete entry; can we use a scoped_ptr<> for this instead? https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:267: if (delete_entry) { FYI - here there are two call sites + bindings for essentially the same callback. There's quite a bit of code involved for each of these call stacks, so you could do something like this instead: device_task_runner_->PostTask(... delete_entry ? base::Owned(entry) : base::Unretained(entry)); However, I wonder if we should be using scoped_ptr, scoped_refptr or shared_ptr here... passing around raw pointers seems fragile to me :-/ https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:193: DeviceEntry* entry, can you document the ownership/lifetime of |entry|? https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:273: DeviceEntry* device_entry; ownership/scope?
How about this? https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:222: CaptureDeviceStartRequest::CaptureDeviceStartRequest( On 2014/12/15 16:25:05, tommi wrote: > no need to wrap? media::VideoCaptureSessionId session_id, does not fit. https://codereview.chromium.org/801363002/diff/40001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/40001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:311: // PostTask failed. The Chrome browser process probably closing down so is
I think this is a good refactoring change. A few comments and questions here and there. https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/20001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:222: CaptureDeviceStartRequest::CaptureDeviceStartRequest( On 2014/12/16 20:15:05, perkj wrote: > On 2014/12/15 16:25:05, tommi wrote: > > no need to wrap? > > media::VideoCaptureSessionId session_id, does not fit. I meant the function: VideoCaptureManager::CaptureDeviceStartRequest::CaptureDeviceStartRequest( https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:221: CaptureDeviceStartRequest::CaptureDeviceStartRequest( 220-221 should fit on one line https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:237: if (device_start_queue_.size() == 1) { nit: no {} https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:247: if (device_start_queue_.size() > 1) { what's the reason for > 1 and not >0? (i.e. !empty()) Is there a chance that the entry might be in device_start_queue_ when size() == 1? https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:248: for (DeviceStartQueue::iterator request = ++device_start_queue_.begin(); it looks like you're incrementing begin() which can't be right. If you want to start at begin() + 1, then I'd prefer to use begin() + 1. If I'm understanding this correctly though, you want to go from element [1] to the end of the array and skip the first one. That assumes some sort of an order (i.e. there's something special about position [0]) which would be good to explain if that's intentional. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:251: device_start_queue_.erase(request); this doesn't delete any object, right? https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:253: << entry->id << "."; should we assert that entry exists in the main vector at the top of this function? (since if it doesn't, that could cause a crash). https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:265: base::Passed(entry->video_capture_device.Pass()))); shouldn't this be base::Passed(&entry->video_capture_device)? https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:271: if (request == device_start_queue_.end()) you should be able to replace this with if (device_start_queue.empty()) return; Under what circumstances do we get there then? https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:300: device_start_queue_.pop_front(); can we DCHECK that the device we're popping is the expected one? https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:308: FROM_HERE, indentation looks a bit weird. Would be good go move FROM_HERE to the end of the previous line or .. hmm.. what about something like this: if (device.get() && !device_task_runner_->PostTask(FROM_HERE, base::Bind(...)))) { https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:312: device->StopAndDeAllocate(); has base::Passed() not nulled |device| at this point? If it does, then deleting the closure should delete the device object and we'd need to call StopAndDeAllocate() before that time. You could do something like: auto* device_ptr = device.get(); auto closure = base::Bind(...); if (device_ptr && !device_task_runner_->PostTask(FROM_HERE, closure)) { device_ptr->StopAndDeallocate(); } https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:315: (*entry_it)->video_capture_device.reset(device.release()); nit: (*entry_it)->video_capture_device.swap(device); Should we DCHECK first that video_capture_device.get() == nullptr ? https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:263: typedef ScopedVector<DeviceEntry> DeviceEntries; nit: empty line before this one https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:274: DeviceEntry* entry; // Owned by |devices_| do we use this pointer? (If it's only used for lookup, then it would be good to add a note on that) https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:278: typedef std::list<CaptureDeviceStartRequest> DeviceStartQueue; nit: empty line above
PTAL https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:221: CaptureDeviceStartRequest::CaptureDeviceStartRequest( On 2014/12/16 22:31:25, tommi wrote: > 220-221 should fit on one line Done. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:237: if (device_start_queue_.size() == 1) { On 2014/12/16 22:31:25, tommi wrote: > nit: no {} Done. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:247: if (device_start_queue_.size() > 1) { On 2014/12/16 22:31:25, tommi wrote: > what's the reason for > 1 and not >0? (i.e. !empty()) > > Is there a chance that the entry might be in device_start_queue_ when size() == > 1? Added more comments. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:248: for (DeviceStartQueue::iterator request = ++device_start_queue_.begin(); On 2014/12/16 22:31:25, tommi wrote: > it looks like you're incrementing begin() which can't be right. If you want to > start at begin() + 1, then I'd prefer to use begin() + 1. > > If I'm understanding this correctly though, you want to go from element [1] to > the end of the array and skip the first one. That assumes some sort of an order > (i.e. there's something special about position [0]) which would be good to > explain if that's intentional. I have to do ++ and not begin() + 1: In file included from /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/list:67: In file included from /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/list:34: /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/safe_iterator.h:361:13: error: no viable overloaded '+=' https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:251: device_start_queue_.erase(request); On 2014/12/16 22:31:25, tommi wrote: > this doesn't delete any object, right? right. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:253: << entry->id << "."; On 2014/12/16 22:31:25, tommi wrote: > should we assert that entry exists in the main vector at the top of this > function? (since if it doesn't, that could cause a crash). Done. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:265: base::Passed(entry->video_capture_device.Pass()))); On 2014/12/16 22:31:25, tommi wrote: > shouldn't this be base::Passed(&entry->video_capture_device)? ../../content/browser/renderer_host/media/video_capture_manager.cc:266:33: error: calling a private constructor of class 'scoped_ptr<media::VideoCaptureDevice, base::DefaultDeleter<media::VideoCaptureDevice> >' base::Passed(entry->video_capture_device))); ^ ../../base/memory/scoped_ptr.h:312:51: note: declared private here MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(scoped_ptr) ^ ../../base/move.h:222:3: note: expanded from macro 'MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03' type(type&); \ From scoped_ptr.h // { // scoped_ptr<Foo> ptr(new Foo("yay")); // ptr manages Foo("yay"). // TakesOwnership(ptr.Pass()); // ptr no longer owns Foo("yay"). // scoped_ptr<Foo> ptr2 = CreateFoo(); // ptr2 owns the return Foo. // scoped_ptr<Foo> ptr3 = // ptr3 now owns what was in ptr2. // PassThru(ptr2.Pass()); // ptr2 is correspondingly nullptr. // } // // Notice that if you do not call Pass() when returning from PassThru(), or // when invoking TakesOwnership(), the code will not compile because scopers // are not copyable; https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:271: if (request == device_start_queue_.end()) On 2014/12/16 22:31:25, tommi wrote: > you should be able to replace this with > > if (device_start_queue.empty()) > return; > > Under what circumstances do we get there then? Rewrote. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:300: device_start_queue_.pop_front(); On 2014/12/16 22:31:25, tommi wrote: > can we DCHECK that the device we're popping is the expected one? Done. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:308: FROM_HERE, On 2014/12/16 22:31:25, tommi wrote: > indentation looks a bit weird. Would be good go move FROM_HERE to the end of > the previous line or .. hmm.. > what about something like this: > > if (device.get() && > !device_task_runner_->PostTask(FROM_HERE, > base::Bind(...)))) { Done. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:312: device->StopAndDeAllocate(); On 2014/12/16 22:31:25, tommi wrote: > has base::Passed() not nulled |device| at this point? If it does, then deleting > the closure should delete the device object and we'd need to call > StopAndDeAllocate() before that time. You could do something like: > > auto* device_ptr = device.get(); > auto closure = base::Bind(...); > > if (device_ptr && > !device_task_runner_->PostTask(FROM_HERE, closure)) { > device_ptr->StopAndDeallocate(); > } Done. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:315: (*entry_it)->video_capture_device.reset(device.release()); On 2014/12/16 22:31:25, tommi wrote: > nit: (*entry_it)->video_capture_device.swap(device); > > Should we DCHECK first that video_capture_device.get() == nullptr ? feels like overkill to me. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:263: typedef ScopedVector<DeviceEntry> DeviceEntries; On 2014/12/16 22:31:25, tommi wrote: > nit: empty line before this one Done. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:274: DeviceEntry* entry; // Owned by |devices_| On 2014/12/16 22:31:25, tommi wrote: > do we use this pointer? (If it's only used for lookup, then it would be good to > add a note on that) Well, its used in HandleQueuedStartRequest() but we do dcheck that it exists. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:278: typedef std::list<CaptureDeviceStartRequest> DeviceStartQueue; On 2014/12/16 22:31:25, tommi wrote: > nit: empty line above Done. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:278: typedef std::list<CaptureDeviceStartRequest> DeviceStartQueue; On 2014/12/16 22:31:25, tommi wrote: > nit: empty line above Done.
will take a look at the new changes in a bit. just clarifying some of my comments this time around. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:248: for (DeviceStartQueue::iterator request = ++device_start_queue_.begin(); On 2014/12/17 10:14:10, perkj wrote: > On 2014/12/16 22:31:25, tommi wrote: > > it looks like you're incrementing begin() which can't be right. If you want > to > > start at begin() + 1, then I'd prefer to use begin() + 1. > > > > If I'm understanding this correctly though, you want to go from element [1] to > > the end of the array and skip the first one. That assumes some sort of an > order > > (i.e. there's something special about position [0]) which would be good to > > explain if that's intentional. > > > I have to do ++ and not begin() + 1: > In file included from > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/list:67: > In file included from > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/list:34: > /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/safe_iterator.h:361:13: > error: no viable overloaded '+=' That error says "error: no viable overloaded '+='" are you sure you typed + 1? :) https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:265: base::Passed(entry->video_capture_device.Pass()))); On 2014/12/17 10:14:10, perkj wrote: > On 2014/12/16 22:31:25, tommi wrote: > > shouldn't this be base::Passed(&entry->video_capture_device)? > > > ../../content/browser/renderer_host/media/video_capture_manager.cc:266:33: > error: calling a private constructor of class > 'scoped_ptr<media::VideoCaptureDevice, > base::DefaultDeleter<media::VideoCaptureDevice> >' > base::Passed(entry->video_capture_device))); > ^ > ../../base/memory/scoped_ptr.h:312:51: note: declared private here > MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(scoped_ptr) > ^ > ../../base/move.h:222:3: note: expanded from macro > 'MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03' > type(type&); \ > > > From scoped_ptr.h > > // { > // scoped_ptr<Foo> ptr(new Foo("yay")); // ptr manages Foo("yay"). > // TakesOwnership(ptr.Pass()); // ptr no longer owns Foo("yay"). > // scoped_ptr<Foo> ptr2 = CreateFoo(); // ptr2 owns the return Foo. > // scoped_ptr<Foo> ptr3 = // ptr3 now owns what was in ptr2. > // PassThru(ptr2.Pass()); // ptr2 is correspondingly nullptr. > // } > // > // Notice that if you do not call Pass() when returning from PassThru(), or > // when invoking TakesOwnership(), the code will not compile because scopers > // are not copyable; you missed an ampersand. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:315: (*entry_it)->video_capture_device.reset(device.release()); On 2014/12/17 10:14:10, perkj wrote: > On 2014/12/16 22:31:25, tommi wrote: > > nit: (*entry_it)->video_capture_device.swap(device); > > > > Should we DCHECK first that video_capture_device.get() == nullptr ? > > feels like overkill to me. which one is overkill: (*entry_it)->video_capture_device.reset(device.release()); (*entry_it)->video_capture_device.swap(device);
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:265: base::Passed(entry->video_capture_device.Pass()))); On 2014/12/17 11:12:53, tommi wrote: > On 2014/12/17 10:14:10, perkj wrote: > > On 2014/12/16 22:31:25, tommi wrote: > > > shouldn't this be base::Passed(&entry->video_capture_device)? > > > > > > ../../content/browser/renderer_host/media/video_capture_manager.cc:266:33: > > error: calling a private constructor of class > > 'scoped_ptr<media::VideoCaptureDevice, > > base::DefaultDeleter<media::VideoCaptureDevice> >' > > base::Passed(entry->video_capture_device))); > > ^ > > ../../base/memory/scoped_ptr.h:312:51: note: declared private here > > MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(scoped_ptr) > > ^ > > ../../base/move.h:222:3: note: expanded from macro > > 'MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03' > > type(type&); \ > > > > > > From scoped_ptr.h > > > > // { > > // scoped_ptr<Foo> ptr(new Foo("yay")); // ptr manages Foo("yay"). > > // TakesOwnership(ptr.Pass()); // ptr no longer owns Foo("yay"). > > // scoped_ptr<Foo> ptr2 = CreateFoo(); // ptr2 owns the return Foo. > > // scoped_ptr<Foo> ptr3 = // ptr3 now owns what was in > ptr2. > > // PassThru(ptr2.Pass()); // ptr2 is correspondingly > nullptr. > > // } > > // > > // Notice that if you do not call Pass() when returning from PassThru(), or > > // when invoking TakesOwnership(), the code will not compile because scopers > > // are not copyable; > > you missed an ampersand. Seems like I have not fully grasped this magic... You are right of course. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:315: (*entry_it)->video_capture_device.reset(device.release()); On 2014/12/17 11:12:53, tommi wrote: > On 2014/12/17 10:14:10, perkj wrote: > > On 2014/12/16 22:31:25, tommi wrote: > > > nit: (*entry_it)->video_capture_device.swap(device); > > > > > > Should we DCHECK first that video_capture_device.get() == nullptr ? > > > > feels like overkill to me. > > which one is overkill: > > (*entry_it)->video_capture_device.reset(device.release()); > (*entry_it)->video_capture_device.swap(device); The dcheck. But It can never hurt.
https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:315: (*entry_it)->video_capture_device.reset(device.release()); On 2014/12/17 11:47:21, perkj wrote: > On 2014/12/17 11:12:53, tommi wrote: > > On 2014/12/17 10:14:10, perkj wrote: > > > On 2014/12/16 22:31:25, tommi wrote: > > > > nit: (*entry_it)->video_capture_device.swap(device); > > > > > > > > Should we DCHECK first that video_capture_device.get() == nullptr ? > > > > > > feels like overkill to me. > > > > which one is overkill: > > > > (*entry_it)->video_capture_device.reset(device.release()); > > (*entry_it)->video_capture_device.swap(device); > > The dcheck. But It can never hurt. Oh the dcheck, right. Yeah, I don't see how that could happen right now but to make sure we never get here and already have a started device that we then implicitly delete and never call StopAndDeallocate. https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/801363002/diff/60001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.h:274: DeviceEntry* entry; // Owned by |devices_| On 2014/12/17 10:14:10, perkj wrote: > On 2014/12/16 22:31:25, tommi wrote: > > do we use this pointer? (If it's only used for lookup, then it would be good > to > > add a note on that) > > Well, its used in HandleQueuedStartRequest() but we do dcheck that it exists. What about making this pointer type a void* to make sure that it never gets used for anything except lookups? Alternatively, we could generate a serial number per instance and use that as an id. https://codereview.chromium.org/801363002/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/80001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:247: // OnDeviceStarted. I'm dcheck crazy :) Can we dcheck that the first element (if any), has its video_capture_device member not set? Something like: DCHECK(device_start_queue_.empty() || !device_start_queue_.front()->video_capture_device.get()); https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:247: // and recreated while we are waiting for the device to be started. ah... hmm.. maybe it's a good thing to use a serial number instead https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:254: << entry->id << "." nit: align << https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:278: // Set to true if the device should be stopped before it have successfully it have -> it has https://codereview.chromium.org/801363002/diff/140001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:320: DCHECK(!device_entry->video_capture_device); missing .get() or is this now supported in scoped_ptr?
PTAL https://codereview.chromium.org/801363002/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/80001/content/browser/renderer... content/browser/renderer_host/media/video_capture_manager.cc:247: // OnDeviceStarted. On 2014/12/17 14:46:12, tommi wrote: > I'm dcheck crazy :) Can we dcheck that the first element (if any), has its > video_capture_device member not set? Something like: > > DCHECK(device_start_queue_.empty() || > !device_start_queue_.front()->video_capture_device.get()); I removed entry all together from the list. https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:247: // and recreated while we are waiting for the device to be started. On 2014/12/17 14:46:12, tommi wrote: > ah... hmm.. maybe it's a good thing to use a serial number instead Done. https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:254: << entry->id << "." On 2014/12/17 14:46:12, tommi wrote: > nit: align << Done. https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/801363002/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:278: // Set to true if the device should be stopped before it have successfully On 2014/12/17 14:46:12, tommi wrote: > it have -> it has Done. https://codereview.chromium.org/801363002/diff/140001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:320: DCHECK(!device_entry->video_capture_device); On 2014/12/17 14:46:12, tommi wrote: > missing .get() or is this now supported in scoped_ptr? Seems to compile. Which operator would that be?
lgtm https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:109: : start_id(g_device_start_id++), is there a way to dcheck that all DeviceEntry instances are created on the same thread? (so that the g_device_start_id++ operation is safe) https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:120: : start_id(serial_id), can we call the member variable serial_id too? Would actually be good to change CaptureDeviceStartRequest to a class now that it's getting to be a bit complex (I'm fine with a todo on that if you agree) https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:287: return e->start_id == start_id; indent by 2 spaces? https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:331: return e->start_id == start_id; and here?
ptal https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:109: : start_id(g_device_start_id++), On 2015/01/13 21:22:44, tommi wrote: > is there a way to dcheck that all DeviceEntry instances are created on the same > thread? (so that the g_device_start_id++ operation is safe) Not that can think of. https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:120: : start_id(serial_id), On 2015/01/13 21:22:44, tommi wrote: > can we call the member variable serial_id too? Would actually be good to change > CaptureDeviceStartRequest to a class now that it's getting to be a bit complex > (I'm fine with a todo on that if you agree) Done. https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:287: return e->start_id == start_id; On 2015/01/13 21:22:44, tommi wrote: > indent by 2 spaces? Done. https://codereview.chromium.org/801363002/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:331: return e->start_id == start_id; On 2015/01/13 21:22:44, tommi wrote: > and here? Done.
The CQ bit was checked by perkj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/801363002/220001
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e770b238860b8907f054cecd8be9681922c2e3a0 Cr-Commit-Position: refs/heads/master@{#312081} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
