Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738763002:
[Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice (Closed)
|
Created:
3 years, 9 months ago by chfremer Modified:
3 years, 8 months ago CC:
chromium-reviews, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice
This CL is supposed to be a pure refactoring. There should be no changes to
existing behavior.
This is part of a series of CLs with the goal of separating the class
VideoCaptureManager from knowledge of things that are going to be private
implementation details of the video capture service. The end goal is to have
VideoCaptureManager talk to an abstraction that can be implemented by either the
video capture Mojo service or the existing (legacy) in-process implementation.
Changes in this CL:
* Add abstraction (interface) BuildableVideoCaptureDevice to be operated by
class VideoCaptureManager. Add a callback interface BuildableDeviceCallbacks
for BuildableVideoCaptureDevice to send messages back to VideoCaptureManager.
* Create an implementation InProcessBuildableVideoCaptureDevice and move the
logic for building and releasing devices from VideoCaptureManager into there.
* Promote struct VideoCaptureManager::DeviceEntry to class
VideoCaptureDeviceEntry and move it to a separate file.
* In VideoCaptureManager, changed ownership of DeviceEntries from
std::unique_ptr<> to scoped_refptr<>. The reason for this is that
VideoCaptureManager wants to temporarily pass ownership of it to an
asynchronous operation. Before this CL, this asynchronous operation took
shared ownership of the whole VideoCaptureManager instead.
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL15.
BUG=584797
TEST=
content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"
content_unittests --gtest_filter="*Video*"
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
Review-Url: https://codereview.chromium.org/2738763002
Cr-Commit-Position: refs/heads/master@{#461217}
Committed: https://chromium.googlesource.com/chromium/src/+/fb141a46523161bc47659b88dd1e76e36a727d6c
Patch Set 1 #Patch Set 2 : Rebase to Feb 10th and change similarity to 10 #
Total comments: 36
Patch Set 3 : Incorporated miu@'s suggestions from PatchSet 2 #
Total comments: 4
Patch Set 4 : Incorporated miu@'s suggestions from PatchSet 3 #
Total comments: 12
Patch Set 5 : Incorporated emircan@'s suggestions from Patch Set 4 #Patch Set 6 : Use base::OnceClosure for ownership, use DeviceBuildContext #
Total comments: 15
Patch Set 7 : Use BuildableDeviceCallbacks* plus OnceClosure instead of DeviceBuildContext #
Total comments: 8
Patch Set 8 : Incorporated mcasas@'s suggestions from PatchSet 7 and Similarity 50 #
Total comments: 16
Patch Set 9 : Incorporated suggestions from Patch Set 8 #Patch Set 10 : Rebase to March 28 #Patch Set 11 : Rebase to March 30 #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 78 (46 generated)
Description was changed from ========== test ========== to ========== [Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice This CL is supposed to be a pure refactoring. There should be no changes to existing behavior. This is part of a series of CLs with the goal of separating the class VideoCaptureManager from knowledge of things that are going to be private implementation details of the video capture service. The end goal is to have VideoCaptureManager talk to an abstraction that can be implemented by either the video capture Mojo service or the existing (legacy) in-process implementation. Changes in this CL: * Add abstraction (interface) BuildableVideoCaptureDevice to be operated by class VideoCaptureManager. Add a callback interface BuildableDeviceCallbacks for BuildableVideoCaptureDevice to send messages back to VideoCaptureManager. * Create an implementation InProcessBuildableVideoCaptureDevice and move the logic for building and releasing devices from VideoCaptureManager into there. * Promote struct VideoCaptureManager::DeviceEntry to class VideoCaptureDeviceEntry and move it to a separate file. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL15. BUG=584797 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ==========
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice This CL is supposed to be a pure refactoring. There should be no changes to existing behavior. This is part of a series of CLs with the goal of separating the class VideoCaptureManager from knowledge of things that are going to be private implementation details of the video capture service. The end goal is to have VideoCaptureManager talk to an abstraction that can be implemented by either the video capture Mojo service or the existing (legacy) in-process implementation. Changes in this CL: * Add abstraction (interface) BuildableVideoCaptureDevice to be operated by class VideoCaptureManager. Add a callback interface BuildableDeviceCallbacks for BuildableVideoCaptureDevice to send messages back to VideoCaptureManager. * Create an implementation InProcessBuildableVideoCaptureDevice and move the logic for building and releasing devices from VideoCaptureManager into there. * Promote struct VideoCaptureManager::DeviceEntry to class VideoCaptureDeviceEntry and move it to a separate file. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL15. BUG=584797 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== [Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice This CL is supposed to be a pure refactoring. There should be no changes to existing behavior. This is part of a series of CLs with the goal of separating the class VideoCaptureManager from knowledge of things that are going to be private implementation details of the video capture service. The end goal is to have VideoCaptureManager talk to an abstraction that can be implemented by either the video capture Mojo service or the existing (legacy) in-process implementation. Changes in this CL: * Add abstraction (interface) BuildableVideoCaptureDevice to be operated by class VideoCaptureManager. Add a callback interface BuildableDeviceCallbacks for BuildableVideoCaptureDevice to send messages back to VideoCaptureManager. * Create an implementation InProcessBuildableVideoCaptureDevice and move the logic for building and releasing devices from VideoCaptureManager into there. * Promote struct VideoCaptureManager::DeviceEntry to class VideoCaptureDeviceEntry and move it to a separate file. * In VideoCaptureManager, changed ownership of DeviceEntries from std::unique_ptr<> to scoped_refptr<>. The reason for this is that VideoCaptureManager wants to temporarily pass ownership of it to an asynchronous operation. Before this CL, this asynchronous operation took shared ownership of the whole VideoCaptureManager instead. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL15. BUG=584797 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ==========
chfremer@chromium.org changed reviewers: + emircan@chromium.org, mcasas@chromium.org, miu@chromium.org
miu@: PTAL emircan@: PTAL mcasas@: FYI (let's finish up with https://codereview.chromium.org/2735083002 first)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/07 23:32:49, chfremer wrote: > miu@: PTAL > emircan@: PTAL > mcasas@: FYI (let's finish up with https://codereview.chromium.org/2735083002 > first) ping
On 2017/03/10 17:27:34, chfremer wrote: > On 2017/03/07 23:32:49, chfremer wrote: > > miu@: PTAL > > emircan@: PTAL > > mcasas@: FYI (let's finish up with https://codereview.chromium.org/2735083002 > > first) > > ping In Patch Set 2, I changed the similarity to 10 in order to make it easier to see which parts of in_process_buildable_video_capture_device.cc are moved from video_capture_manager.cc. Not quite sure though if it helps more than it hurts.
Comments for PS2: https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:27: virtual void OnDeviceAboutToStart(media::VideoFacingMode facing_mode) = 0; I've seen a lot of similar code throughout Chromium use the "will" and "did" naming in situations like this. So consider naming these: WillStartDevice(facing_mode); DidStartDevice(entry); OnDeviceStartFailed(entry); https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:43: class BuildableVideoCaptureDevice { naming/description suggestion: It looks like this would more-accurately be called a "launcher" (VideoCaptureDeviceLauncher), since this code is all about creating and starting a VideoCaptureDevice, and then owning its lifecycle from there, and notifying other entities of when it started. Your call... https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:48: // The passed-in |context_reference| must guarantee that the context relevant I'm a little confused by this. If the BuildableVideoCaptureDevice::CreateAndStartDeviceAsync() method is taking owership of |context_reference|, isn't it the responsibility of the impl to not destroy |context_reference| until after it is no longer needed? Maybe the comment is redundant? Or, am I misunderstanding something? https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:61: virtual bool HasDevice() const = 0; naming suggestion: s/HasDevice/IsDeviceAlive/ They're mostly equivalent except that my suggestion would hint to readers of the code that something interesting has to happen before we have a device, and something interesting could happen to make it go away later. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:59: return base::MakeUnique<content::VideoCaptureGpuJpegDecoder>(decode_done_cb); Can we delete this function? If the caller is going to do this: auto decoder = CreateGpuJpegDecoder(my_done_callback); Why can't it just: auto decoder = base::MakeUnique<content::VideoCaptureGpuJpegDecoder>(my_done_callback); Having the function implies there are special additional steps to instantiating the class, which there aren't. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:63: media::VideoCaptureDevice* device, nit: Should this arg just be std::unique_ptr<...> device? Then, you don't need the delete statement below. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:97: DCHECK(false); Debug and Release builds are meant to behave the same. So, this whole dtor should either be: // ReleaseDeviceAsync should have been called before this point. DCHECK(!device_); or: if (device_) ReleaseDeviceAsyncNonVirtual(); If it's difficult to reason which of these is correct, go with the DCHECK(). There's tons of Chromium infrastructure to catch memory leakage and crashes long before this code will reach stable channel. :) Also, if you go with the former, then there's no need for a special extra "...NonVirtual()" method. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:116: base::Closure start_capture_function; naming nit: Up to you, but IMHO, s/_function/_closure/ here and in the next variable since these aren't actually function pointers. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:269: return device_task_runner_->BelongsToCurrentThread(); Seems redundant to have this. Same reasoning as why we don't need CreateGpuJpegDecoder(). I haven't seen other Chromium code ever separate this "belongs to thread DCHECK" out into a separate method. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_entry.cc (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_entry.cc:28: void VideoCaptureDeviceEntry::CreateAndStartDeviceAsync( It looks like every method in this class is just a one-liner hop to a method call in |controller_| or |buildable_device_|. So, we're adding function call overhead without any benefit, and lots of LOC that don't add any value. IMHO, it would be better to delete all of the methods, and just provide accessors: BuildableVideoCaptureDevice* device(); VideoCaptureController* controller(); Then, instead of callers doing this: entry->TakePhoto(...); They do this: entry->device()->TakePhoto(...); -------------- Or, another idea: Maybe VideoCaptureDeviceEntry should be merged into VideoCaptureController? It already has a "composed of" relationship to VideoCaptureController; and it wouldn't be a far stretch for VideoCaptureController to own |buildable_device_|. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_entry.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_entry.h:14: // When first created (in GetOrCreateDeviceEntry()), this consists of Is GetOrCreateDeviceEntry() in another class? Perhaps you could update the class-level comments in terms of what owns instances of this and how it uses this. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:258: bool VideoCaptureManager::LookupDeviceDescriptor( Suggestion for simplification: media::VideoCaptureDeviceDescriptor* VideoCaptureManager::LookupDeviceDescriptor(const std::string& id) { const DeviceInfo* info = GetDeviceInfoById(id); return info ? info->descriptor : nullptr; } https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:399: MakeScopedRefptrOwnership(GetDeviceEntrySharedRefBySerialId(serial_id))); Interesting. It feels like you're creating your own base::Closure here with the Ownership interface and the OwnershipCollection impl. What if you just use base::Bind() to create a "done callback" Closure that does nothing other than release ref-counts for all the objects needed during the operation? Something like: void ReleaseReferences(const scoped_refptr<VideoCaptureManager>& manager, const scoped_refptr<DeviceEntry>& entry) {} and then: entry->CreateAndStartDeviceAsync(..., base::Bind(&ReleaseReferences, this, GetDeviceEntrySharedRefBySerialId(serial_id))); https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:423: entry->ReleaseDeviceAsync(MakeScopedRefptrOwnership( ditto here: Feels like a done callback would work equally well without needing an extra "Ownership" utility class. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:554: if (entry->HasDevice() == false) nit: if (!entry->HasDevice()) (and in multiple places elsewhere...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
miu@: PTAL https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:27: virtual void OnDeviceAboutToStart(media::VideoFacingMode facing_mode) = 0; On 2017/03/11 01:13:21, miu wrote: > I've seen a lot of similar code throughout Chromium use the "will" and "did" > naming in situations like this. So consider naming these: > > WillStartDevice(facing_mode); > DidStartDevice(entry); > OnDeviceStartFailed(entry); > I like those names when reading the calling code. But I don't like them when reading the implementing class, in this case VideoCaptureManager. When seeing a class expose methods WillDoXYZ() or DidXYZ(), I would intuitively expect them to return bool and tell the caller whether or not the class did or will be doing XYZ. Let's go with the "will" and "did" convention for now. The "WillStartDevice" is hopefully going away in a few CLs, so we may be able to simplify then. Done. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:43: class BuildableVideoCaptureDevice { On 2017/03/11 01:13:21, miu wrote: > naming/description suggestion: It looks like this would more-accurately be > called a "launcher" (VideoCaptureDeviceLauncher), since this code is all about > creating and starting a VideoCaptureDevice, and then owning its lifecycle from > there, and notifying other entities of when it started. Your call... Hmm, now that I think about it with a fresh mind and some distance (it's been many months since I wrote this code), my impression is as follows. The abstraction "BuildableVideoCaptureDevice" has two parts. One part is about creating and releasing a device. The other part is about operating the device. I agree that "launcher" would be a good name for the first part. But with the second part being present, I feel "BuildableVideoCaptureDevice" is a more suitable name. To make things cleaner, I think we should split the interface in two, one for each part. The first interface should be a "factory" that creates an instance of the second interface, which would be for the device operation. I am not sure why I did not do it this way a few months back, but I was probably just trying to stay conceptually close to the existing code and keep this a pure and mechanical "cut-some-existing-part-out-of-a-large-class" refactoring. Would it be okay if I apply the splitting of this interface after the end of the existing CL range 15-20 that is about refactoring VideoCaptureManager? I am afraid doing it here at 15 would require a lot of merging work for 16-20. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:48: // The passed-in |context_reference| must guarantee that the context relevant On 2017/03/11 01:13:21, miu wrote: > I'm a little confused by this. If the > BuildableVideoCaptureDevice::CreateAndStartDeviceAsync() method is taking > owership of |context_reference|, isn't it the responsibility of the impl to not > destroy |context_reference| until after it is no longer needed? > > Maybe the comment is redundant? Or, am I misunderstanding something? Your understanding is correct. The comment is confusing. The comment was meant for the caller, not the implementor of the method. I felt that the purpose of |context_reference| may not be understood without this information, but my attempt to explain it apparently backfired :-). Could you give a suggestion, what kind of comment (including no comment at all) would help you as a reader understand the purpose of |context_reference|? https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:61: virtual bool HasDevice() const = 0; On 2017/03/11 01:13:21, miu wrote: > naming suggestion: s/HasDevice/IsDeviceAlive/ They're mostly equivalent except > that my suggestion would hint to readers of the code that something interesting > has to happen before we have a device, and something interesting could happen to > make it go away later. Done. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:59: return base::MakeUnique<content::VideoCaptureGpuJpegDecoder>(decode_done_cb); On 2017/03/11 01:13:22, miu wrote: > Can we delete this function? If the caller is going to do this: > > auto decoder = CreateGpuJpegDecoder(my_done_callback); > > Why can't it just: > > auto decoder = > base::MakeUnique<content::VideoCaptureGpuJpegDecoder>(my_done_callback); > > Having the function implies there are special additional steps to instantiating > the class, which there aren't. No. We are not calling this function directly in this file. We pass a Callback bound to it to the constructor of VideoCaptureDeviceClient (in line 285). VideoCaptureDeviceClient cannot create VideoCaptureGpuJpegDecoder itself because it is used in more than one context. When used inside the video capture Mojo service, it creates a different (currently, none) jpeg decoder. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:63: media::VideoCaptureDevice* device, On 2017/03/11 01:13:22, miu wrote: > nit: Should this arg just be std::unique_ptr<...> device? Then, you don't need > the delete statement below. I agree that would be cleaner. The reason I did it this way is to enable the (somewhat awkward) fallback mechanism happening in line 430. I don't think we have any test case covering that code path, and I am not sure if it is needed, but since it was there, I did not want to change it. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:97: DCHECK(false); On 2017/03/11 01:13:22, miu wrote: > Debug and Release builds are meant to behave the same. So, this whole dtor > should either be: > > // ReleaseDeviceAsync should have been called before this point. > DCHECK(!device_); > > or: > > if (device_) > ReleaseDeviceAsyncNonVirtual(); > > If it's difficult to reason which of these is correct, go with the DCHECK(). > There's tons of Chromium infrastructure to catch memory leakage and crashes long > before this code will reach stable channel. :) > > Also, if you go with the former, then there's no need for a special extra > "...NonVirtual()" method. Done. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:116: base::Closure start_capture_function; On 2017/03/11 01:13:21, miu wrote: > naming nit: Up to you, but IMHO, s/_function/_closure/ here and in the next > variable since these aren't actually function pointers. Done. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:269: return device_task_runner_->BelongsToCurrentThread(); On 2017/03/11 01:13:22, miu wrote: > Seems redundant to have this. Same reasoning as why we don't need > CreateGpuJpegDecoder(). I haven't seen other Chromium code ever separate this > "belongs to thread DCHECK" out into a separate method. This was taken from class VideoCaptureManager. Agreed that it is not needed. Removed for both classes. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_entry.cc (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_entry.cc:28: void VideoCaptureDeviceEntry::CreateAndStartDeviceAsync( On 2017/03/11 01:13:22, miu wrote: > It looks like every method in this class is just a one-liner hop to a method > call in |controller_| or |buildable_device_|. So, we're adding function call > overhead without any benefit, and lots of LOC that don't add any value. IMHO, it > would be better to delete all of the methods, and just provide accessors: > > BuildableVideoCaptureDevice* device(); > VideoCaptureController* controller(); > > Then, instead of callers doing this: > > entry->TakePhoto(...); > > They do this: > > entry->device()->TakePhoto(...); > > -------------- > > Or, another idea: Maybe VideoCaptureDeviceEntry should be merged into > VideoCaptureController? It already has a "composed of" relationship to > VideoCaptureController; and it wouldn't be a far stretch for > VideoCaptureController to own |buildable_device_|. I agree that this class, now that all interesting logic has moved into |buildable_device_|, is not really doing much. I wouldn't say it does nothing, though. It does the |g_device_start_id| thing, and (more importantly) it hides the implementation detail that there is a "controller" and a "buildable device" from the VideoCaptureManager. This has value. But I agree that it may not be enough to justify the existence of this class. Note that, for clarity, this CL was originally meant to contain nothing other than the cut made into VideoCaptureManager. But I really like the idea of merging it into VideoCaptureController. Let me try doing that. Done. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_entry.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_entry.h:14: // When first created (in GetOrCreateDeviceEntry()), this consists of On 2017/03/11 01:13:22, miu wrote: > Is GetOrCreateDeviceEntry() in another class? Perhaps you could update the > class-level comments in terms of what owns instances of this and how it uses > this. Thanks for pointing this out. I removed the "(in GetOrCreateDeviceEntry())" part of the comment, since this class is no longer inside the scope of VideoCaptureManager. I generally try to avoid using the class-level comment to explain where a class is used. Doing so would encourage tight coupling between the class and its users. To look up usages of the class, we can instead safely use the code search tool. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:258: bool VideoCaptureManager::LookupDeviceDescriptor( On 2017/03/11 01:13:22, miu wrote: > Suggestion for simplification: > > media::VideoCaptureDeviceDescriptor* > VideoCaptureManager::LookupDeviceDescriptor(const std::string& id) { > const DeviceInfo* info = GetDeviceInfoById(id); > return info ? info->descriptor : nullptr; > } Done. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:399: MakeScopedRefptrOwnership(GetDeviceEntrySharedRefBySerialId(serial_id))); On 2017/03/11 01:13:22, miu wrote: > Interesting. It feels like you're creating your own base::Closure here with the > Ownership interface and the OwnershipCollection impl. What if you just use > base::Bind() to create a "done callback" Closure that does nothing other than > release ref-counts for all the objects needed during the operation? Something > like: > > void ReleaseReferences(const scoped_refptr<VideoCaptureManager>& manager, > const scoped_refptr<DeviceEntry>& entry) {} > > and then: > > entry->CreateAndStartDeviceAsync(..., > base::Bind(&ReleaseReferences, > this, > GetDeviceEntrySharedRefBySerialId(serial_id))); > Interesting comparison of this mechanism to a base::Closure. I agree, that a base::Closure could be used here to a similar effect. However, I see some disadvantages of doing so. 1.) A base::Closure needs to explicitly be called, and using it here opens up the possibility of the method implementation forgetting to call it. 2.) Since it is much more general, a base::Closure does not make the intent clear to the reader of the method implementation. It does not reveal that the closure owns the relevant context. Readers of the method implementation may not expect that invoking the closure may destroy the called instance itself. 3.) Disregarding the fact that base::Closure is an commonly used existing concept in Chromium and using a std::unique_ptr<Ownership> is not, the latter mechanism is much simpler. I feel that the advantages of introducing Ownership and OwnershipCollection outweigh the cost of the extra code. Please let me know your view on this. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:423: entry->ReleaseDeviceAsync(MakeScopedRefptrOwnership( On 2017/03/11 01:13:22, miu wrote: > ditto here: Feels like a done callback would work equally well without needing > an extra "Ownership" utility class. See other reply. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:554: if (entry->HasDevice() == false) On 2017/03/11 01:13:22, miu wrote: > nit: > > if (!entry->HasDevice()) > > (and in multiple places elsewhere...) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:43: class BuildableVideoCaptureDevice { On 2017/03/13 22:02:03, chfremer wrote: > On 2017/03/11 01:13:21, miu wrote: > > naming/description suggestion: It looks like this would more-accurately be > > called a "launcher" (VideoCaptureDeviceLauncher), since this code is all about > > creating and starting a VideoCaptureDevice, and then owning its lifecycle from > > there, and notifying other entities of when it started. Your call... > > Hmm, now that I think about it with a fresh mind and some distance (it's been > many months since I wrote this code), my impression is as follows. The > abstraction "BuildableVideoCaptureDevice" has two parts. One part is about > creating and releasing a device. The other part is about operating the device. I > agree that "launcher" would be a good name for the first part. But with the > second part being present, I feel "BuildableVideoCaptureDevice" is a more > suitable name. Ack. > To make things cleaner, I think we should split the interface in two, one for > each part. The first interface should be a "factory" that creates an instance of > the second interface, which would be for the device operation. I am not sure why > I did not do it this way a few months back, but I was probably just trying to > stay conceptually close to the existing code and keep this a pure and mechanical > "cut-some-existing-part-out-of-a-large-class" refactoring. IIUC, by "factory," you mean "launcher," right? > Would it be okay if I apply the splitting of this interface after the end of the > existing CL range 15-20 that is about refactoring VideoCaptureManager? I am > afraid doing it here at 15 would require a lot of merging work for 16-20. SGTM. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:59: return base::MakeUnique<content::VideoCaptureGpuJpegDecoder>(decode_done_cb); On 2017/03/13 22:02:03, chfremer wrote: > On 2017/03/11 01:13:22, miu wrote: > > Can we delete this function? If the caller is going to do this: > > > > auto decoder = CreateGpuJpegDecoder(my_done_callback); > > > > Why can't it just: > > > > auto decoder = > > base::MakeUnique<content::VideoCaptureGpuJpegDecoder>(my_done_callback); > > > > Having the function implies there are special additional steps to > instantiating > > the class, which there aren't. > > No. We are not calling this function directly in this file. We pass a Callback > bound to it to the constructor of VideoCaptureDeviceClient (in line 285). > VideoCaptureDeviceClient cannot create VideoCaptureGpuJpegDecoder itself because > it is used in more than one context. When used inside the video capture Mojo > service, it creates a different (currently, none) jpeg decoder. Whoops, didn't see that. SGTM. https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:63: media::VideoCaptureDevice* device, On 2017/03/13 22:02:03, chfremer wrote: > On 2017/03/11 01:13:22, miu wrote: > > nit: Should this arg just be std::unique_ptr<...> device? Then, you don't need > > the delete statement below. > > I agree that would be cleaner. > The reason I did it this way is to enable the (somewhat awkward) fallback > mechanism happening in line 430. I don't think we have any test case covering > that code path, and I am not sure if it is needed, but since it was there, I did > not want to change it. Acknowledged. https://codereview.chromium.org/2738763002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2738763002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:630: bool VideoCaptureManager::GetDeviceFormatsInUse( Not related to this CL, but this method signature is wonky too: It has a meaningless bool return value (because it unconditionally returns true), and it modifies a list of things when it could just return the one thing it provides (when available). IMHO, it should look like: base::Optional<media::VideoCaptureFormat> GetDeviceFormat(stream_type, device_id) { ... return device_in_use ? device_in_use->GetVideoCaptureFormat() : base::nullopt; } (You might consider fixing this in one of your upcoming CLs, if any modify these LOC.) https://codereview.chromium.org/2738763002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.h (right): https://codereview.chromium.org/2738763002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.h:247: void DestroyDeviceEntryIfNoClients(VideoCaptureController* controller); Looks like you might have wanted to also rename this. Something like DestroyControllerIfNoClients().
emicran@: PTAL content/browser/renderer_host/media/{!video}_*
https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/buildable_video_capture_device.h
(right):
https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/buildable_video_capture_device.h:43: class
BuildableVideoCaptureDevice {
On 2017/03/16 00:01:35, miu wrote:
> On 2017/03/13 22:02:03, chfremer wrote:
> > On 2017/03/11 01:13:21, miu wrote:
> > > naming/description suggestion: It looks like this would more-accurately be
> > > called a "launcher" (VideoCaptureDeviceLauncher), since this code is all
> about
> > > creating and starting a VideoCaptureDevice, and then owning its lifecycle
> from
> > > there, and notifying other entities of when it started. Your call...
> >
> > Hmm, now that I think about it with a fresh mind and some distance (it's
been
> > many months since I wrote this code), my impression is as follows. The
> > abstraction "BuildableVideoCaptureDevice" has two parts. One part is about
> > creating and releasing a device. The other part is about operating the
device.
> I
> > agree that "launcher" would be a good name for the first part. But with the
> > second part being present, I feel "BuildableVideoCaptureDevice" is a more
> > suitable name.
>
> Ack.
>
> > To make things cleaner, I think we should split the interface in two, one
for
> > each part. The first interface should be a "factory" that creates an
instance
> of
> > the second interface, which would be for the device operation. I am not sure
> why
> > I did not do it this way a few months back, but I was probably just trying
to
> > stay conceptually close to the existing code and keep this a pure and
> mechanical
> > "cut-some-existing-part-out-of-a-large-class" refactoring.
>
> IIUC, by "factory," you mean "launcher," right?
Sorry, yes, I meant the "launcher" part. I got ahead of myself thinking about
what we might call it after splitting the interface. When split out, it may make
sense to call it a "factory", because it would be creating an object
(representing a launched device) that can then be operated. Let's revisit that
idea when we do the splitting.
>
> > Would it be okay if I apply the splitting of this interface after the end of
> the
> > existing CL range 15-20 that is about refactoring VideoCaptureManager? I am
> > afraid doing it here at 15 would require a lot of merging work for 16-20.
>
> SGTM.
https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/buildable_video_capture_device.h:48: // The
passed-in |context_reference| must guarantee that the context relevant
On 2017/03/13 22:02:02, chfremer wrote:
> On 2017/03/11 01:13:21, miu wrote:
> > I'm a little confused by this. If the
> > BuildableVideoCaptureDevice::CreateAndStartDeviceAsync() method is taking
> > owership of |context_reference|, isn't it the responsibility of the impl to
> not
> > destroy |context_reference| until after it is no longer needed?
> >
> > Maybe the comment is redundant? Or, am I misunderstanding something?
>
> Your understanding is correct. The comment is confusing.
> The comment was meant for the caller, not the implementor of the method.
> I felt that the purpose of |context_reference| may not be understood without
> this information, but my attempt to explain it apparently backfired :-). Could
> you give a suggestion, what kind of comment (including no comment at all)
would
> help you as a reader understand the purpose of |context_reference|?
I still haven't found a better way of explaining |context_reference| with a
comment. Please let me know if you have any ideas what we could write here. I am
also okay with removing the comment altogether if it more confusing than
helpful.
https://codereview.chromium.org/2738763002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):
https://codereview.chromium.org/2738763002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:630: bool
VideoCaptureManager::GetDeviceFormatsInUse(
On 2017/03/16 00:01:36, miu wrote:
> Not related to this CL, but this method signature is wonky too: It has a
> meaningless bool return value (because it unconditionally returns true), and
it
> modifies a list of things when it could just return the one thing it provides
> (when available). IMHO, it should look like:
>
> base::Optional<media::VideoCaptureFormat> GetDeviceFormat(stream_type,
> device_id) {
> ...
> return device_in_use ? device_in_use->GetVideoCaptureFormat() :
> base::nullopt;
> }
>
> (You might consider fixing this in one of your upcoming CLs, if any modify
these
> LOC.)
Agreed. Also it seems that the behavior of the implementation does not match the
specification in the method-level comment in the header. I'll file this as a bug
to look at separately from this CL. crbug.com/702271
https://codereview.chromium.org/2738763002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.h (right):
https://codereview.chromium.org/2738763002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:247: void
DestroyDeviceEntryIfNoClients(VideoCaptureController* controller);
On 2017/03/16 00:01:36, miu wrote:
> Looks like you might have wanted to also rename this. Something like
> DestroyControllerIfNoClients().
Done.
content/browser/renderer_host/media/{!video}_* lgtm % nits
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
File
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc
(right):
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
2017
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:112:
// Use of |this| is safe, because |context_reference| guarantees that |this|
s/Use of |this| is safe/Use of Unretained() is safe/
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
File
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h
(right):
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
2017
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.h:31:
VideoCaptureController* entry,
VideoCaptureController* const entry
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/ownership.h (right):
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
content/browser/renderer_host/media/ownership.h:29:
ScopedRefptrOwnership(scoped_refptr<T> refptr) : refptr_(std::move(refptr)) {}
Make this constructor to private and move MakeScopedRefptrOwnership here as a
static method?
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.h (right):
https://codereview.chromium.org/2738763002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:29: //
CreateAndStartDeviceAsync() has beend called, ReleaseDeviceAsync() must be
s/beend/been/
chfremer@chromium.org changed reviewers: + avi@chromium.org
avi@: Please RS content/browser/BUILD.gn https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/16 20:54:03, emircan wrote: > 2017 Done. https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:112: // Use of |this| is safe, because |context_reference| guarantees that |this| On 2017/03/16 20:54:03, emircan wrote: > s/Use of |this| is safe/Use of Unretained() is safe/ Done. https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/16 20:54:03, emircan wrote: > 2017 Done. https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.h:31: VideoCaptureController* entry, On 2017/03/16 20:54:03, emircan wrote: > VideoCaptureController* const entry Actually, now that |entry| and |controller| are the same, the |entry| parameter is redundant. I am removing it now. Making the pointer const does not add any value, since it is already a copy (of a pointer). It would be equivalent to making a parameter "const int" instead of just "int", which we also don't do. https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... File content/browser/renderer_host/media/ownership.h (right): https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... content/browser/renderer_host/media/ownership.h:29: ScopedRefptrOwnership(scoped_refptr<T> refptr) : refptr_(std::move(refptr)) {} On 2017/03/16 20:54:03, emircan wrote: > Make this constructor to private and move MakeScopedRefptrOwnership here as a > static method? I moved the MakeScopedRefptrOwnership out of this class on purpose, because it makes usage sites look a lot nicer by not requiring the template parameter to be repeated. MakeScopedRefptrOwnership(some_scoped_refptr_instance) vs. ScopedRefptrOwnership<SomeType>::CreateInstance(some_scoped_refptr_instance) https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2738763002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:29: // CreateAndStartDeviceAsync() has beend called, ReleaseDeviceAsync() must be On 2017/03/16 20:54:03, emircan wrote: > s/beend/been/ Done.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Revisiting prior discussion: https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/buildable_video_capture_device.h:48: // The passed-in |context_reference| must guarantee that the context relevant On 2017/03/16 17:39:39, chfremer wrote: > I still haven't found a better way of explaining |context_reference| with a > comment. Please let me know if you have any ideas what we could write here. I am > also okay with removing the comment altogether if it more confusing than > helpful. That's because it should be a "done callback" to release the references that need to be held until the operation is complete. ;-) So, I don't have a strong opinion, but you seemed to, which is why I didn't push back more earlier. FWIW, if I was writing this code, I'd have used Closures rather than add a custom reference-holding scheme. Another way to look at this: If you have to write the code in ownership.h/.cc, why is it not being committed in src/base? Let's revisit the earlier thread, where you discuss some disadvantages of using Closures. CIL: > 1.) A base::Closure needs to explicitly be called, and using it here opens up > the possibility of the method implementation forgetting to call it. Actually, this isn't a problem at all. The Closure takes ownership of all objects bound to it. So, if your code didn't Run() it, the Closure would still eventually be destroyed anyway, which would chain to the destruction to those objects. > 2.) Since it is much more general, a base::Closure does not make the intent > clear to the reader of the method implementation. It does not reveal that the > closure owns the relevant context. Readers of the method implementation may not > expect that invoking the closure may destroy the called instance itself. Wait. Sure it does. A Closure is, by definition, a binding of a function with a set of owned objects that will be passed as arguments. If you name it a "done callback," then it's also clear that the client is using this to execute something after the operation completes. That "something" could be anything, and is not important to the method implementation, only that it is Run() after everything the impl does. The client is the entity having the knowledge that some objects must remain alive until the operation completes; so I don't see how this isn't a reasonable interface contract. FWIW, there are plenty of examples of code throughout Chromium that use "done callbacks" for nothing other than destroying objects after an async operation completes. Also, there are lots of examples where people have written code comments on the impl side, like "Warning: Running this callback may delete |this|!" > 3.) Disregarding the fact that base::Closure is an commonly used existing > concept in Chromium and using a std::unique_ptr<Ownership> is not, the latter > mechanism is much simpler. Let's compare code written both ways. First, here's an example where you use Ownership: + auto context_reference = base::MakeUnique<OwnershipCollection>(); + context_reference->AttachOwnership( + MakeScopedRefptrOwnership(scoped_refptr<VideoCaptureManager>(this))); + context_reference->AttachOwnership( + MakeScopedRefptrOwnership(GetControllerSharedRefFromSerialId(serial_id))); + // TODO(chfremer): Check if request->params() can actually be different from + // controller->parameters, and simplify if this is not the case. + controller->CreateAndStartDeviceAsync( + request->params(), static_cast<BuildableDeviceCallbacks*>(this), + std::move(context_reference)); Here's how this would look using a Closure: + // TODO(chfremer): Check if request->params() can actually be different from + // controller->parameters, and simplify if this is not the case. + controller->CreateAndStartDeviceAsync( + request->params(), static_cast<BuildableDeviceCallbacks*>(this), + base::Bind(&OnStartDeviceCompleted, + this, GetControllerSharedRefFromSerialId(serial_id)); Of course, you'd also need to write the trivial OnStartDeviceCompleted() function: + void OnStartDeviceCompleted( + const scoped_refptr<VideoCaptureManager>& manager, + const scoped_refptr<VideoCaptureController>& controller) {} But where you would be adding 3 LOC for this helper function, the 69 LOC in ownership.h and ownership.cc could be deleted. In addition, OwnershipCollection is maintaining a vector w/ heap allocation: This cost in CPU and memory is incurred at run time, not at compile time. > I feel that the advantages of introducing Ownership and OwnershipCollection > outweigh the cost of the extra code. Please let me know your view on this. If you're still not convinced of my position, you may want to get second opinions and ask this question on chromium-dev@.
Thanks for the detailed explanations. I was hoping for some good reasons to not have to use Ownership, and was surprised to not get any further push back at first. This is very helpful. On 2017/03/17 21:18:15, miu wrote: > Revisiting prior discussion: > > https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... > File content/browser/renderer_host/media/buildable_video_capture_device.h > (right): > > https://codereview.chromium.org/2738763002/diff/60001/content/browser/rendere... > content/browser/renderer_host/media/buildable_video_capture_device.h:48: // The > passed-in |context_reference| must guarantee that the context relevant > On 2017/03/16 17:39:39, chfremer wrote: > > I still haven't found a better way of explaining |context_reference| with a > > comment. Please let me know if you have any ideas what we could write here. I > am > > also okay with removing the comment altogether if it more confusing than > > helpful. > > That's because it should be a "done callback" to release the references that > need to be held until the operation is complete. ;-) > > So, I don't have a strong opinion, but you seemed to, which is why I didn't push > back more earlier. FWIW, if I was writing this code, I'd have used Closures > rather than add a custom reference-holding scheme. Another way to look at this: > If you have to write the code in ownership.h/.cc, why is it not being committed > in src/base? > > Let's revisit the earlier thread, where you discuss some disadvantages of using > Closures. CIL: > > > 1.) A base::Closure needs to explicitly be called, and using it here opens up > > the possibility of the method implementation forgetting to call it. > > Actually, this isn't a problem at all. The Closure takes ownership of all > objects bound to it. So, if your code didn't Run() it, the Closure would still > eventually be destroyed anyway, which would chain to the destruction to those > objects. Agreed. I didn't think of that. > > 2.) Since it is much more general, a base::Closure does not make the intent > > clear to the reader of the method implementation. It does not reveal that the > > closure owns the relevant context. Readers of the method implementation may > not > > expect that invoking the closure may destroy the called instance itself. > > Wait. Sure it does. A Closure is, by definition, a binding of a function with a > set of owned objects that will be passed as arguments. > > If you name it a "done callback," then it's also clear that the client is using > this to execute something after the operation completes. That "something" could > be anything, and is not important to the method implementation, only that it is > Run() after everything the impl does. The client is the entity having the > knowledge that some objects must remain alive until the operation completes; so > I don't see how this isn't a reasonable interface contract. I think you got me convinced here. My mistake here was the way I thought about the meaning of a base::Callback (including base::Closure). I used to think of it as "something that can be invoked, but that does not own anything". I see now that a base::Callback expresses both invokability and ownership. But something still bugs me. In our example + void CreateAndStartDeviceAsync(const media::VideoCaptureParams& params, + BuildableDeviceCallbacks* callbacks, + std::unique_ptr<Ownership> context_reference); we had the handle to the callback interface, i.e. |callbacks|, separated from the ownership of the required context, i.e. |context_reference|. When using a base::OnceClosure |done_cb| for the ownership instead, I find it slightly confusing that we now have two somewhat competing handles to make callbacks to and that |done_cb| does not actually have to be invoked at all. Thinking outloud what general conclusion I can draw from this, it seems a |done_cb| works great if there is only a single callback signature. But if we have more than one, e.g. all methods in BuildableDeviceCallbacks, a |done_cb| does not seem to be ideal. If we wanted to combine a multi-callback-signature interface with context ownership into a single object, we could do that by making it look like this + void CreateAndStartDeviceAsync(const media::VideoCaptureParams& params, + std::unique_ptr<BuildableDeviceCallbacks> callback_context); and then pass in an custom implementation of BuildableDeviceCallbacks that owns the relevant context and forwards all calls to the interested listener. But we probably want to avoid having to create such custom implementations in general. It seems what I want is something like base::OnceCallback<BuildableDeviceCallback>, i.e. a closure with a multi-method interface. > FWIW, there are plenty of examples of code throughout Chromium that use "done > callbacks" for nothing other than destroying objects after an async operation > completes. Also, there are lots of examples where people have written code > comments on the impl side, like "Warning: Running this callback may delete > |this|!" > > > 3.) Disregarding the fact that base::Closure is an commonly used existing > > concept in Chromium and using a std::unique_ptr<Ownership> is not, the latter > > mechanism is much simpler. > > Let's compare code written both ways. First, here's an example where you use > Ownership: > > + auto context_reference = base::MakeUnique<OwnershipCollection>(); > + context_reference->AttachOwnership( > + MakeScopedRefptrOwnership(scoped_refptr<VideoCaptureManager>(this))); > + context_reference->AttachOwnership( > + > MakeScopedRefptrOwnership(GetControllerSharedRefFromSerialId(serial_id))); > + // TODO(chfremer): Check if request->params() can actually be different from > + // controller->parameters, and simplify if this is not the case. > + controller->CreateAndStartDeviceAsync( > + request->params(), static_cast<BuildableDeviceCallbacks*>(this), > + std::move(context_reference)); > > Here's how this would look using a Closure: > > + // TODO(chfremer): Check if request->params() can actually be different from > + // controller->parameters, and simplify if this is not the case. > + controller->CreateAndStartDeviceAsync( > + request->params(), static_cast<BuildableDeviceCallbacks*>(this), > + base::Bind(&OnStartDeviceCompleted, > + this, GetControllerSharedRefFromSerialId(serial_id)); > > Of course, you'd also need to write the trivial OnStartDeviceCompleted() > function: > > + void OnStartDeviceCompleted( > + const scoped_refptr<VideoCaptureManager>& manager, > + const scoped_refptr<VideoCaptureController>& controller) {} > > But where you would be adding 3 LOC for this helper function, the 69 LOC in > ownership.h and ownership.cc could be deleted. What I meant is that the Ownership framework is much simpler than the base::Callback framework. > In addition, OwnershipCollection is maintaining a vector w/ heap allocation: > This cost in CPU and memory is incurred at run time, not at compile time. > > > I feel that the advantages of introducing Ownership and OwnershipCollection > > outweigh the cost of the extra code. Please let me know your view on this. > > If you're still not convinced of my position, you may want to get second > opinions and ask this question on chromium-dev@. I still don't think the solution using a |done_cb| is ideal, but I can certainly live with it. I'll go add a patch set using a base::OnceClosure |done_cb| as you suggested and see how it feels.
On 2017/03/17 22:44:53, chfremer wrote: > But something still bugs me. In our example > + void CreateAndStartDeviceAsync(const media::VideoCaptureParams& params, > + BuildableDeviceCallbacks* callbacks, > + std::unique_ptr<Ownership> context_reference); > we had the handle to the callback interface, i.e. |callbacks|, separated from > the ownership of the required context, i.e. |context_reference|. When using a > base::OnceClosure |done_cb| for the ownership instead, I find it slightly > confusing that we now have two somewhat competing handles to make callbacks to > and that |done_cb| does not actually have to be invoked at all. Oh, okay. Agreed. So, let's think how we might incorporate this functionality into BuildableDeviceCallbacks. OTOH, what if the impl of BuildableDeviceCallbacks holds the references, and releases them whenever DidStartDevice() or OnDeviceStartFailed() is called? In other words, I'm not saying you have to use Closure here: I was focused entirely on replacing the 3rd argument with it; but, now I see we could merge this functionality into the 2nd argument. To be clear, you can think of DidStartDevice() and OnDeviceStartFailed() as two possible "done callbacks," only one of which will be called at the end of the CreateAndStartDeviceAsync() operation. > Thinking outloud what general conclusion I can draw from this, it seems a > |done_cb| works great if there is only a single callback signature. But if we > have more than one, e.g. all methods in BuildableDeviceCallbacks, a |done_cb| > does not seem to be ideal. If we wanted to combine a multi-callback-signature > interface with context ownership into a single object, we could do that by > making it look like this > + void CreateAndStartDeviceAsync(const media::VideoCaptureParams& params, > + std::unique_ptr<BuildableDeviceCallbacks> > callback_context); Actually, instead, what I've seen done before that works well: Let BuildableDeviceCallbacks own itself instead. Since one of DidStartDevice() or OnDeviceStartFailed() must be called, and only one call can be made (not two or more), the impl of BuildableDeviceCallbacks could simply self-destruct when either of those methods are called. You would want to make it clear in code comments that calling one of the "done" methods could lead to immediately invalidating the entire object graph visible to the BuildableDevice impl (including itself, |this|). > I still don't think the solution using a |done_cb| is ideal, but I can certainly > live with it. > I'll go add a patch set using a base::OnceClosure |done_cb| as you suggested and > see how it feels. Based on your last e-mail, I'm now a bigger fan of merging the ownership into BuildableDeviceCallbacks. You may even want to rename it to something like DeviceBuildContext. :)
On 2017/03/17 23:11:26, miu wrote: > On 2017/03/17 22:44:53, chfremer wrote: > > But something still bugs me. In our example > > + void CreateAndStartDeviceAsync(const media::VideoCaptureParams& params, > > + BuildableDeviceCallbacks* callbacks, > > + std::unique_ptr<Ownership> > context_reference); > > we had the handle to the callback interface, i.e. |callbacks|, separated from > > the ownership of the required context, i.e. |context_reference|. When using a > > base::OnceClosure |done_cb| for the ownership instead, I find it slightly > > confusing that we now have two somewhat competing handles to make callbacks to > > and that |done_cb| does not actually have to be invoked at all. > > Oh, okay. Agreed. So, let's think how we might incorporate this functionality > into BuildableDeviceCallbacks. OTOH, what if the impl of > BuildableDeviceCallbacks holds the references, and releases them whenever > DidStartDevice() or OnDeviceStartFailed() is called? In other words, I'm not > saying you have to use Closure here: I was focused entirely on replacing the 3rd > argument with it; but, now I see we could merge this functionality into the 2nd > argument. > > To be clear, you can think of DidStartDevice() and OnDeviceStartFailed() as two > possible "done callbacks," only one of which will be called at the end of the > CreateAndStartDeviceAsync() operation. > > > Thinking outloud what general conclusion I can draw from this, it seems a > > |done_cb| works great if there is only a single callback signature. But if we > > have more than one, e.g. all methods in BuildableDeviceCallbacks, a |done_cb| > > does not seem to be ideal. If we wanted to combine a multi-callback-signature > > interface with context ownership into a single object, we could do that by > > making it look like this > > + void CreateAndStartDeviceAsync(const media::VideoCaptureParams& params, > > + std::unique_ptr<BuildableDeviceCallbacks> > > callback_context); > > Actually, instead, what I've seen done before that works well: Let > BuildableDeviceCallbacks own itself instead. Since one of DidStartDevice() or > OnDeviceStartFailed() must be called, and only one call can be made (not two or > more), the impl of BuildableDeviceCallbacks could simply self-destruct when > either of those methods are called. You would want to make it clear in code > comments that calling one of the "done" methods could lead to immediately > invalidating the entire object graph visible to the BuildableDevice impl > (including itself, |this|). I agree. The self-destruct design is not a bad option, but I don't like about it that it requires comments to express its intended usage. I have recently learned that base::OnceClosure has a mechanism of enforcing the Run() method to be called on an rvalue, i.e. my_closure->Run() fails a static_assert, while the correct usage is std::move(my_closure)->Run(). That seems very elegant and it would be nice if we could do something similar for BuildableDeviceCallbacks. I will give it a try on Monday. > > I still don't think the solution using a |done_cb| is ideal, but I can > certainly > > live with it. > > I'll go add a patch set using a base::OnceClosure |done_cb| as you suggested > and > > see how it feels. > > Based on your last e-mail, I'm now a bigger fan of merging the ownership into > BuildableDeviceCallbacks. You may even want to rename it to something like > DeviceBuildContext. :)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
miu@: PTAL For PatchSet 6 I did the following two things. 1.) Use base::OnceCallback instead of content::Ownership to model pure ownership. 2.) For BuildableVideoCaptureDevice::CreateAndStartDeviceAsync() introduce a forwarding class DeviceBuildContext, that combines ownership and callback API and expresses (and enforces at compile-time) that methods DidStartDevice() and OnDeviceStartFailed() require the context to be released. Please let me know how that feels to you as a reader. One thing I noticed while working on this is that the DeviceBuildContext does not resolve the confusion caused by using base::OnceClosure to model pure ownership. It still has a base::OnceClosure member that never get called. Using a self-destructing object would be a valid workaround, but I still don't like how much it relies on non-checked comments as contract. As another idea besides the destructor-only Ownership interface, what we could consider is something like a "base::NeverClosure", i.e. a base::Callback that is expected to be never called. As a result, it would properly express that it models pure ownership. This could be realized as a base::Callback with a new RepeatMode::Never. It would also allow using base::Bind() to build the ownership collections. However, this whole approached seems a bit like forcing a square-shaped object through a circular-shaped hole. I still think the Ownership interface would be more clear. Maybe I should really post a thread on Chromium-dev@ and gather more feedback and ideas. Please let me know what you think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2017/03/20 21:32:25, chfremer wrote: > miu@: PTAL Will take a look shortly... > Using a self-destructing object would be a valid workaround, but I still don't > like how much it relies on non-checked comments as contract. That's why we run ASAN bots on unit tests. This is your check for use-after-free bugs. Also, note that sometimes you simply cannot produce a reasonable code structure to enforce a contract. That's where code comments and code reviewing come in. ;-) For example, already in your change, there is no code structure guaranteeing DidStartDevice() or OnDeviceStartFailed() are ever called, and we're okay with that because we reviewed the InProcessBuildable...Device impl and have proven it to ourselves. > As another idea besides the destructor-only Ownership interface, what we could > consider is something like a "base::NeverClosure", i.e. a base::Callback that is > expected to be never called. As a result, it would properly express that it I guess I'd need to take a look at PS6. It's a little strange to create a Closure without the intention of ever running it. > I still think the Ownership > interface would be more clear. Maybe I should really post a thread on > Chromium-dev@ and gather more feedback and ideas. It sounds like you're still not certain. I'm not sure what else I can say; so yes, chromium-dev@ would be a good idea. Send a quick question e-mail with a link to the ownership.h/.cc from PS5 to solicit some other viewpoints. :)
Comments on PS6: https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:66: callbacks_->OnDeviceStartFailed(controller); This is an interesting way ensure the methods are called on an object "just about to be destroyed." There are probably super-fancy C++ ways to subvert this, but that would set off red flags with any reasonable future code reviewer. So, it seems to work well. :) https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:75: using DeviceBuildContext = DeviceBuildContextT<>; Suggestion: Taking your idea, but re-stating it with IMHO much simpler code. Instead of a separate BuildableDeviceCallbacks and DeviceBuildContextT<T>, what if we just define a single abstract class that acts as the interface, with the special "done" semantics baked-in: class DeviceBuildContext { protected: DeviceBuildContext() = default; DeviceBuildContext(const base::OnceClosure& done_cb) : done_cb_(done_cb) {} DeviceBuildContext(DeviceBulidContext&& context) : done_cb_(std::move(context.done_cb_)) {} virtual ~DeviceBuildContext() { if (!done_cb_.is_null()) done_cb_.Run(); } // Returns false if no descriptor was found. virtual const media::VideoCaptureDeviceDescriptor* LookupDeviceDescriptor( const std::string& id) = 0; virtual void WillStartDevice(media::VideoFacingMode facing_mode) = 0; virtual void DidStartDevice(VideoCaptureController* controller) && = 0; virtual void OnDeviceStartFailed(VideoCaptureController* controller) && = 0; private: // Disallow calling done-condition methods on an L-value instance. This // R-value requirement is to ensure the DeviceBuildContext is about to be // destroyed right after one of the done-condition methods is called. void DidStartDevice(VideoCaptureController* controller) const &; void OnDeviceStartFailed(VideoCaptureController* controller) const &; // Run immediately after one of the done-condition methods is called. base::OnceClosure done_cb_; }; Because they are private, no subclass can override the L-value done methods, and they are invisible to callers. Also, you would not define these "disallowed" methods anywhere: since there cannot be any callers, the linker should not require them to be defined. Also interesting is the |done_cb|: Since DeviceBuildContext is a move-only class, it is guaranteed to be run once at the end, automatically, when the final DeviceBuildContext is destroyed after the DidStartDevice() call. WDYT? https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:67: delete device; After this line: done_cb.Run(); Even though you know it contains a no-op method and is just being used to hold references to ref-counted objects, it's good practice to Run() it anyway: You never know when someone might add code to it in a future change. https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:118: context.LookupDeviceDescriptor(controller->device_id()); |context| is invalid at this point (because you've base::Passed() it on L113 above). ... and throughout the rest of this method... https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:169: base::OnceClosure done_cb) { Need to ensure |done_cb| is Run(). https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:407: base::OnceClosure done_cb) { Run |done_cb| here too. https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:400: base::Bind([](scoped_refptr<VideoCaptureManager>, Nice! I forgot they fixed base::Bind() to allow anonymous C++ functions. :)
Just replies without a new patch set to keep the discussion going. Will post a new patch set when we have settled on a solution. https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:66: callbacks_->OnDeviceStartFailed(controller); On 2017/03/20 23:02:15, miu wrote: > This is an interesting way ensure the methods are called on an object "just > about to be destroyed." There are probably super-fancy C++ ways to subvert this, > but that would set off red flags with any reasonable future code reviewer. So, > it seems to work well. :) Agreed. I had no idea this was possible until I checked how base::OnceCallback is implemented. https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:75: using DeviceBuildContext = DeviceBuildContextT<>; On 2017/03/20 23:02:15, miu wrote: > Suggestion: Taking your idea, but re-stating it with IMHO much simpler code. > Instead of a separate BuildableDeviceCallbacks and DeviceBuildContextT<T>, what > if we just define a single abstract class that acts as the interface, with the > special "done" semantics baked-in: > > class DeviceBuildContext { > protected: > DeviceBuildContext() = default; > DeviceBuildContext(const base::OnceClosure& done_cb) : done_cb_(done_cb) {} > DeviceBuildContext(DeviceBulidContext&& context) > : done_cb_(std::move(context.done_cb_)) {} > > virtual ~DeviceBuildContext() { > if (!done_cb_.is_null()) > done_cb_.Run(); > } > > // Returns false if no descriptor was found. > virtual const media::VideoCaptureDeviceDescriptor* LookupDeviceDescriptor( > const std::string& id) = 0; > > virtual void WillStartDevice(media::VideoFacingMode facing_mode) = 0; > virtual void DidStartDevice(VideoCaptureController* controller) && = 0; > virtual void OnDeviceStartFailed(VideoCaptureController* controller) && = 0; > > private: > // Disallow calling done-condition methods on an L-value instance. This > // R-value requirement is to ensure the DeviceBuildContext is about to be > // destroyed right after one of the done-condition methods is called. > void DidStartDevice(VideoCaptureController* controller) const &; > void OnDeviceStartFailed(VideoCaptureController* controller) const &; > > // Run immediately after one of the done-condition methods is called. > base::OnceClosure done_cb_; > }; > > Because they are private, no subclass can override the L-value done methods, and > they are invisible to callers. Also, you would not define these "disallowed" > methods anywhere: since there cannot be any callers, the linker should not > require them to be defined. That is a very nice idea. I tried something similar when I was experimenting but ran into some issue that may prevent this from working. It seems that the && method qualifier does not play nice with abstract classes. IIUC, an instance of an abstract class has to be a pointer, and when calling a method on a pointer, it is impossible to call on an R-value, because dereferencing a pointer always delivers an L-value, see [1]. [1] http://stackoverflow.com/questions/30093923/function-qualifier-behaviour > Also interesting is the |done_cb|: Since DeviceBuildContext is a move-only > class, it is guaranteed to be run once at the end, automatically, when the final > DeviceBuildContext is destroyed after the DidStartDevice() call. If I read correctly, this is not because DeviceBuildContext is a move-only class, but because we are explicitly calling |done_cb_.Run()| in the destructor. Even though it is nice that we can do that, I don't see why we need to do that if the only job of |done_cb_| is to carry ownership. The owned resources would be freed on destruction of |done_cb_|, and therefore on destruction of DeviceBuildContext, even if we don't call |done_cb_.Run()|. > WDYT? https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:67: delete device; On 2017/03/20 23:02:16, miu wrote: > After this line: > > done_cb.Run(); > > Even though you know it contains a no-op method and is just being used to hold > references to ref-counted objects, it's good practice to Run() it anyway: You > never know when someone might add code to it in a future change. Done. https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:118: context.LookupDeviceDescriptor(controller->device_id()); On 2017/03/20 23:02:15, miu wrote: > |context| is invalid at this point (because you've base::Passed() it on L113 > above). > > ... and throughout the rest of this method... Ouch! Thanks for catching that. What I don't understand is, how this could have passed the unit tests (which I did run before uploading). I am confused now. Will have to check that. Also, it is now beginning to dawn on me, why I may have ended up separating the ownership from the callback interface. In a situation like this, the extra flexibility comes in handy. I don't see any good solutions other than either a.) Separate out the callback methods LookupDeviceDescriptor() and WillStartDevice() into a separate interface that is passed by raw pointer b.) Duplicate the definition of |after_start_capture_callback|, c.) Clean up the design such that the calls to LookupDeviceDescriptor() and WillStartDevice() do not have to happen here. Note: This will happen in one of the subsequent CLs, but merging that into this CL would make the CL (even more) unmanagably large. Do you see any better alternatives? https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:169: base::OnceClosure done_cb) { On 2017/03/20 23:02:16, miu wrote: > Need to ensure |done_cb| is Run(). Done. https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:407: base::OnceClosure done_cb) { On 2017/03/20 23:02:16, miu wrote: > Run |done_cb| here too. Done. https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:400: base::Bind([](scoped_refptr<VideoCaptureManager>, On 2017/03/20 23:02:16, miu wrote: > Nice! I forgot they fixed base::Bind() to allow anonymous C++ functions. :) Acknowledged.
On 2017/03/20 22:17:25, miu wrote: > On 2017/03/20 21:32:25, chfremer wrote: > > miu@: PTAL > > Will take a look shortly... > > > Using a self-destructing object would be a valid workaround, but I still don't > > like how much it relies on non-checked comments as contract. > > That's why we run ASAN bots on unit tests. This is your check for use-after-free > bugs. > > Also, note that sometimes you simply cannot produce a reasonable code structure > to enforce a contract. That's where code comments and code reviewing come in. > ;-) For example, already in your change, there is no code structure guaranteeing > DidStartDevice() or OnDeviceStartFailed() are ever called, and we're okay with > that because we reviewed the InProcessBuildable...Device impl and have proven it > to ourselves. Agreed. I guess there is a trade-off (which I hate :-) ) between what kinds of contracts we can express in C++ and the effort/obscurity of the code needed to do it. In case of the DeviceBuildContextT in Patch Set 6, I actually feel that the cost of the obscurity (the WTFs it induces in a reader not familiar with the idea) may not be worth the benefits of the stricter enforcement it provides. > > As another idea besides the destructor-only Ownership interface, what we could > > consider is something like a "base::NeverClosure", i.e. a base::Callback that > is > > expected to be never called. As a result, it would properly express that it > > I guess I'd need to take a look at PS6. It's a little strange to create a > Closure without the intention of ever running it. > > > I still think the Ownership > > interface would be more clear. Maybe I should really post a thread on > > Chromium-dev@ and gather more feedback and ideas. > > It sounds like you're still not certain. I'm not sure what else I can say; so > yes, chromium-dev@ would be a good idea. Send a quick question e-mail with a > link to the ownership.h/.cc from PS5 to solicit some other viewpoints. :) Will do that. Thanks for the encouragement.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
miu@: PTAL avi@: Please RS content/browser/BUILD.gn In order to be able to move forward with the reviews, I am opting to use base::OnceClosure for now. I asked Chromium-dev@ for input on the options of modeling ownership. We probably do not want to wait for a final outcome before moving along with reviews, so I am proposing to use what we have with Patch Set 7.
lgtm build stamp
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay and, can you plz reupload with a different --similarity ? Now it's hard to parse in_process_buildable_video_capture_device.cc https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:18: class CONTENT_EXPORT BuildableDeviceCallbacks { Why not make this class class BuildableVideoCaptureDevice{ public: class Callbacks { }; ? https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:21: // Returns false if no descriptor was found. s/false/nullptr/? https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:26: virtual void OnDeviceStartFailed(VideoCaptureController* controller) = 0; These two methods are used similarly, so what about s/DidStartDevice/OnDeviceStartSucceed/ and s/OnDeviceStartFailed/OnDeviceStartFailed/ or somesuch? https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:142: const media::VideoCaptureParams& parameters() const { return parameters_; } Heh two things: this class is starting to have way too many methods, and it's a code smell to me that has accessors to so many private methods: it feels like it exposes too much internals.
mcasas@: PTAL https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:18: class CONTENT_EXPORT BuildableDeviceCallbacks { On 2017/03/22 00:33:42, mcasas wrote: > Why not make this class > class BuildableVideoCaptureDevice{ > public: > class Callbacks { > }; > > ? The reason why I generally avoid nesting interfaces like this is that it forces the class implementing the nested interface to know about the outer interface, even when it has no need to know about it. Done. https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:21: // Returns false if no descriptor was found. On 2017/03/22 00:33:42, mcasas wrote: > s/false/nullptr/? Done. https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... content/browser/renderer_host/media/buildable_video_capture_device.h:26: virtual void OnDeviceStartFailed(VideoCaptureController* controller) = 0; On 2017/03/22 00:33:42, mcasas wrote: > These two methods are used similarly, so what about > s/DidStartDevice/OnDeviceStartSucceed/ > and > s/OnDeviceStartFailed/OnDeviceStartFailed/ > or somesuch? We currently have names WillStartDevice() and DidStartDevice() as per miu's suggestion and in line with other places in Chromium, see comments in PatchSet2. Note that I am planning to straighten out the naming in CL18, see https://codereview.chromium.org/2769543002/diff/20001/content/browser/rendere... https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2738763002/diff/160001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:142: const media::VideoCaptureParams& parameters() const { return parameters_; } On 2017/03/22 00:33:42, mcasas wrote: > Heh two things: this class is starting to have way too > many methods, and it's a code smell to me that has > accessors to so many private methods: it feels like it > exposes too much internals. I agree with both observations. However, I would argue that it is still better than what we had before this CL. We got here by first cutting out a large chunk of functionality from the monolithic VideoCaptureManager class and subsequently (in PatchSet 3) merging the class VideoCaptureDeviceEntry into VideoCaptureController, because we felt it would make things simpler. Do you see any way of improving this without doing a full redesign? If not, I would be okay with leaving things as is and consider it as "design issue that has shifted from VideoCaptureManager to VideoCaptureController".
PS8 lgtm % nit: https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:118: context.LookupDeviceDescriptor(controller->device_id()); On 2017/03/20 23:58:26, chfremer wrote: > On 2017/03/20 23:02:15, miu wrote: > > |context| is invalid at this point (because you've base::Passed() it on L113 > > above). > > > > ... and throughout the rest of this method... > > Ouch! Thanks for catching that. > What I don't understand is, how this could have passed the unit tests (which I > did run before uploading). I am confused now. Will have to check that. I'm guessing nothing stomped on the memory of the destroyed object? ASAN bots would catch this use-after-free. > Also, it is now beginning to dawn on me, why I may have ended up separating the > ownership from the callback interface. In a situation like this, the extra > flexibility comes in handy. I don't see any good solutions other than either > a.) Separate out the callback methods LookupDeviceDescriptor() and > WillStartDevice() into a separate interface that is passed by raw pointer > b.) Duplicate the definition of |after_start_capture_callback|, > c.) Clean up the design such that the calls to LookupDeviceDescriptor() and > WillStartDevice() do not have to happen here. Note: This will happen in one of > the subsequent CLs, but merging that into this CL would make the CL (even more) > unmanagably large. > > Do you see any better alternatives? Not at this point. Maybe in the future CLs something will trigger a "light bulb" moment. :) https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:68: std::move(done_cb).Run(); Oh, I mentioned this in the other CL, but here is where it's introduced: Is it necessary to std::move() here (and elsewhere, where done_cb.Run() is the last thing a method does before returning)?
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:145: // Use of Unretained() is safe, because |context_reference| guarantees s/context_reference/done_cb/ ...and elsewhere...
Sending a few comments seeing that miu@ has also commented in this PS. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:124: // Use of Unretained() is safe, because |context_reference| guarantees Seems like |context_reference| doesn't exist around here. (Also in l. 145, 161). https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:141: } This probably predatesthis CL, but I suggest we move this else{} upwards: if (!descriptor) { callbacks->OnDeviceStartFailed(controller); base::ResetAndReturn(&done_cb)->Run(); return; } // and here continue knowing that |descriptor| verifies, l.116. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:204: std::move(done_cb).Run(); Same as in l.139, I'd say here base::ResetAndReturn(&done_cb)->Run(); because it's more explicit what you want to do. Also to consider around all this methods and interfaces is to use ScopedClosureRunner [1] which guarantees in prose and code that the a given closure is either std::move()d or Run(). Keep it in mind for future refactorings ;) [1] https://cs.chromium.org/chromium/src/base/callback_helpers.h?type=cs&q=ResetA..., https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:284: // device is destroyed on the device_task_runner_ and |context_reference| s/device_task_runner_/|device_task_runner_|/ |context_reference| ? https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:400: } nit: no {} for one line bodies. Probably this predates you, but can't harm. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:518: } How is this method supposed to be used? (It isn't used in this CL).
Incorporated suggestions from PatchSet 8. mcasas@: Please let me know if you want to take another pass on this. I want to figure out the test failure on Chromium OS on CL14 anyway before I land this one. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:68: std::move(done_cb).Run(); On 2017/03/22 22:09:50, miu wrote: > Oh, I mentioned this in the other CL, but here is where it's introduced: Is it > necessary to std::move() here (and elsewhere, where done_cb.Run() is the last > thing a method does before returning)? Yes. The OnceCallback must be moved in order to invoke Run(). However, I just learned from mcasas@, that base::ResetAndReturn(&done_cb).Run() now also works on OnceCallbacks (as of Feb 22nd). https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:124: // Use of Unretained() is safe, because |context_reference| guarantees On 2017/03/22 22:43:16, mcasas wrote: > Seems like |context_reference| doesn't exist around here. > (Also in l. 145, 161). Done. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:141: } On 2017/03/22 22:43:16, mcasas wrote: > This probably predatesthis CL, but I suggest we move this else{} > upwards: > > if (!descriptor) { > callbacks->OnDeviceStartFailed(controller); > base::ResetAndReturn(&done_cb)->Run(); > return; > } > // and here continue knowing that |descriptor| verifies, l.116. Done. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:145: // Use of Unretained() is safe, because |context_reference| guarantees On 2017/03/22 22:16:51, miu wrote: > s/context_reference/done_cb/ > > ...and elsewhere... Done. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:204: std::move(done_cb).Run(); On 2017/03/22 22:43:16, mcasas wrote: > Same as in l.139, I'd say here > base::ResetAndReturn(&done_cb)->Run(); > > because it's more explicit what you want to do. > > Also to consider around all this methods and interfaces > is to use ScopedClosureRunner [1] which guarantees in > prose and code that the a given closure is either > std::move()d or Run(). Keep it in mind for future > refactorings ;) > > [1] > https://cs.chromium.org/chromium/src/base/callback_helpers.h?type=cs&q=ResetA..., > Thanks. I didn't know that base::ResetAndReturn() now supports OnceCallbacks (it did not last time I tried, seems it was fixed recently). ScopedClosureRunner currently does not support OnceCallbacks. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:284: // device is destroyed on the device_task_runner_ and |context_reference| On 2017/03/22 22:43:16, mcasas wrote: > s/device_task_runner_/|device_task_runner_|/ > > |context_reference| ? Done. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:400: } On 2017/03/22 22:43:16, mcasas wrote: > nit: no {} for one line bodies. Probably this predates you, > but can't harm. Done. https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:518: } On 2017/03/22 22:43:16, mcasas wrote: > How is this method supposed to be used? > (It isn't used in this CL). Good catch. With the merge of VideoCaptureDeviceEntry into VideoCaptureController, this is now no longer needed. Removed.
On 2017/03/22 23:20:38, chfremer wrote: > Incorporated suggestions from PatchSet 8. > > mcasas@: Please let me know if you want to take another pass on this. I want to > figure out the test failure on Chromium OS on CL14 anyway before I land this > one. > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > File > content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc > (right): > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:68: > std::move(done_cb).Run(); > On 2017/03/22 22:09:50, miu wrote: > > Oh, I mentioned this in the other CL, but here is where it's introduced: Is it > > necessary to std::move() here (and elsewhere, where done_cb.Run() is the last > > thing a method does before returning)? > > Yes. The OnceCallback must be moved in order to invoke Run(). > However, I just learned from mcasas@, that base::ResetAndReturn(&done_cb).Run() > now also works on OnceCallbacks (as of Feb 22nd). > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:124: > // Use of Unretained() is safe, because |context_reference| guarantees > On 2017/03/22 22:43:16, mcasas wrote: > > Seems like |context_reference| doesn't exist around here. > > (Also in l. 145, 161). > > Done. > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:141: > } > On 2017/03/22 22:43:16, mcasas wrote: > > This probably predatesthis CL, but I suggest we move this else{} > > upwards: > > > > if (!descriptor) { > > callbacks->OnDeviceStartFailed(controller); > > base::ResetAndReturn(&done_cb)->Run(); > > return; > > } > > // and here continue knowing that |descriptor| verifies, l.116. > > Done. > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:145: > // Use of Unretained() is safe, because |context_reference| guarantees > On 2017/03/22 22:16:51, miu wrote: > > s/context_reference/done_cb/ > > > > ...and elsewhere... > > Done. > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:204: > std::move(done_cb).Run(); > On 2017/03/22 22:43:16, mcasas wrote: > > Same as in l.139, I'd say here > > base::ResetAndReturn(&done_cb)->Run(); > > > > because it's more explicit what you want to do. > > > > Also to consider around all this methods and interfaces > > is to use ScopedClosureRunner [1] which guarantees in > > prose and code that the a given closure is either > > std::move()d or Run(). Keep it in mind for future > > refactorings ;) > > > > [1] > > > https://cs.chromium.org/chromium/src/base/callback_helpers.h?type=cs&q=ResetA..., > > > > Thanks. I didn't know that base::ResetAndReturn() now supports OnceCallbacks (it > did not last time I tried, seems it was fixed recently). ScopedClosureRunner > currently does not support OnceCallbacks. > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:284: > // device is destroyed on the device_task_runner_ and |context_reference| > On 2017/03/22 22:43:16, mcasas wrote: > > s/device_task_runner_/|device_task_runner_|/ > > > > |context_reference| ? > > Done. > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:400: > } > On 2017/03/22 22:43:16, mcasas wrote: > > nit: no {} for one line bodies. Probably this predates you, > > but can't harm. > > Done. > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > https://codereview.chromium.org/2738763002/diff/180001/content/browser/render... > content/browser/renderer_host/media/video_capture_controller.cc:518: } > On 2017/03/22 22:43:16, mcasas wrote: > > How is this method supposed to be used? > > (It isn't used in this CL). > > Good catch. With the merge of VideoCaptureDeviceEntry into > VideoCaptureController, this is now no longer needed. > > Removed. lgtm to unblock this patch. We should find a better way of having a series of interface/class methods just to forward to an inner class/interface methods in another thread. It's essentially bloating the code and leaking abstractions. base::Bind() is not too friendly to this but perhaps some template magic...?
PS10 and still lgtm
On 2017/03/29 23:03:14, miu wrote: > PS10 and still lgtm Just in case you are wondering why I haven't started landing, I am still trying to resolve an issue with a build bot failure on CL14, see https://codereview.chromium.org/2772963002/. If you like, I could put up CL19 for review in the meantime, but I don't know if it would be a good idea before being able to catch up on landing things.
On 2017/03/29 23:19:37, chfremer wrote: > On 2017/03/29 23:03:14, miu wrote: > > PS10 and still lgtm > > Just in case you are wondering why I haven't started landing, I am still trying > to resolve an issue with a build bot failure on CL14, see > https://codereview.chromium.org/2772963002/. > If you like, I could put up CL19 for review in the meantime, but I don't know if > it would be a good idea before being able to catch up on landing things. No problem. Just wanted to make sure you weren't waiting on me for anything. :)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, avi@chromium.org, mcasas@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2738763002/#ps240001 (title: "Rebase to March 30")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490992632467330,
"parent_rev": "45ad7627ca8532008afad85705e13facbb815ffe", "commit_rev":
"fb141a46523161bc47659b88dd1e76e36a727d6c"}
Message was sent while issue was closed.
Description was changed from ========== [Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice This CL is supposed to be a pure refactoring. There should be no changes to existing behavior. This is part of a series of CLs with the goal of separating the class VideoCaptureManager from knowledge of things that are going to be private implementation details of the video capture service. The end goal is to have VideoCaptureManager talk to an abstraction that can be implemented by either the video capture Mojo service or the existing (legacy) in-process implementation. Changes in this CL: * Add abstraction (interface) BuildableVideoCaptureDevice to be operated by class VideoCaptureManager. Add a callback interface BuildableDeviceCallbacks for BuildableVideoCaptureDevice to send messages back to VideoCaptureManager. * Create an implementation InProcessBuildableVideoCaptureDevice and move the logic for building and releasing devices from VideoCaptureManager into there. * Promote struct VideoCaptureManager::DeviceEntry to class VideoCaptureDeviceEntry and move it to a separate file. * In VideoCaptureManager, changed ownership of DeviceEntries from std::unique_ptr<> to scoped_refptr<>. The reason for this is that VideoCaptureManager wants to temporarily pass ownership of it to an asynchronous operation. Before this CL, this asynchronous operation took shared ownership of the whole VideoCaptureManager instead. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL15. BUG=584797 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== [Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice This CL is supposed to be a pure refactoring. There should be no changes to existing behavior. This is part of a series of CLs with the goal of separating the class VideoCaptureManager from knowledge of things that are going to be private implementation details of the video capture service. The end goal is to have VideoCaptureManager talk to an abstraction that can be implemented by either the video capture Mojo service or the existing (legacy) in-process implementation. Changes in this CL: * Add abstraction (interface) BuildableVideoCaptureDevice to be operated by class VideoCaptureManager. Add a callback interface BuildableDeviceCallbacks for BuildableVideoCaptureDevice to send messages back to VideoCaptureManager. * Create an implementation InProcessBuildableVideoCaptureDevice and move the logic for building and releasing devices from VideoCaptureManager into there. * Promote struct VideoCaptureManager::DeviceEntry to class VideoCaptureDeviceEntry and move it to a separate file. * In VideoCaptureManager, changed ownership of DeviceEntries from std::unique_ptr<> to scoped_refptr<>. The reason for this is that VideoCaptureManager wants to temporarily pass ownership of it to an asynchronous operation. Before this CL, this asynchronous operation took shared ownership of the whole VideoCaptureManager instead. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL15. BUG=584797 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... Review-Url: https://codereview.chromium.org/2738763002 Cr-Commit-Position: refs/heads/master@{#461217} Committed: https://chromium.googlesource.com/chromium/src/+/fb141a46523161bc47659b88dd1e... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/fb141a46523161bc47659b88dd1e... |
