Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697453002:
[DesktopCapturer] FallbackDesktopCapturerWrapper and its tests (Closed)
Description[DesktopCapturer] FallbackDesktopCapturerWrapper and its tests
FallbackDesktopCapturerWrapper is a DesktopCapturer implementation, which owns
two DesktopCapturer implementations. If the main DesktopCapturer fails, it uses
the secondary capturer. The logic is now used in ScreenCapturerWinMagnifier, and
it can also be shared in ScreenCapturerWinDirectx to fallback to Gdi capturer on
privilege prompt or login screen.
BUG=684937
Review-Url: https://codereview.webrtc.org/2697453002
Cr-Commit-Position: refs/heads/master@{#16677}
Committed: https://chromium.googlesource.com/external/webrtc/+/8fefe9889d2e3a8fc781f842ee7cabfdce8efe3f
Patch Set 1 #
Total comments: 11
Patch Set 2 : Resolve review comments #
Total comments: 22
Patch Set 3 : Resolve review comments #
Total comments: 14
Patch Set 4 : Resolve review comments #Patch Set 5 : Resolve review comments #
Messages
Total messages: 53 (42 generated)
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #1 (id:1) has been deleted
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/desktop_capturer_differ_wrapper.h (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/desktop_capturer_differ_wrapper.h:30: private DesktopCapturer::Callback { IIRC, private inheritance is not allowed in Chromium; either make it public or use a member variable (https://google.github.io/styleguide/cppguide.html#Inheritance). TBH, this feels like a legitimate use of private inheritance (a member variable is overkill, yet there's no value in exposing the inheritance to callers), so I'll defer to Sergey on this. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fake_desktop_capturer.cc (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fake_desktop_capturer.cc:20: generator_(nullptr) {} You can remove these initializers since you're now doing it in the header. I don't have a strong opinion either way, but don't use both. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fake_desktop_capturer.h (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fake_desktop_capturer.h:47: // would never be negative. Can you make the return type unsigned to signify this? Also, the function should be called frames_captured (or maybe even num_frames_captured, but please change capture_attempts to num_capture_attempts if you do that) so that it doesn't sound like it should return a bool. Similarly for the member variable, below. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:33: secondary_capturer_->Start(callback); Optional: Is Start ever an expensive operation? If so, it might make sense to defer starting the secondary capturer until it's needed. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:46: const std::vector<std::pair<DesktopCapturer::Result, bool>>& results() const; Optional: This is probably overkill for a test. You can make the members all protected and access them directly from the tests. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/shared_memory.cc (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/shared_memory.cc:42: factory.get())); I don't think this is an acceptable use of unique_ptr. You're storing a bare pointer to it with indefinite scope, so there's no guarantee it will remain valid (the comment in the header is not enough of a guarantee). It looks like you're only using this class in the FakeCapturer, in which case I'd be a lot happier if this class moved there to make it clear that it's only used in tests. I'd be even happier if you could move it there and also find a way to make it comply with the semantics of unique_ptr.
The CQ bit was checked by zijiehe@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.webrtc.org/...
https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/desktop_capturer_differ_wrapper.h (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/desktop_capturer_differ_wrapper.h:30: private DesktopCapturer::Callback { On 2017/02/15 18:43:48, Jamie wrote: > IIRC, private inheritance is not allowed in Chromium; either make it public or > use a member variable > (https://google.github.io/styleguide/cppguide.html#Inheritance). > > TBH, this feels like a legitimate use of private inheritance (a member variable > is overkill, yet there's no value in exposing the inheritance to callers), so > I'll defer to Sergey on this. I have not noticed this guide. Since using private inheritance is actively disallowed, I will undo the change to this file. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fake_desktop_capturer.cc (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fake_desktop_capturer.cc:20: generator_(nullptr) {} On 2017/02/15 18:43:48, Jamie wrote: > You can remove these initializers since you're now doing it in the header. I > don't have a strong opinion either way, but don't use both. I definitely forgot to remove these lines after the change to header file. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fake_desktop_capturer.h (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fake_desktop_capturer.h:47: // would never be negative. On 2017/02/15 18:43:48, Jamie wrote: > Can you make the return type unsigned to signify this? > > Also, the function should be called frames_captured (or maybe even > num_frames_captured, but please change capture_attempts to num_capture_attempts > if you do that) so that it doesn't sound like it should return a bool. Similarly > for the member variable, below. In coding style, https://google.github.io/styleguide/cppguide.html#Integer_Types, unsigned integer types are discouraged. (In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.) Personally I think this should only be applied to for-loop or other indexes. But as long as the guide line states this, I would prefer to follow it. I have updated the name of functions and variables. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:33: secondary_capturer_->Start(callback); On 2017/02/15 18:43:48, Jamie wrote: > Optional: Is Start ever an expensive operation? If so, it might make sense to > defer starting the secondary capturer until it's needed. No, Start() should be an extremely cheap operation, a typical capturer only copies the Callback* pointer in Start() function. https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/shared_memory.cc (right): https://codereview.webrtc.org/2697453002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/shared_memory.cc:42: factory.get())); On 2017/02/15 18:43:49, Jamie wrote: > I don't think this is an acceptable use of unique_ptr. You're storing a bare > pointer to it with indefinite scope, so there's no guarantee it will remain > valid (the comment in the header is not enough of a guarantee). > > It looks like you're only using this class in the FakeCapturer, in which case > I'd be a lot happier if this class moved there to make it clear that it's only > used in tests. I'd be even happier if you could move it there and also find a > way to make it comply with the semantics of unique_ptr. This class is used in FallbackDesktopCapturerWrapper, which needs to share one SharedMemoryFactory instance with two capturers. I may update the API of SharedMemoryFactoryWrapper a little bit to avoid confusion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:43: secondary_capturer_->SetSharedMemoryFactory(shared_memory_factory_->Wrap()); There is a potential threading issue that comes with using the same factory for multiple capturers. Capturers are allowed to use the factory on a separate thread. See the comments for SetSharedMemoryFactory() in desktop_capturer.h . It shouldn't be a problem for the current use-case as all capturers in WebRTC call SharedMemoryFactory on the same thread on which Capture is called (it would be a problem with remoting::DesktopCaptureProxy). But it's worth calling out this limitation in the comments in the header. Also please add DCHECKs in SharedMemoryFactoryWrapper() to verify it never used on multiple threads. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:70: } else { no else after return please. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:88: secondary_capturer_->FocusOnSelectedSource(); Do we need to call both capturers here? https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:100: if (result == Result::ERROR_PERMANENT) { it's possible that OnCaptureResult() was called by the secondary capturer after ERROR_TEMPORARY from the main one. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:103: secondary_capturer_->CaptureFrame(); this may loop indefinitely if secondary_capturer_ keeps returning ERROR_TEMPORARY. I think we need to remember if the current Capture() call is being handled by main or the secondary capturer. Alternatively you could use a separate callback handler for each of the two capturers (which would require a separate class to implement DesktopCapturer::Callback). https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h:36: // DesktopCapturer interface add '.' at the end of the comment https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h:47: // DesktopCapturer::Callback interface end comment with a period https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/shared_memory.cc (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/shared_memory.cc:48: return std::unique_ptr<SharedMemoryFactory>(new SharedMemoryFactoryWrapper( A better solution would be use a ref-counted holder for SharedMemoryFactory, so the shared factory is destroyed only after all "wrappers" are destroyed. SharedDesktopFrame works this way. There is nothing in DesktopCapturer interface to guarantee that capturers stop using this factory when they are destroyed. DesktopCapturer implementations are allowed to capture on a separate thread (see DesktopCaptureProxy). Again this may not be a problem right now, but it should be documented somewhere. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/shared_memory.h (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/shared_memory.h:81: class SharedMemoryFactoryWrapper : public SharedMemoryFactory { Suggest putting this class in fallback_desktop_capturer_wrapper.cc, since it's unlikely to be used anywhere else.
The CQ bit was checked by zijiehe@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.webrtc.org/...
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 zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21563) linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/23130) linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/19823)
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #3 (id:60001) has been deleted
https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:43: secondary_capturer_->SetSharedMemoryFactory(shared_memory_factory_->Wrap()); On 2017/02/15 23:08:43, Sergey Ulanov wrote: > There is a potential threading issue that comes with using the same factory for > multiple capturers. Capturers are allowed to use the factory on a separate > thread. See the comments for SetSharedMemoryFactory() in desktop_capturer.h . It > shouldn't be a problem for the current use-case as all capturers in WebRTC call > SharedMemoryFactory on the same thread on which Capture is called (it would be a > problem with remoting::DesktopCaptureProxy). But it's worth calling out this > limitation in the comments in the header. Also please add DCHECKs in > SharedMemoryFactoryWrapper() to verify it never used on multiple threads. Done. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:70: } else { On 2017/02/15 23:08:43, Sergey Ulanov wrote: > no else after return please. Done. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:88: secondary_capturer_->FocusOnSelectedSource(); On 2017/02/15 23:08:43, Sergey Ulanov wrote: > Do we need to call both capturers here? I think it should be simpler to always forward the functions to the secondary_capturer_. Since we cannot guarantee the impact of each functions, if we do not call the secondary_capturer_, we may need to store all the function calls, and replay them when we start the secondary_capturer_. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:100: if (result == Result::ERROR_PERMANENT) { On 2017/02/15 23:08:43, Sergey Ulanov wrote: > it's possible that OnCaptureResult() was called by the secondary capturer after > ERROR_TEMPORARY from the main one. No, this class does not handle the OnCaptureResult() of the secondary_capturer_. secondary_capturer_ always get the original callback. (line 33) https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:103: secondary_capturer_->CaptureFrame(); On 2017/02/15 23:08:43, Sergey Ulanov wrote: > this may loop indefinitely if secondary_capturer_ keeps returning > ERROR_TEMPORARY. I think we need to remember if the current Capture() call is > being handled by main or the secondary capturer. Alternatively you could use a > separate callback handler for each of the two capturers (which would require a > separate class to implement DesktopCapturer::Callback). Same as the reason above. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h:36: // DesktopCapturer interface On 2017/02/15 23:08:43, Sergey Ulanov wrote: > add '.' at the end of the comment Done. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h:47: // DesktopCapturer::Callback interface On 2017/02/15 23:08:43, Sergey Ulanov wrote: > end comment with a period Done. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/shared_memory.cc (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/shared_memory.cc:48: return std::unique_ptr<SharedMemoryFactory>(new SharedMemoryFactoryWrapper( On 2017/02/15 23:08:43, Sergey Ulanov wrote: > A better solution would be use a ref-counted holder for SharedMemoryFactory, so > the shared factory is destroyed only after all "wrappers" are destroyed. > SharedDesktopFrame works this way. There is nothing in DesktopCapturer interface > to guarantee that capturers stop using this factory when they are destroyed. > DesktopCapturer implementations are allowed to capture on a separate thread (see > DesktopCaptureProxy). Again this may not be a problem right now, but it should > be documented somewhere. I have added comments for both SharedMemoryFactoryWrapper class and CreateSharedMemory() function. Considering this class is becoming complex, I added test cases. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/shared_memory.h (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/shared_memory.h:81: class SharedMemoryFactoryWrapper : public SharedMemoryFactory { On 2017/02/15 23:08:43, Sergey Ulanov wrote: > Suggest putting this class in fallback_desktop_capturer_wrapper.cc, since it's > unlikely to be used anywhere else. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:88: secondary_capturer_->FocusOnSelectedSource(); On 2017/02/16 01:57:07, Hzj_jie wrote: > On 2017/02/15 23:08:43, Sergey Ulanov wrote: > > Do we need to call both capturers here? > > I think it should be simpler to always forward the functions to the > secondary_capturer_. Since we cannot guarantee the impact of each functions, if > we do not call the secondary_capturer_, we may need to store all the function > calls, and replay them when we start the secondary_capturer_. This makes sense fro SelectSource(), but not for FocusOnSelectedSource(). Unlike SelectSource() FocusOnSelectedSource() shouldn't mutate capturer state, so calling it twice makes no sense. If the concern is the second capturer failing then this can be (main_capturer_->FocusOnSelectedSource() || secondary_capturer_->FocusOnSelectedSource()), so the second capturer would be called only if the main one fails. Or just always using secondary_capturer_ would work as well. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:62: secondary_capturer_->Start(callback); Add a comment there to explain how the callbacks are handled and why secondary capturer gets the original callback https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h (right): https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h:53: class SharedMemoryFactoryWrapper : public SharedMemoryFactory { Just declare the class here and move the definition to the .cc file? https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h:53: class SharedMemoryFactoryWrapper : public SharedMemoryFactory { Wrapper normally takes ownership of the wrapped object, which may or may not be the case here, so it's confusing. Maybe call it SharedMemoryFactoryProxy and move owned_factory_ to FallbackDesktopCapturerWrapper. I.e. there will be only one constructor that takes raw SharedMemoryFactory* pointer. That would simplify this class and make it clear that it is a non-owning proxy for SharedMemoryFactory. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc (right): https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:13: #include <map> map<> is not used in this file https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:40: static int kId; nit: Does this need to be a class member? https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:72: FakeDesktopCapturer* secondary_capturer(); Just make the fields protected. Technically it contradicts the style guide, but protected fields are common in tests and I don't think there are any downsides
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #4 (id:100001) has been deleted
https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:88: secondary_capturer_->FocusOnSelectedSource(); On 2017/02/16 19:14:17, Sergey Ulanov wrote: > On 2017/02/16 01:57:07, Hzj_jie wrote: > > On 2017/02/15 23:08:43, Sergey Ulanov wrote: > > > Do we need to call both capturers here? > > > > I think it should be simpler to always forward the functions to the > > secondary_capturer_. Since we cannot guarantee the impact of each functions, > if > > we do not call the secondary_capturer_, we may need to store all the function > > calls, and replay them when we start the secondary_capturer_. > > This makes sense fro SelectSource(), but not for FocusOnSelectedSource(). Unlike > SelectSource() FocusOnSelectedSource() shouldn't mutate capturer state, so > calling it twice makes no sense. If the concern is the second capturer failing > then this can be (main_capturer_->FocusOnSelectedSource() || > secondary_capturer_->FocusOnSelectedSource()), so the second capturer would be > called only if the main one fails. Or just always using secondary_capturer_ > would work as well. Oh, got you. I almost forgot the functionality of this function. I would suggest to change this function into a const to indicate it won't impact the internal state of a DesktopCapturer. I would do it in a separated change, what do you think? https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:62: secondary_capturer_->Start(callback); On 2017/02/16 19:14:17, Sergey Ulanov wrote: > Add a comment there to explain how the callbacks are handled and why secondary > capturer gets the original callback Done. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h (right): https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h:53: class SharedMemoryFactoryWrapper : public SharedMemoryFactory { On 2017/02/16 19:14:17, Sergey Ulanov wrote: > Wrapper normally takes ownership of the wrapped object, which may or may not be > the case here, so it's confusing. Maybe call it SharedMemoryFactoryProxy and > move owned_factory_ to FallbackDesktopCapturerWrapper. I.e. there will be only > one constructor that takes raw SharedMemoryFactory* pointer. That would simplify > this class and make it clear that it is a non-owning proxy for > SharedMemoryFactory. Done. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h:53: class SharedMemoryFactoryWrapper : public SharedMemoryFactory { On 2017/02/16 19:14:18, Sergey Ulanov wrote: > Just declare the class here and move the definition to the .cc file? The reason of putting it here is for test cases: the initial design is to provide a generic implementation of SharedMemoryFactory with proper ownership management. But if we simplify this class to remove the ownership, I am happy to remove the test cases and move this class into cc file. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc (right): https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:13: #include <map> On 2017/02/16 19:14:18, Sergey Ulanov wrote: > map<> is not used in this file Done. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:40: static int kId; On 2017/02/16 19:14:18, Sergey Ulanov wrote: > nit: Does this need to be a class member? Otherwise I need to add a long prefix to indicate it's for FakeSharedMemory. Say, kFakeSharedMemoryId, which, I think, equals to FakeSharedMemory::kId. I do not have a strong opinion. Indeed, I have typed at least 20x times characters here to explain the reason. :) If you think it's important, I am happy to move it out of the class. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:72: FakeDesktopCapturer* secondary_capturer(); On 2017/02/16 19:14:18, Sergey Ulanov wrote: > Just make the fields protected. Technically it contradicts the style guide, but > protected fields are common in tests and I don't think there are any downsides Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm when my comment about kId is addressed. https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:88: secondary_capturer_->FocusOnSelectedSource(); On 2017/02/16 21:31:28, Hzj_jie wrote: > Oh, got you. I almost forgot the functionality of this function. I would suggest > to change this function into a const to indicate it won't impact the internal > state of a DesktopCapturer. I would do it in a separated change, what do you > think? I don't think it needs to be const. E.g. an X11 capturer implementation may initialize X11 connection lazily and making FocusOnSelectedSource() const would prohibit such implementation. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc (right): https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:40: static int kId; On 2017/02/16 21:31:28, Hzj_jie wrote: > On 2017/02/16 19:14:18, Sergey Ulanov wrote: > > nit: Does this need to be a class member? > > Otherwise I need to add a long prefix to indicate it's for FakeSharedMemory. > Say, kFakeSharedMemoryId, which, I think, equals to FakeSharedMemory::kId. I do > not have a strong opinion. Indeed, I have typed at least 20x times characters > here to explain the reason. :) > If you think it's important, I am happy to move it out of the class. Doesn't really matter much, but most other consts in other places are declared non-nested, except when they are shared outside of a single .cc file. But I've just realized that this is not a const. Then it shouldn't use const-style name. Rename it to next_id_?
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22916)
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.webrtc.org/2697453002/#ps140001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc (right): https://codereview.webrtc.org/2697453002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc:88: secondary_capturer_->FocusOnSelectedSource(); On 2017/02/17 20:16:23, Sergey Ulanov wrote: > On 2017/02/16 21:31:28, Hzj_jie wrote: > > Oh, got you. I almost forgot the functionality of this function. I would > suggest > > to change this function into a const to indicate it won't impact the internal > > state of a DesktopCapturer. I would do it in a separated change, what do you > > think? > > I don't think it needs to be const. E.g. an X11 capturer implementation may > initialize X11 connection lazily and making FocusOnSelectedSource() const would > prohibit such implementation. Done. https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc (right): https://codereview.webrtc.org/2697453002/diff/80001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper_unittest.cc:40: static int kId; On 2017/02/17 20:16:23, Sergey Ulanov wrote: > On 2017/02/16 21:31:28, Hzj_jie wrote: > > On 2017/02/16 19:14:18, Sergey Ulanov wrote: > > > nit: Does this need to be a class member? > > > > Otherwise I need to add a long prefix to indicate it's for FakeSharedMemory. > > Say, kFakeSharedMemoryId, which, I think, equals to FakeSharedMemory::kId. I > do > > not have a strong opinion. Indeed, I have typed at least 20x times characters > > here to explain the reason. :) > > If you think it's important, I am happy to move it out of the class. > > Doesn't really matter much, but most other consts in other places are declared > non-nested, except when they are shared outside of a single .cc file. > But I've just realized that this is not a const. Then it shouldn't use > const-style name. Rename it to next_id_? Done.
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487368508017530,
"parent_rev": "4ef903d3db47e854b3290c35eb6f64ee1558e7d2", "commit_rev":
"8fefe9889d2e3a8fc781f842ee7cabfdce8efe3f"}
Message was sent while issue was closed.
Description was changed from ========== [DesktopCapturer] FallbackDesktopCapturerWrapper and its tests FallbackDesktopCapturerWrapper is a DesktopCapturer implementation, which owns two DesktopCapturer implementations. If the main DesktopCapturer fails, it uses the secondary capturer. The logic is now used in ScreenCapturerWinMagnifier, and it can also be shared in ScreenCapturerWinDirectx to fallback to Gdi capturer on privilege prompt or login screen. BUG=684937 ========== to ========== [DesktopCapturer] FallbackDesktopCapturerWrapper and its tests FallbackDesktopCapturerWrapper is a DesktopCapturer implementation, which owns two DesktopCapturer implementations. If the main DesktopCapturer fails, it uses the secondary capturer. The logic is now used in ScreenCapturerWinMagnifier, and it can also be shared in ScreenCapturerWinDirectx to fallback to Gdi capturer on privilege prompt or login screen. BUG=684937 Review-Url: https://codereview.webrtc.org/2697453002 Cr-Commit-Position: refs/heads/master@{#16677} Committed: https://chromium.googlesource.com/external/webrtc/+/8fefe9889d2e3a8fc781f842e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/8fefe9889d2e3a8fc781f842e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
