Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885653002:
[Mojo Video Capture] Do not instantiate in-process VideoCaptureSystem when using service (Closed)
|
Created:
3 years, 7 months ago by chfremer Modified:
3 years, 7 months ago CC:
chromium-reviews, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, xjz+watch_chromium.org, mfoltz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Mojo Video Capture] Do not instantiate in-process VideoCaptureSystem when using service
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL25_cleanup1.
In MediaStreamManager, we would instantiate a VideoCaptureSystem instance that
would never be used when the video capture service is enabled. This CL cleans
this up by allowing instances of InProcessVideoCaptureProvider to operate
without a VideoCaptureSystem and, in that case, only support non-device capture.
BUG=584797
TEST=
content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
Review-Url: https://codereview.chromium.org/2885653002
Cr-Commit-Position: refs/heads/master@{#474031}
Committed: https://chromium.googlesource.com/chromium/src/+/5b1950b384ac25a537dd95f8f7fb62900bf192f7
Patch Set 1 #
Total comments: 8
Patch Set 2 : Added a DCHECK #Patch Set 3 : Remove |optional_| prefix #
Total comments: 12
Patch Set 4 : Incorporate suggestions from PatchSet 3 #
Total comments: 2
Patch Set 5 : Incorporate feedback from Patch Set 4 #Patch Set 6 : Use SEQUENCE_CHECKER macros and Rebase to May 19th #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 31 (19 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
chfremer@chromium.org changed reviewers: + emircan@chromium.org, mcasas@chromium.org
mcasas@: PTAL emircan@: PTAL media_stream_manager
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_...)
https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc:201: } If we want to run |done_cb| in every possible return path, consider adding in l.167-ish base::ScopedClosureRunner scoped_callback(done_cb); which will call Run() on destruction. https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc:213: optional_video_capture_system_->CreateDevice(device_id); If it's optional, we should check if (optional_video_capture_system_), right? https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_device_launcher.h (right): https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.h:75: media::VideoCaptureSystem* const optional_video_capture_system_; I think having a naked pointer here is enough to imply that it can be nullptr, so the prefix optional_ is unnecessary. Also, if you really want to convey the optionality, consider base::Optional which in its uses [1] states: > A very common use case is for classes and structures > that have an object not always available [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
PTAL https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc:201: } On 2017/05/16 22:22:22, mcasas wrote: > If we want to run |done_cb| in every possible return path, > consider adding in l.167-ish > > base::ScopedClosureRunner scoped_callback(done_cb); > > which will call Run() on destruction. Agreed that this would be better and cleaner. Unfortunately it looks like ScopedClosureRunner does not (yet) support move-only types like OnceClosure. https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc:213: optional_video_capture_system_->CreateDevice(device_id); On 2017/05/16 22:22:22, mcasas wrote: > If it's optional, we should check if (optional_video_capture_system_), > right? True. There is already a check in LaunchDeviceAsync() that should make it so that this method here never gets called when |optional_video_capture_system_| is nullptr. I'll add a DCHECK here to make sure. https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_device_launcher.h (right): https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.h:75: media::VideoCaptureSystem* const optional_video_capture_system_; On 2017/05/16 22:22:22, mcasas wrote: > I think having a naked pointer here is enough > to imply that it can be nullptr, so the prefix > optional_ is unnecessary. Also, if you really want > to convey the optionality, consider base::Optional > which in its uses [1] states: > > A very common use case is for classes and structures > > that have an object not always available > > > [1] > https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When... I disagree with the idea that the raw pointer would be enough to suggest it is optional, since there are many cases where a raw pointer is used for non-optional non-owned references. I feel that the use of base::Optional<> is a bit heavyweight for raw pointer types (since they already allow nullptr as a value). And I feel that using the variable name conveys the optionality much more strongly than the type, because the variable name appears explicitly at every usage site. After this consideration, I still feel the proposed state is the best option. If you still prefer one of the other two options, let me know and I'll make the change.
mcasas@chromium.org changed reviewers: + miu@chromium.org
https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_device_launcher.h (right): https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.h:75: media::VideoCaptureSystem* const optional_video_capture_system_; On 2017/05/16 22:45:02, chfremer wrote: > On 2017/05/16 22:22:22, mcasas wrote: > > I think having a naked pointer here is enough > > to imply that it can be nullptr, so the prefix > > optional_ is unnecessary. Also, if you really want > > to convey the optionality, consider base::Optional > > which in its uses [1] states: > > > A very common use case is for classes and structures > > > that have an object not always available > > > > > > [1] > > > https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When... > > I disagree with the idea that the raw pointer would be enough to suggest it is > optional, since there are many cases where a raw pointer is used for > non-optional non-owned references. > > I feel that the use of base::Optional<> is a bit heavyweight for raw pointer > types (since they already allow nullptr as a value). And I feel that using the > variable name conveys the optionality much more strongly than the type, because > the variable name appears explicitly at every usage site. After this > consideration, I still feel the proposed state is the best option. If you still > prefer one of the other two options, let me know and I'll make the change. A quick search [1] yields no |optional_| members in content/, so I'd say, non-owned raw pointers should be checked for non-nullness. miu@ WDYT? [1] grep -rn "optional_" content/ | grep "\.h"
mcasas@: PTAL Removed the "optional_" prefix for PatchSet 3 https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_device_launcher.h (right): https://codereview.chromium.org/2885653002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.h:75: media::VideoCaptureSystem* const optional_video_capture_system_; On 2017/05/17 23:19:03, mcasas wrote: > On 2017/05/16 22:45:02, chfremer wrote: > > On 2017/05/16 22:22:22, mcasas wrote: > > > I think having a naked pointer here is enough > > > to imply that it can be nullptr, so the prefix > > > optional_ is unnecessary. Also, if you really want > > > to convey the optionality, consider base::Optional > > > which in its uses [1] states: > > > > A very common use case is for classes and structures > > > > that have an object not always available > > > > > > > > > [1] > > > > > > https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When... > > > > I disagree with the idea that the raw pointer would be enough to suggest it is > > optional, since there are many cases where a raw pointer is used for > > non-optional non-owned references. > > > > I feel that the use of base::Optional<> is a bit heavyweight for raw pointer > > types (since they already allow nullptr as a value). And I feel that using the > > variable name conveys the optionality much more strongly than the type, > because > > the variable name appears explicitly at every usage site. After this > > consideration, I still feel the proposed state is the best option. If you > still > > prefer one of the other two options, let me know and I'll make the change. > > A quick search [1] yields no |optional_| members in content/, > so I'd say, non-owned raw pointers should be checked for > non-nullness. miu@ WDYT? > > > [1] grep -rn "optional_" content/ | grep "\.h" It appears that my reasoning for why I feel that adding the "optional_" prefix is a good idea did not convince you, so I am removing the prefix. The fact that there are no |optional_*| members in content/ does not change my opinion that adding it here would have been a (minor) improvement, though.
emircan@: PTAL (need you sign-off for media_stream_manager.cc, since you are an OWNER)
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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_provider.cc (right): https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.cc:30: std::unique_ptr<media::VideoCaptureSystem> optional_video_capture_system, in l.29 of the .h, this parameter is called |video_capture_system|, but here is called |optional_...|, which is misleading, since then there's no difference with l.21, and you could rearrange this to be a single factory called: std::unique_ptr<VideoCaptureProvider> InProcessVideoCaptureProvider::CreateInstance( scoped_refptr<base::SingleThreadTaskRunner> device_task_runner, std::unique_ptr<media::VideoCaptureSystem> video_capture_system = nullptr); or similar, correct? https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_provider.h (right): https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. This class was landed in 2017: https://chromium.googlesource.com/chromium/src/+log/eb9bfd648536ce835bb620265... https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:15: class CONTENT_EXPORT InProcessVideoCaptureProvider nit: CONTENT_EXPORT might not be needed since all its usage is from inside content (same .dll) -- but only maybe, because of video_capture_manager_unittest.cc https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:41: optional_video_capture_system_; With the same rationale as in the other comments, I think the |optional_| prefix here is superfluous since this unique ptr can perfectly be not initialized to anything meaningful, as proven by the lack of DCHECK()s and the test before use in l.40. The absence of the DCHECK is not trivial: you should strive to document by using pre-post conditions enforced by the compiler and not (only) by naming conventions. OTOH there's plenty of examples of member unique_ptrs that are tested-before-use and it seems an established pattern versus the prefix, which is not present in content/. FTR I'd say it's appropriate to add a comment saying that this member can be null and in that case you fall back to whatever. With that, both the casual reader would gets the information, and the compiler will enforce your preconditions. https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:43: scoped_refptr<base::SingleThreadTaskRunner> device_task_runner_; nit: const? https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:44: }; This class needs a thread checker.
mcasas@: PTAL https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_provider.cc (right): https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.cc:30: std::unique_ptr<media::VideoCaptureSystem> optional_video_capture_system, On 2017/05/18 17:42:12, mcasas wrote: > in l.29 of the .h, this parameter is called |video_capture_system|, > but here is called |optional_...|, which is misleading, since then > there's no difference with l.21, and you could rearrange this to > be a single factory called: > > std::unique_ptr<VideoCaptureProvider> > InProcessVideoCaptureProvider::CreateInstance( > scoped_refptr<base::SingleThreadTaskRunner> device_task_runner, > std::unique_ptr<media::VideoCaptureSystem> video_capture_system = nullptr); > > or similar, correct? Correct, thanks for catching this! I forgot to remove the "optional_" prefix in this class. I guess, technically, we could just use a public constructor like InProcessVideoCaptureProvider( scoped_refptr<base::SingleThreadTaskRunner> device_task_runner, std::unique_ptr<media::VideoCaptureSystem> video_capture_system = nullptr); The sole reason I went for the (rather verbose) two factory methods, is to be able to convey intent via their method name. Please let me know if you would prefer the public constructor. Done. https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_provider.h (right): https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/18 17:42:12, mcasas wrote: > This class was landed in 2017: > > https://chromium.googlesource.com/chromium/src/+log/eb9bfd648536ce835bb620265... Done. https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:15: class CONTENT_EXPORT InProcessVideoCaptureProvider On 2017/05/18 17:42:12, mcasas wrote: > nit: CONTENT_EXPORT might not be needed since all > its usage is from inside content (same .dll) -- but only > maybe, because of video_capture_manager_unittest.cc True. Just tried, but turns out you were right about content_unittests requiring the export. https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:41: optional_video_capture_system_; On 2017/05/18 17:42:12, mcasas wrote: > With the same rationale as in the other comments, I think > the |optional_| prefix here is superfluous since this unique ptr > can perfectly be not initialized to anything meaningful, as proven > by the lack of DCHECK()s and the test before use in l.40. > > The absence of the DCHECK is not trivial: you should strive > to document by using pre-post conditions enforced by the > compiler and not (only) by naming conventions. OTOH there's > plenty of examples of member unique_ptrs that are > tested-before-use and it seems an established pattern versus > the prefix, which is not present in content/. A DCHECK only enforces at runtime, though. At "read-time" the reader still has to go and find the DCHECKs in the implementation to find out whether or not a member is optional. > FTR I'd say it's appropriate to add a comment saying that this > member can be null and in that case you fall back to whatever. > With that, both the casual reader would gets the information, > and the compiler will enforce your preconditions. Agreed that it can be done this way. And I forgot to remove the "optional_" prefix here. Done. https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:43: scoped_refptr<base::SingleThreadTaskRunner> device_task_runner_; On 2017/05/18 17:42:12, mcasas wrote: > nit: const? Done. https://codereview.chromium.org/2885653002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_provider.h:44: }; On 2017/05/18 17:42:12, mcasas wrote: > This class needs a thread checker. Done.
lgtm % nit. https://codereview.chromium.org/2885653002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2885653002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc:96: callbacks->OnDeviceLaunchFailed(); This becomes the second caller of OnDeviceLaunchFailed(). I think you should add some logs to explain why it failed to distinguish between failures.
https://codereview.chromium.org/2885653002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2885653002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc:96: callbacks->OnDeviceLaunchFailed(); On 2017/05/18 18:40:16, emircan wrote: > This becomes the second caller of OnDeviceLaunchFailed(). I think you should add > some logs to explain why it failed to distinguish between failures. Good point. I realized that this code path is actually expected to never be reached, so instead of adding the logging, I am proposing to use a NOTREACHED here. 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.
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 Link to the patchset: https://codereview.chromium.org/2885653002/#ps120001 (title: "Use SEQUENCE_CHECKER macros and Rebase to May 19th")
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": 120001, "attempt_start_ts": 1495564879525560,
"parent_rev": "7b779900fe4ae45ab5217311904203e2d71a0885", "commit_rev":
"5b1950b384ac25a537dd95f8f7fb62900bf192f7"}
Message was sent while issue was closed.
Description was changed from ========== [Mojo Video Capture] Do not instantiate in-process VideoCaptureSystem when using service This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL25_cleanup1. In MediaStreamManager, we would instantiate a VideoCaptureSystem instance that would never be used when the video capture service is enabled. This CL cleans this up by allowing instances of InProcessVideoCaptureProvider to operate without a VideoCaptureSystem and, in that case, only support non-device capture. BUG=584797 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== [Mojo Video Capture] Do not instantiate in-process VideoCaptureSystem when using service This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL25_cleanup1. In MediaStreamManager, we would instantiate a VideoCaptureSystem instance that would never be used when the video capture service is enabled. This CL cleans this up by allowing instances of InProcessVideoCaptureProvider to operate without a VideoCaptureSystem and, in that case, only support non-device capture. BUG=584797 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... Review-Url: https://codereview.chromium.org/2885653002 Cr-Commit-Position: refs/heads/master@{#474031} Committed: https://chromium.googlesource.com/chromium/src/+/5b1950b384ac25a537dd95f8f7fb... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/5b1950b384ac25a537dd95f8f7fb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
