Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2673373003:
getUserMeida: report device starting states (Closed)
|
Created:
3 years, 10 months ago by braveyao Modified:
3 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, viettrungluu+watch_chromium.org, qsr+mojo_chromium.org, Aaron Boodman, posciak+watch_chromium.org, chfremer+watch_chromium.org, mac-reviews_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, agrieve+watch_chromium.org, xjz+watch_chromium.org, darin (slow to review), miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptiongetUserMeida: report device starting states
Chromium used to report device started immediately at requesting user media,
despite the device operation results. This causes several kinds of problems.
- Notification of capture starting is shown before device starts.
- gUM gets successful callback even if the device fails to start.
So we decide to make gUM return the real device starting states
(here we focus on the video capturing).
This is the first part of the complete modification.
The design doc is here, https://goo.gl/WTZkEl
The complete CL is here, https://codereview.chromium.org/2658643002
This CL implements the device-started-state reporting part. Each device will
report OnStarted event all the way back to VideoCaptureImpl when it's started
successfully(Error report has been done already).
This CL also includes all the necessary modifications due to the interface
change.
This CL will take no effect to getUserMedia behavior at present until another
CL which will handle the OnStarted event is landed.
BUG=674959, 651910
Review-Url: https://codereview.chromium.org/2673373003
Cr-Commit-Position: refs/heads/master@{#453388}
Committed: https://chromium.googlesource.com/chromium/src/+/a18b95669ed38c0931818534ce17f131a006e476
Patch Set 1 #
Total comments: 34
Patch Set 2 : rebase to Feb 10 #Patch Set 3 : address comments on PS#1 #
Total comments: 4
Patch Set 4 : address comments on PS#3 #
Total comments: 20
Patch Set 5 : rebase to Feb16 #Patch Set 6 : address comments on PS#4 and revise unittests #
Total comments: 10
Patch Set 7 : address comments on PS#6 and restore added state check in frame callback. #
Total comments: 10
Patch Set 8 : address comments on PS#7 #
Total comments: 17
Patch Set 9 : rebase to Feb27 #Patch Set 10 : address comments on PS#8 #Messages
Total messages: 143 (108 generated)
The CQ bit was checked by braveyao@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.
braveyao@chromium.org changed reviewers: + chfremer@chromium.org, miu@chromium.org
Hi, this cl is to report starting states of video capture devices to VideoCaptureImpl. Another cl will be uploaded to handle this OnStart event after this one. These two together can make getUserMedia return the synced user media request result. Many files are touched, with limited code change though. miu@, please help to review the changes in content/browser/media/capture and media/capture/content. I wonder if you are OK to take a look to media/capture/video too? chfremer@, please help to review the changes in content/browser/renderer_host/media and services/video_capture. Thanks a lot!
PS1 comments. I took a look at content/browser/media/capture/* and media/capture/*. There seem to be a lot of impls that you did not change to make sure they call OnStarted(). Please make sure to check for them all: ~/chromium/src$ git grep -l -P '(public|protected|private) (media::)?VideoCaptureDevice' content/browser/media/capture/desktop_capture_device.h content/browser/media/capture/desktop_capture_device_aura.h content/browser/media/capture/desktop_capture_device_aura_unittest.cc content/browser/media/capture/desktop_capture_device_unittest.cc content/browser/media/capture/screen_capture_device_android.h content/browser/media/capture/screen_capture_device_android_unittest.cc content/browser/media/capture/web_contents_video_capture_device.h content/browser/media/capture/web_contents_video_capture_device_unittest.cc content/browser/renderer_host/media/video_capture_manager_unittest.cc media/capture/video/android/video_capture_device_android.h media/capture/video/android/video_capture_device_factory_android.h media/capture/video/fake_video_capture_device.h media/capture/video/fake_video_capture_device_factory.h media/capture/video/fake_video_capture_device_unittest.cc media/capture/video/file_video_capture_device.h media/capture/video/file_video_capture_device_factory.h media/capture/video/linux/v4l2_capture_delegate_unittest.cc media/capture/video/linux/video_capture_device_chromeos.h media/capture/video/linux/video_capture_device_factory_linux.h media/capture/video/linux/video_capture_device_linux.h media/capture/video/mac/video_capture_device_decklink_mac.h media/capture/video/mac/video_capture_device_factory_mac.h media/capture/video/mac/video_capture_device_mac.h media/capture/video/video_capture_device_client.cc media/capture/video/video_capture_device_client.h media/capture/video/video_capture_device_unittest.cc media/capture/video/win/video_capture_device_factory_win.h media/capture/video/win/video_capture_device_mf_win.h media/capture/video/win/video_capture_device_win.h services/video_capture/test/mock_device_factory.cc services/video_capture/test/mock_device_factory.h services/video_capture/test/mock_device_test.h https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/desktop_capture_device_aura_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/desktop_capture_device_aura_unittest.cc:59: MOCK_METHOD0(OnStarted, void(void)); One or more unit tests should probably EXPECT_CALL(OnStarted), right? https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/desktop_capture_device_aura_unittest.cc:59: MOCK_METHOD0(OnStarted, void(void)); Where does DesktopCaptureDeviceAura call OnStarted()? https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/desktop_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/desktop_capture_device_unittest.cc:78: MOCK_METHOD0(OnStarted, void(void)); ditto: One or more unit tests should EXPECT_CALL(OnStarted()). https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/screen_capture_device_android_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/screen_capture_device_android_unittest.cc:36: MOCK_METHOD0(OnStarted, void(void)); ditto: EXPECT_CALL https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:325: void OnStarted() override {} Where does WebContentsVideoCaptureDevice call OnStarted()? (It seems to be missing from this patch.) https://codereview.chromium.org/2673373003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/2673373003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:265: // report back device started state. Maybe add: However, if the user-prompt failed to show, report a failed start immediately. https://codereview.chromium.org/2673373003/diff/1/media/capture/content/scree... File media/capture/content/screen_capture_device_core.cc (right): https://codereview.chromium.org/2673373003/diff/1/media/capture/content/scree... media/capture/content/screen_capture_device_core.cc:128: else if (oracle_proxy_.get()) nit: Shouldn't need the ".get()". That's a legacy requirement on the smart pointers that they fixed a while ago. https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.h:218: // VideoCaptureDevice reports it's successfully started. There are two things you'll want to consider here: 1. Any thread can call OnStarted() (at least, FWICT in this patch). Consider whether this is acceptable, or that OnStarted() must be called on the same thread that called AllocateAndStart(). 2. OnStarted() may be called synchronously (i.e., within a call to AllocateAndStart()), or asynchronously. Are your client impl's prepared for that? Regardless of whether you decide to change (or not change) anything here, please document these behaviors in the method comments here.
Looks good overall. My only concern is about tightening the contract a little bit to ensure we have a clear specification of what is expected to happen in which order. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_host.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_host.cc:125: BrowserThread::IO, FROM_HERE, Why is it necessary to schedule this to the end of the current message queue instead of invoking device_id_to_observer_map_[controller_id]->OnStateChanged() directly? I see that this is also done for OnError() and OnEnded(), but not for the other methods. But I couldn't find any documentation explaining why this is done. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:251: .Times(AtMost(1)); Do we really want to modify the contract between VideoCaptureHost and VideoCaptureObserver such that it is legal for VideoCaptureHost to report STOPPED without first reporting STARTED? As a user of the interface that would feel unexpected to me. If a stop request can cancel a start request before it has completed, could we make it that in that case neither STARTED nor STOPPED is reported? I would prefer that we guarantee that we get either a) STARTED followed by STOPPED or b) no state changes at all. https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.h:218: // VideoCaptureDevice reports it's successfully started. On 2017/02/09 22:21:49, miu wrote: > There are two things you'll want to consider here: > > 1. Any thread can call OnStarted() (at least, FWICT in this patch). Consider > whether this is acceptable, or that OnStarted() must be called on the same > thread that called AllocateAndStart(). > > 2. OnStarted() may be called synchronously (i.e., within a call to > AllocateAndStart()), or asynchronously. Are your client impl's prepared for > that? > > Regardless of whether you decide to change (or not change) anything here, please > document these behaviors in the method comments here. Regardless of which exact thread it comes from, we should guarantee two things: 1.) OnStarted() is not called concurrently with any of the other methods (actually, none of the VideoCaptureDevice::Client methods should be called concurrently) 2.) OnStarted() is guaranteed to get invoked before the first frame is delivered.
The CQ bit was checked by braveyao@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by braveyao@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 braveyao@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...
Thanks so much for your comments! All addressed/responded. PTAL! miu@, all the device implementations have called OnStarted() as I believe, double checked. Please let me know if I still missed any case. (Also retested with all getUserMedia cases with the full workable cl on all platforms, https://codereview.chromium.org/2658643002/#) chfremer@, I may have some opinions different than yours as in the replies. Please let me know they can't answer your concerns. https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/desktop_capture_device_aura_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/desktop_capture_device_aura_unittest.cc:59: MOCK_METHOD0(OnStarted, void(void)); On 2017/02/09 22:21:49, miu wrote: > Where does DesktopCaptureDeviceAura call OnStarted()? It's called in ScreenCaptureDeviceCore::CaptureStarted, https://codereview.chromium.org/2673373003/diff/20001/media/capture/content/s.... In the past we only forward the ERROR report. Now Success report will be forwarded to Client too. https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/desktop_capture_device_aura_unittest.cc:59: MOCK_METHOD0(OnStarted, void(void)); On 2017/02/09 22:21:49, miu wrote: > One or more unit tests should probably EXPECT_CALL(OnStarted), right? Done. Thanks for the suggestion! https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/desktop_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/desktop_capture_device_unittest.cc:78: MOCK_METHOD0(OnStarted, void(void)); On 2017/02/09 22:21:49, miu wrote: > ditto: One or more unit tests should EXPECT_CALL(OnStarted()). Done. https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/screen_capture_device_android_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/screen_capture_device_android_unittest.cc:36: MOCK_METHOD0(OnStarted, void(void)); On 2017/02/09 22:21:49, miu wrote: > ditto: EXPECT_CALL Done. https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:325: void OnStarted() override {} On 2017/02/09 22:21:49, miu wrote: > Where does WebContentsVideoCaptureDevice call OnStarted()? (It seems to be > missing from this patch.) It's called in ScreenCaptureDeviceCore::CaptureStarted, https://codereview.chromium.org/2673373003/diff/20001/media/capture/content/s.... Same to Aura and Tab capture. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_host.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_host.cc:125: BrowserThread::IO, FROM_HERE, On 2017/02/09 23:34:52, chfremer wrote: > Why is it necessary to schedule this to the end of the current message queue > instead of invoking device_id_to_observer_map_[controller_id]->OnStateChanged() > directly? > > I see that this is also done for OnError() and OnEnded(), but not for the other > methods. > But I couldn't find any documentation explaining why this is done. The EventHandler methods used to call "PostTask" before. And since cl, https://codereview.chromium.org/1017503002, some of them are switched to IPC message methods and then Mojo(I suppose). I guess they will all be unified eventually in Mojo? https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:251: .Times(AtMost(1)); On 2017/02/09 23:34:52, chfremer wrote: > Do we really want to modify the contract between VideoCaptureHost and > VideoCaptureObserver such that it is legal for VideoCaptureHost to report > STOPPED without first reporting STARTED? > As a user of the interface that would feel unexpected to me. > If a stop request can cancel a start request before it has completed, could we > make it that in that case neither STARTED nor STOPPED is reported? > > I would prefer that we guarantee that we get either a) STARTED followed by > STOPPED or b) no state changes at all. It's natural to me that a stop request can cancel a start request before it has completed. And at this point, only the STOPPED report will be concerned, as the STARTED is not important any more. WDTY? https://codereview.chromium.org/2673373003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/2673373003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:265: // report back device started state. On 2017/02/09 22:21:49, miu wrote: > Maybe add: However, if the user-prompt failed to show, report a failed start > immediately. Done. https://codereview.chromium.org/2673373003/diff/1/media/capture/content/scree... File media/capture/content/screen_capture_device_core.cc (right): https://codereview.chromium.org/2673373003/diff/1/media/capture/content/scree... media/capture/content/screen_capture_device_core.cc:128: else if (oracle_proxy_.get()) On 2017/02/09 22:21:49, miu wrote: > nit: Shouldn't need the ".get()". That's a legacy requirement on the smart > pointers that they fixed a while ago. Done. https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.h:218: // VideoCaptureDevice reports it's successfully started. On 2017/02/09 23:34:52, chfremer wrote: > On 2017/02/09 22:21:49, miu wrote: > > There are two things you'll want to consider here: > > > > 1. Any thread can call OnStarted() (at least, FWICT in this patch). Consider > > whether this is acceptable, or that OnStarted() must be called on the same > > thread that called AllocateAndStart(). > > > > 2. OnStarted() may be called synchronously (i.e., within a call to > > AllocateAndStart()), or asynchronously. Are your client impl's prepared for > > that? > > > > Regardless of whether you decide to change (or not change) anything here, > please > > document these behaviors in the method comments here. > > Regardless of which exact thread it comes from, we should guarantee two things: > 1.) OnStarted() is not called concurrently with any of the other methods > (actually, none of the VideoCaptureDevice::Client methods should be called > concurrently) > 2.) OnStarted() is guaranteed to get invoked before the first frame is > delivered. All the methods will be posted to IO thread (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...). So there won't be concurrent calls. The state machine will guarantee that the first frame won't be processed until OnStarted is received. (see https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp...) Please let me know if you have more concerns. https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.h:218: // VideoCaptureDevice reports it's successfully started. On 2017/02/09 22:21:49, miu wrote: > There are two things you'll want to consider here: > > 1. Any thread can call OnStarted() (at least, FWICT in this patch). Consider > whether this is acceptable, or that OnStarted() must be called on the same > thread that called AllocateAndStart(). > > 2. OnStarted() may be called synchronously (i.e., within a call to > AllocateAndStart()), or asynchronously. Are your client impl's prepared for > that? > > Regardless of whether you decide to change (or not change) anything here, please > document these behaviors in the method comments here. All the events will be posted to IO thread. See https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide.... Please let me know if there is any more concern.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
A few more specific recommendations regarding the interface contracts. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_host.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_host.cc:125: BrowserThread::IO, FROM_HERE, On 2017/02/14 01:05:49, braveyao wrote: > On 2017/02/09 23:34:52, chfremer wrote: > > Why is it necessary to schedule this to the end of the current message queue > > instead of invoking > device_id_to_observer_map_[controller_id]->OnStateChanged() > > directly? > > > > I see that this is also done for OnError() and OnEnded(), but not for the > other > > methods. > > But I couldn't find any documentation explaining why this is done. > > The EventHandler methods used to call "PostTask" before. And since cl, > https://codereview.chromium.org/1017503002, some of them are switched to IPC > message methods and then Mojo(I suppose). > I guess they will all be unified eventually in Mojo? Thanks for the reference to that CL. It looks like mcasas@ removed the PostTask for some of the methods and left it in place for others. The question is why is the PostTask needed for some but not all of the methods? I feel that if we are adding a new method OnStarted() we need to be able to answer this in order to decide whether or not PostTask needs to be used in OnStarted(). https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:251: .Times(AtMost(1)); On 2017/02/14 01:05:49, braveyao wrote: > On 2017/02/09 23:34:52, chfremer wrote: > > Do we really want to modify the contract between VideoCaptureHost and > > VideoCaptureObserver such that it is legal for VideoCaptureHost to report > > STOPPED without first reporting STARTED? > > As a user of the interface that would feel unexpected to me. > > If a stop request can cancel a start request before it has completed, could we > > make it that in that case neither STARTED nor STOPPED is reported? > > > > I would prefer that we guarantee that we get either a) STARTED followed by > > STOPPED or b) no state changes at all. > > It's natural to me that a stop request can cancel a start request before it has > completed. And at this point, only the STOPPED report will be concerned, as the > STARTED is not important any more. > WDTY? I think it is confusing for the listener when a STOPPED event can arrive without any preceding STARTED event. If starting is aborted without any visible effect for the listener, there is no reason to send any notification to the listener, not even a STOPPED. If we do want to notify listeners about the aborted attemt to start, we should send a STARTING event followed by a STARTING_ABORTED event. https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/1/media/capture/video/video_c... media/capture/video/video_capture_device.h:218: // VideoCaptureDevice reports it's successfully started. On 2017/02/14 01:05:49, braveyao wrote: > On 2017/02/09 23:34:52, chfremer wrote: > > On 2017/02/09 22:21:49, miu wrote: > > > There are two things you'll want to consider here: > > > > > > 1. Any thread can call OnStarted() (at least, FWICT in this patch). Consider > > > whether this is acceptable, or that OnStarted() must be called on the same > > > thread that called AllocateAndStart(). > > > > > > 2. OnStarted() may be called synchronously (i.e., within a call to > > > AllocateAndStart()), or asynchronously. Are your client impl's prepared for > > > that? > > > > > > Regardless of whether you decide to change (or not change) anything here, > > please > > > document these behaviors in the method comments here. > > > > Regardless of which exact thread it comes from, we should guarantee two > things: > > 1.) OnStarted() is not called concurrently with any of the other methods > > (actually, none of the VideoCaptureDevice::Client methods should be called > > concurrently) > > 2.) OnStarted() is guaranteed to get invoked before the first frame is > > delivered. > > All the methods will be posted to IO thread > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...). > So there won't be concurrent calls. > The state machine will guarantee that the first frame won't be processed until > OnStarted is received. (see > https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp...) > > Please let me know if you have more concerns. Please see my follow-up comment in Patch Set 2. https://codereview.chromium.org/2673373003/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:222: virtual void OnStarted() = 0; A couple of recommendations here: 1.) I recommend removing the sentence "This is OK because ...". Reason: The description for this method is supposed to define a contract between implementations of VideoCaptureDevice and implementations of VideoCaptureDevice::Client. This contract should be independent of information regarding what implementations we happen to use downstream and how they behave (e.g. VideoCaptureImpl). It should only state what guarantees it makes. 2.) When stating that it may be invoked synchronously, let us make it clear where the synchronous call would originate from, see example below. 3.) As part of the contract, we should specify whether or not we guarantee that OnStarted() gets called before any video frame is sent. Silently relying on VideoCaptureImpl being used downstream and assuming it (and everything in between) correctly handles the case where video frames are sent before OnStarted() gets called is dangerous, because even if it may be true today this can easily change and cause things to break in unexpected ways. 4.) Allowing video frames to get sent before OnStarted() makes the contract much more complicated than if we would guarantee that OnStarted() is always called before any frame is sent. It would also raise the question if it can happen that the OnStarted() call arrives concurrently with the delivery of a frame. I highly recommend going for the simpler contract. Example: VideoCaptureDevice reports it's successfully started. This method may be called either synchronously from an invocation of VideoCaptureDevice::AllocateAndStart() or asynchronously from any thread after AllocateAndStart() has returned. It is guaranteed to get called before any video frames are delivered.
The CQ bit was checked by braveyao@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 checked by braveyao@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 checked by braveyao@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 #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
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 braveyao@chromium.org to run a CQ dry run
Patchset #4 (id:80002) 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: 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 braveyao@chromium.org to run a CQ dry run
Patchset #4 (id:110001) 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Hi miu@ and chfremer@, all comments addressed in PS#4. PTAL! https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... File content/browser/media/capture/desktop_capture_device_aura_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/media/captu... content/browser/media/capture/desktop_capture_device_aura_unittest.cc:59: MOCK_METHOD0(OnStarted, void(void)); On 2017/02/14 01:05:48, braveyao wrote: > On 2017/02/09 22:21:49, miu wrote: > > One or more unit tests should probably EXPECT_CALL(OnStarted), right? > > Done. > > Thanks for the suggestion! Oh, no. This test case is to stop immediately after start. So OnStarted won't be guaranteed to be called. At it seems that the screen capture doesn't really started in this unittest. I tried not to call Stop and there is still no OnStart event. I tried the method in desktop_capture_device_unittest.cc to wait for the event and it will time out. No ChromeOS at my hand to investigate more right now. So add the EXCEPT_CAll(OnStarted) as a placeholder for now. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_host.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_host.cc:125: BrowserThread::IO, FROM_HERE, On 2017/02/14 19:21:22, chfremer wrote: > On 2017/02/14 01:05:49, braveyao wrote: > > On 2017/02/09 23:34:52, chfremer wrote: > > > Why is it necessary to schedule this to the end of the current message queue > > > instead of invoking > > device_id_to_observer_map_[controller_id]->OnStateChanged() > > > directly? > > > > > > I see that this is also done for OnError() and OnEnded(), but not for the > > other > > > methods. > > > But I couldn't find any documentation explaining why this is done. > > > > The EventHandler methods used to call "PostTask" before. And since cl, > > https://codereview.chromium.org/1017503002, some of them are switched to IPC > > message methods and then Mojo(I suppose). > > I guess they will all be unified eventually in Mojo? > > Thanks for the reference to that CL. It looks like mcasas@ removed the > PostTask for some of the methods and left it in place for others. > The question is why is the PostTask needed for some but not all of the > methods? > > I feel that if we are adding a new method OnStarted() we need to be > able to answer this in order to decide whether or not PostTask needs > to be used in OnStarted(). Done. Thanks so much for pushing on this! In the beginning I didn't notice that not all methods need PostTask. Now I found out that the OnError and OnEnd events still need PostTask to finish some other clearing jobs before they are handled. (DesktopCaptureApiTest and VideoCaptureTest may fail if we remove PostTask to these two.) Only OnStart can be processed directly. Changes to DoError/DoEnd is too sophisticated, which deserves a separate study and cl. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:251: .Times(AtMost(1)); On 2017/02/14 19:21:22, chfremer wrote: > On 2017/02/14 01:05:49, braveyao wrote: > > On 2017/02/09 23:34:52, chfremer wrote: > > > Do we really want to modify the contract between VideoCaptureHost and > > > VideoCaptureObserver such that it is legal for VideoCaptureHost to report > > > STOPPED without first reporting STARTED? > > > As a user of the interface that would feel unexpected to me. > > > If a stop request can cancel a start request before it has completed, could > we > > > make it that in that case neither STARTED nor STOPPED is reported? > > > > > > I would prefer that we guarantee that we get either a) STARTED followed by > > > STOPPED or b) no state changes at all. > > > > It's natural to me that a stop request can cancel a start request before it > has > > completed. And at this point, only the STOPPED report will be concerned, as > the > > STARTED is not important any more. > > WDTY? > > I think it is confusing for the listener when a STOPPED event can arrive without > any preceding STARTED event. If starting is aborted without any visible effect > for the listener, there is no reason to send any notification to the listener, > not even a STOPPED. If we do want to notify listeners about the aborted attemt > to start, we should send a STARTING event followed by a STARTING_ABORTED event. As we discussed, the current processing is OK. Keep it as is. Let me know if there is any other concern. https://codereview.chromium.org/2673373003/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:222: virtual void OnStarted() = 0; On 2017/02/14 19:21:23, chfremer wrote: > A couple of recommendations here: > > 1.) I recommend removing the sentence "This is OK because ...". > Reason: > The description for this method is supposed to define a contract between > implementations of VideoCaptureDevice and implementations of > VideoCaptureDevice::Client. This contract should be independent of information > regarding what implementations we happen to use downstream and how they behave > (e.g. VideoCaptureImpl). It should only state what guarantees it makes. > > 2.) When stating that it may be invoked synchronously, let us make it clear > where the synchronous call would originate from, see example below. > > 3.) As part of the contract, we should specify whether or not we guarantee that > OnStarted() gets called before any video frame is sent. Silently relying on > VideoCaptureImpl being used downstream and assuming it (and everything in > between) correctly handles the case where video frames are sent before > OnStarted() gets called is dangerous, because even if it may be true today this > can easily change and cause things to break in unexpected ways. > > 4.) Allowing video frames to get sent before OnStarted() makes the contract much > more complicated than if we would guarantee that OnStarted() is always called > before any frame is sent. It would also raise the question if it can happen that > the OnStarted() call arrives concurrently with the delivery of a frame. > I highly recommend going for the simpler contract. > > Example: > VideoCaptureDevice reports it's successfully started. > This method may be called either synchronously from an invocation of > VideoCaptureDevice::AllocateAndStart() or asynchronously from any thread after > AllocateAndStart() has returned. It is guaranteed to get called before any > video frames are delivered. Done. Basically, there is hundreds of milliseconds delay after we start the capture device to get the first captured frame. Plus the existing state machine in VCI, it won't be a problem if we call OnStarted synchronously. So here only Video/Sceen capture on Android and Video capture on Linux/CrOS, which will call OnStarted asynchronously, need to be taken care of. The simple process is to set the capture flag after OnStarted event and forward capture frame only after the flag is set.
I have a couple more questions, especially about threading. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:249: // is stopped immediately. Please remove the "now", because the reader of the comment is not typically interested in the history of the code. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:251: .Times(AtMost(1)); On 2017/02/15 21:47:15, braveyao wrote: > On 2017/02/14 19:21:22, chfremer wrote: > > On 2017/02/14 01:05:49, braveyao wrote: > > > On 2017/02/09 23:34:52, chfremer wrote: > > > > Do we really want to modify the contract between VideoCaptureHost and > > > > VideoCaptureObserver such that it is legal for VideoCaptureHost to report > > > > STOPPED without first reporting STARTED? > > > > As a user of the interface that would feel unexpected to me. > > > > If a stop request can cancel a start request before it has completed, > could > > we > > > > make it that in that case neither STARTED nor STOPPED is reported? > > > > > > > > I would prefer that we guarantee that we get either a) STARTED followed by > > > > STOPPED or b) no state changes at all. > > > > > > It's natural to me that a stop request can cancel a start request before it > > has > > > completed. And at this point, only the STOPPED report will be concerned, as > > the > > > STARTED is not important any more. > > > WDTY? > > > > I think it is confusing for the listener when a STOPPED event can arrive > without > > any preceding STARTED event. If starting is aborted without any visible effect > > > for the listener, there is no reason to send any notification to the listener, > > > not even a STOPPED. If we do want to notify listeners about the aborted attemt > > > to start, we should send a STARTING event followed by a STARTING_ABORTED > event. > > As we discussed, the current processing is OK. Keep it as is. > Let me know if there is any other concern. As per our offline discussion, I understand that the listener requires the STOPPED event to tear down state that it creates even before requesting to start the device. In that case, let us make the contract as clear as possible. It seems that if STARTED is called, it must be called before STOPPED. We should express that in this test using gmock's InSequence. Then, in the file video_capture.mojom, there is a description comment outlining the protocol of communication between Observer and VideoCaptureHost. We should make a note there that it is legal that no STARTED event is sent in case a Stop request cancels the preceding Start request. https://codereview.chromium.org/2673373003/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:222: virtual void OnStarted() = 0; On 2017/02/15 21:47:16, braveyao wrote: > On 2017/02/14 19:21:23, chfremer wrote: > > A couple of recommendations here: > > > > 1.) I recommend removing the sentence "This is OK because ...". > > Reason: > > The description for this method is supposed to define a contract between > > implementations of VideoCaptureDevice and implementations of > > VideoCaptureDevice::Client. This contract should be independent of information > > > regarding what implementations we happen to use downstream and how they behave > > > (e.g. VideoCaptureImpl). It should only state what guarantees it makes. > > > > 2.) When stating that it may be invoked synchronously, let us make it clear > > where the synchronous call would originate from, see example below. > > > > 3.) As part of the contract, we should specify whether or not we guarantee > that > > OnStarted() gets called before any video frame is sent. Silently relying on > > VideoCaptureImpl being used downstream and assuming it (and everything in > > between) correctly handles the case where video frames are sent before > > OnStarted() gets called is dangerous, because even if it may be true today > this > > can easily change and cause things to break in unexpected ways. > > > > 4.) Allowing video frames to get sent before OnStarted() makes the contract > much > > more complicated than if we would guarantee that OnStarted() is always called > > before any frame is sent. It would also raise the question if it can happen > that > > the OnStarted() call arrives concurrently with the delivery of a frame. > > I highly recommend going for the simpler contract. > > > > Example: > > VideoCaptureDevice reports it's successfully started. > > This method may be called either synchronously from an invocation of > > VideoCaptureDevice::AllocateAndStart() or asynchronously from any thread after > > > AllocateAndStart() has returned. It is guaranteed to get called before any > > video frames are delivered. > > Done. > > Basically, there is hundreds of milliseconds delay after we start the capture > device to get the first captured frame. Plus the existing state machine in VCI, > it won't be a problem if we call OnStarted synchronously. > So here only Video/Sceen capture on Android and Video capture on Linux/CrOS, > which will call OnStarted asynchronously, need to be taken care of. The simple > process is to set the capture flag after OnStarted event and forward capture > frame only after the flag is set. I looked at the device implementations and had some questions regarding potential race conditions. Please look at my other comments. Note, that for this it does not matter how many milliseconds of delay there typically is. https://codereview.chromium.org/2673373003/diff/130001/content/browser/media/... File content/browser/media/capture/desktop_capture_device_aura_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/130001/content/browser/media/... content/browser/media/capture/desktop_capture_device_aura_unittest.cc:160: // |STARTED| is reported asynchronously now, which may not be got if capture Please remove the word "now". https://codereview.chromium.org/2673373003/diff/130001/content/browser/media/... File content/browser/media/capture/screen_capture_device_android_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/130001/content/browser/media/... content/browser/media/capture/screen_capture_device_android_unittest.cc:97: // |STARTED| is reported asynchronously now, which may not be got if capture Please remove the word "now". https://codereview.chromium.org/2673373003/diff/130001/content/browser/render... File content/browser/renderer_host/media/video_capture_host.cc (right): https://codereview.chromium.org/2673373003/diff/130001/content/browser/render... content/browser/renderer_host/media/video_capture_host.cc:124: void VideoCaptureHost::OnStarted(VideoCaptureControllerID controller_id) { Thanks. This piece of information is very useful to readers of the code. We could make it even more useful by placing it in a better location. A comment here will only be found by readers of the implementation of OnStarted(). However, the information actually seems to be a crucial part of the contract between VideoCaptureHost and VideoCaptureController. It basically says that implementations of OnStarted() are required to handle OnError() and OnEnded() asynchronously (and bad things will happen if they don't). The best location for this information would be in video_capture_controller_event_handler.h. https://codereview.chromium.org/2673373003/diff/130001/content/browser/render... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/130001/content/browser/render... content/browser/renderer_host/media/video_capture_unittest.cc:248: // |STARTED| is reported asynchronously now, which may not be got if capture Please remove the word "now". https://codereview.chromium.org/2673373003/diff/130001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:264: // On Android, we must wait for user input to start capturing before we can The "On Android, " is not needed, because it is clear from the context that we are on Android here. https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:401: nativeOnStarted(mNativeVideoCaptureDeviceAndroid); Without having tried to read and understand all of this class ... does this code path mean that is is legal to call startCapture() when the device is already started? And if that happens, we are sending out another STARTED event to all clients? If that is the case, the contract would have to state that STARTED events may arrive at the clients any number of times while the device is running (which would be pretty confusing). https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:415: nativeOnStarted(mNativeVideoCaptureDeviceAndroid); It seems that here we invoke OnStarted() after mIsRunning has been set to true and after mCamera.startPreview() has been called. Can it happen that a frame from the camera arrives and is sent before this invocation of OnStarted()? If not, why? https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java:120: Log.e(TAG, "Get captured frame in unexpected state."); Is this really unexpected? If the camera can deliver frames before the state has changed to STARTED in method onConfigured(), wouldn't hitting this code path and discarding the frame be expected behavior? https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:362: client_->OnStarted(); Unless ReceiveFrame() is invoked on the same thread as this one, this looks like a race condition, i.e. it could happen that ReceiveFrame() is called before OnStarted(). Note, that ReceiveFrame() does not check |state_| in order to discard frames until |state_ == kCapturing|. Any reason to not call client_->OnStarted() before [capture_device_ startCapture]? https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/wi... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:425: client_->OnStarted(); Unless FrameReceived() is called on the same thread as this, this looks like a race condition. FrameReceived() may get called before OnStarted(). Any reason to not call client_->OnStarted() before media_control_->Run()?
The CQ bit was checked by braveyao@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 braveyao@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_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by braveyao@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 braveyao@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 #7 (id:190001) has been deleted
Patchset #6 (id:170001) has been deleted
Patchset #5 (id:150001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by braveyao@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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 braveyao@chromium.org to run a CQ dry run
Patchset #6 (id:230001) 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: This issue passed the CQ dry run.
Hi Chfremer@ & Miu@, all comments addressed/answered. PTAL. PS: I didn't really understand the MOCK_METHOD, EXPECT_CALL and GMock Warning until yesterday. Shame on me, again... Revised unittests to add as many as possible EXPECT_CALL(OnStarted) properly. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:249: // is stopped immediately. On 2017/02/16 01:00:50, chfremer wrote: > Please remove the "now", because the reader of the comment is not typically > interested in the history of the code. Done. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:251: .Times(AtMost(1)); On 2017/02/16 01:00:50, chfremer wrote: > On 2017/02/15 21:47:15, braveyao wrote: > > On 2017/02/14 19:21:22, chfremer wrote: > > > On 2017/02/14 01:05:49, braveyao wrote: > > > > On 2017/02/09 23:34:52, chfremer wrote: > > > > > Do we really want to modify the contract between VideoCaptureHost and > > > > > VideoCaptureObserver such that it is legal for VideoCaptureHost to > report > > > > > STOPPED without first reporting STARTED? > > > > > As a user of the interface that would feel unexpected to me. > > > > > If a stop request can cancel a start request before it has completed, > > could > > > we > > > > > make it that in that case neither STARTED nor STOPPED is reported? > > > > > > > > > > I would prefer that we guarantee that we get either a) STARTED followed > by > > > > > STOPPED or b) no state changes at all. > > > > > > > > It's natural to me that a stop request can cancel a start request before > it > > > has > > > > completed. And at this point, only the STOPPED report will be concerned, > as > > > the > > > > STARTED is not important any more. > > > > WDTY? > > > > > > I think it is confusing for the listener when a STOPPED event can arrive > > without > > > any preceding STARTED event. If starting is aborted without any visible > effect > > > > > for the listener, there is no reason to send any notification to the > listener, > > > > > not even a STOPPED. If we do want to notify listeners about the aborted > attemt > > > > > to start, we should send a STARTING event followed by a STARTING_ABORTED > > event. > > > > As we discussed, the current processing is OK. Keep it as is. > > Let me know if there is any other concern. > > As per our offline discussion, I understand that the listener requires > the STOPPED event to tear down state that it creates even before requesting > to start the device. > > In that case, let us make the contract as clear as possible. > It seems that if STARTED is called, it must be called before STOPPED. > We should express that in this test using gmock's InSequence. > > Then, in the file video_capture.mojom, there is a description comment > outlining the protocol of communication between Observer and VideoCaptureHost. > We should make a note there that it is legal that no STARTED event is > sent in case a Stop request cancels the preceding Start request. > Done. https://codereview.chromium.org/2673373003/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:222: virtual void OnStarted() = 0; On 2017/02/16 01:00:50, chfremer wrote: > On 2017/02/15 21:47:16, braveyao wrote: > > On 2017/02/14 19:21:23, chfremer wrote: > > > A couple of recommendations here: > > > > > > 1.) I recommend removing the sentence "This is OK because ...". > > > Reason: > > > The description for this method is supposed to define a contract between > > > implementations of VideoCaptureDevice and implementations of > > > VideoCaptureDevice::Client. This contract should be independent of > information > > > > > regarding what implementations we happen to use downstream and how they > behave > > > > > (e.g. VideoCaptureImpl). It should only state what guarantees it makes. > > > > > > 2.) When stating that it may be invoked synchronously, let us make it clear > > > where the synchronous call would originate from, see example below. > > > > > > 3.) As part of the contract, we should specify whether or not we guarantee > > that > > > OnStarted() gets called before any video frame is sent. Silently relying on > > > VideoCaptureImpl being used downstream and assuming it (and everything in > > > between) correctly handles the case where video frames are sent before > > > OnStarted() gets called is dangerous, because even if it may be true today > > this > > > can easily change and cause things to break in unexpected ways. > > > > > > 4.) Allowing video frames to get sent before OnStarted() makes the contract > > much > > > more complicated than if we would guarantee that OnStarted() is always > called > > > before any frame is sent. It would also raise the question if it can happen > > that > > > the OnStarted() call arrives concurrently with the delivery of a frame. > > > I highly recommend going for the simpler contract. > > > > > > Example: > > > VideoCaptureDevice reports it's successfully started. > > > This method may be called either synchronously from an invocation of > > > VideoCaptureDevice::AllocateAndStart() or asynchronously from any thread > after > > > > > AllocateAndStart() has returned. It is guaranteed to get called before any > > > video frames are delivered. > > > > Done. > > > > Basically, there is hundreds of milliseconds delay after we start the capture > > device to get the first captured frame. Plus the existing state machine in > VCI, > > it won't be a problem if we call OnStarted synchronously. > > So here only Video/Sceen capture on Android and Video capture on Linux/CrOS, > > which will call OnStarted asynchronously, need to be taken care of. The simple > > process is to set the capture flag after OnStarted event and forward capture > > frame only after the flag is set. > > I looked at the device implementations and had some questions regarding > potential race conditions. Please look at my other comments. > > Note, that for this it does not matter how many milliseconds of delay there > typically is. Acknowledged. From this point of view, it is the thing good to have. I went through all the device implementations. Please have a look to the changes there. https://codereview.chromium.org/2673373003/diff/130001/content/browser/media/... File content/browser/media/capture/desktop_capture_device_aura_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/130001/content/browser/media/... content/browser/media/capture/desktop_capture_device_aura_unittest.cc:160: // |STARTED| is reported asynchronously now, which may not be got if capture On 2017/02/16 01:00:50, chfremer wrote: > Please remove the word "now". Done. https://codereview.chromium.org/2673373003/diff/130001/content/browser/media/... File content/browser/media/capture/screen_capture_device_android_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/130001/content/browser/media/... content/browser/media/capture/screen_capture_device_android_unittest.cc:97: // |STARTED| is reported asynchronously now, which may not be got if capture On 2017/02/16 01:00:50, chfremer wrote: > Please remove the word "now". Done. https://codereview.chromium.org/2673373003/diff/130001/content/browser/render... File content/browser/renderer_host/media/video_capture_host.cc (right): https://codereview.chromium.org/2673373003/diff/130001/content/browser/render... content/browser/renderer_host/media/video_capture_host.cc:124: void VideoCaptureHost::OnStarted(VideoCaptureControllerID controller_id) { On 2017/02/16 01:00:50, chfremer wrote: > Thanks. This piece of information is very useful to readers of the code. > > We could make it even more useful by placing it in a better location. > A comment here will only be found by readers of the implementation of > OnStarted(). > However, the information actually seems to be a crucial part of the contract > between VideoCaptureHost and VideoCaptureController. It basically says that > implementations of OnStarted() are required to handle OnError() and OnEnded() > asynchronously (and bad things will happen if they don't). > The best location for this information would be in > video_capture_controller_event_handler.h. Done. https://codereview.chromium.org/2673373003/diff/130001/content/browser/render... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/130001/content/browser/render... content/browser/renderer_host/media/video_capture_unittest.cc:248: // |STARTED| is reported asynchronously now, which may not be got if capture On 2017/02/16 01:00:50, chfremer wrote: > Please remove the word "now". Done. https://codereview.chromium.org/2673373003/diff/130001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:264: // On Android, we must wait for user input to start capturing before we can On 2017/02/16 01:00:50, chfremer wrote: > The "On Android, " is not needed, because it is clear from the context that we > are on Android here. Done. https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:401: nativeOnStarted(mNativeVideoCaptureDeviceAndroid); On 2017/02/16 01:00:50, chfremer wrote: > Without having tried to read and understand all of this class ... > > does this code path mean that is is legal to call startCapture() when > the device is already started? And if that happens, we are sending out > another STARTED event to all clients? > > If that is the case, the contract would have to state that STARTED > events may arrive at the clients any number of times while the device > is running (which would be pretty confusing). Done. Anyway, if there are multiple OnStarted events from the same device, but still only one will be forwarded to VideoCaptureImpl. https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:415: nativeOnStarted(mNativeVideoCaptureDeviceAndroid); On 2017/02/16 01:00:50, chfremer wrote: > It seems that here we invoke OnStarted() after mIsRunning has been set to true > and after mCamera.startPreview() has been called. Can it happen that a frame > from the camera arrives and is sent before this invocation of OnStarted()? If > not, why? Done. https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java:120: Log.e(TAG, "Get captured frame in unexpected state."); On 2017/02/16 01:00:50, chfremer wrote: > Is this really unexpected? > If the camera can deliver frames before the state has changed to STARTED in > method onConfigured(), wouldn't hitting this code path and discarding the frame > be expected behavior? This check also includes the situation that getting captured frame after we stop capture. Even I didn't encounter such situation with camera capture, but it does happen to screen capture and caused some issues. So I define it as Error. Remove the log here though. https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:362: client_->OnStarted(); On 2017/02/16 01:00:50, chfremer wrote: > Unless ReceiveFrame() is invoked on the same thread as this one, this looks like > a race condition, i.e. it could happen that ReceiveFrame() is called before > OnStarted(). > > Note, that ReceiveFrame() does not check |state_| in order to discard frames > until |state_ == kCapturing|. > > Any reason to not call client_->OnStarted() before [capture_device_ > startCapture]? OnStarted must be called at the last of AllocatAndStart to ensure the device is successfully started. https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/wi... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2673373003/diff/130001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:425: client_->OnStarted(); On 2017/02/16 01:00:50, chfremer wrote: > Unless FrameReceived() is called on the same thread as this, this looks like a > race condition. FrameReceived() may get called before OnStarted(). > > Any reason to not call client_->OnStarted() before media_control_->Run()? We need to call OnStarted() after the capture is successfully started. Besides: there is the reason of issue651910
Most things look resolved. For the rest, I have follow-up questions. https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:251: .Times(AtMost(1)); On 2017/02/17 20:37:20, braveyao wrote: > On 2017/02/16 01:00:50, chfremer wrote: > > On 2017/02/15 21:47:15, braveyao wrote: > > > On 2017/02/14 19:21:22, chfremer wrote: > > > > On 2017/02/14 01:05:49, braveyao wrote: > > > > > On 2017/02/09 23:34:52, chfremer wrote: > > > > > > Do we really want to modify the contract between VideoCaptureHost and > > > > > > VideoCaptureObserver such that it is legal for VideoCaptureHost to > > report > > > > > > STOPPED without first reporting STARTED? > > > > > > As a user of the interface that would feel unexpected to me. > > > > > > If a stop request can cancel a start request before it has completed, > > > could > > > > we > > > > > > make it that in that case neither STARTED nor STOPPED is reported? > > > > > > > > > > > > I would prefer that we guarantee that we get either a) STARTED > followed > > by > > > > > > STOPPED or b) no state changes at all. > > > > > > > > > > It's natural to me that a stop request can cancel a start request before > > it > > > > has > > > > > completed. And at this point, only the STOPPED report will be concerned, > > as > > > > the > > > > > STARTED is not important any more. > > > > > WDTY? > > > > > > > > I think it is confusing for the listener when a STOPPED event can arrive > > > without > > > > any preceding STARTED event. If starting is aborted without any visible > > effect > > > > > > > for the listener, there is no reason to send any notification to the > > listener, > > > > > > > not even a STOPPED. If we do want to notify listeners about the aborted > > attemt > > > > > > > to start, we should send a STARTING event followed by a STARTING_ABORTED > > > event. > > > > > > As we discussed, the current processing is OK. Keep it as is. > > > Let me know if there is any other concern. > > > > As per our offline discussion, I understand that the listener requires > > the STOPPED event to tear down state that it creates even before requesting > > to start the device. > > > > In that case, let us make the contract as clear as possible. > > It seems that if STARTED is called, it must be called before STOPPED. > > We should express that in this test using gmock's InSequence. I couldn't find this change in PatchSet#6. Also note that the InSequence constraint should probably go to all the unit tests that check for the arrival of the STARTED and STOPPED events, not just this one. > > Then, in the file video_capture.mojom, there is a description comment > > outlining the protocol of communication between Observer and VideoCaptureHost. > > We should make a note there that it is legal that no STARTED event is > > sent in case a Stop request cancels the preceding Start request. > > I also couldn't find this change in PatchSet#6. > Done. https://codereview.chromium.org/2673373003/diff/250001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_event_handler.h (right): https://codereview.chromium.org/2673373003/diff/250001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_event_handler.h:54: virtual void OnStarted(VideoCaptureControllerID id) = 0; The second sentence of the comment for this method concerns several other methods. It would probably not be found by a reader only looking at OnError or OnEnded. Therefore it should go to the class description. Could you please move it to there? https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:414: mIsRunning = true; I am not sure we can move mIsRunning = true out from the scope that has mPreviewBufferLock locked. Otherwise, setting mIsRunning = true may happen concurrently with the check if (!mIsRunning) in method onPreviewFrame(). I don't know Java well enough to know whether or not bools are safe to use concurrently. Could you please confirm that this is intended and valid? https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:410: if (state_ != kCapturing) Unless this runs on the same thread as AllocateAndStart(), this check may happen concurrently with AllocateAndStart() changing |state_| to kCapturing. I don't know if this is a legal atomic operation or if it needs to be synchronized using locks. Could you please check/confirm? https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/wi... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:461: if (state_ != kCapturing) Unless this runs on the same thread as AllocateAndStart(), this check may happen concurrently with AllocateAndStart() changing |state_| to kCapturing. I don't know if this is a legal atomic operation or if it needs to be synchronized using locks. Could you please check/confirm?
The CQ bit was checked by braveyao@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...
https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... File media/capture/content/thread_safe_capture_oracle.cc (right): https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... media/capture/content/thread_safe_capture_oracle.cc:230: if (!client_ || !capture_) Dropping frames at this stage could break things (there were several similar bugs in the past related to dropping frames). I thought about this whole change a bit more, and I'm not convinced this is the right overall approach to solving the problem (as I understand it). I think you should remove this logic (and in all the other VCD impls). We shouldn't require that OnStarted() be called before any frames can be delivered. This requirement was never necessary before. Instead, the API contract was that frames could be delivered at any point once AllocateAndStart() is called. If it is a requirement for gUM, then gUM should drop frames at the consumer end. But, that might be a very bad idea: dropping *any* frames can break things because not all VCD's continuously produce frames. In other words, there are start-up scenarios where a producer will send only one frame and then no others for a long period of time, and if that first frame is dropped, consumers will never see a first frame. (again, there were bugs relating to this) Stepping back, I wonder if it'd be simpler to just scrap all the browser-side changes. Remove OnStarted() altogether. Instead, in the render process, could gUM simply infer that receiving a first frame means the capture device successfully started? The web platform event for "started capture" could be dispatched at the time the first frame arrives at the consumer.
On 2017/02/18 01:18:46, miu wrote: > https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... > File media/capture/content/thread_safe_capture_oracle.cc (right): > > https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... > media/capture/content/thread_safe_capture_oracle.cc:230: if (!client_ || > !capture_) > Dropping frames at this stage could break things (there were several similar > bugs in the past related to dropping frames). > > I thought about this whole change a bit more, and I'm not convinced this is the > right overall approach to solving the problem (as I understand it). > > I think you should remove this logic (and in all the other VCD impls). We > shouldn't require that OnStarted() be called before any frames can be delivered. > This requirement was never necessary before. Instead, the API contract was that > frames could be delivered at any point once AllocateAndStart() is called. > > If it is a requirement for gUM, then gUM should drop frames at the consumer end. > But, that might be a very bad idea: dropping *any* frames can break things > because not all VCD's continuously produce frames. In other words, there are > start-up scenarios where a producer will send only one frame and then no others > for a long period of time, and if that first frame is dropped, consumers will > never see a first frame. (again, there were bugs relating to this) > > Stepping back, I wonder if it'd be simpler to just scrap all the browser-side > changes. Remove OnStarted() altogether. Instead, in the render process, could > gUM simply infer that receiving a first frame means the capture device > successfully started? The web platform event for "started capture" could be > dispatched at the time the first frame arrives at the consumer. Hi Miu@, As a matter of fact, I also agree that We shouldn't require that OnStarted() be called before any frames can be delivered. Also practically, there won't be frame captured that quickly. But frame dropping is quite common in WebRTC, i.e. due to CPU/Network and etc.. It's BAD of course, especially to the first frame. But at least audio call can start, which is more important for a call session. I will talk to chfremer@ again. We also discussed about the possibility to infer the first frame as device-started. This has some drawbacks. First there is hundreds milliseconds delay between OnStarted event and the first frame. It's too late for gUM to get the success callback to start the call then. And if for any reason, no first frame for quite a while, then gUM won't get the success callback to start the call at all. That's really bad. I suppose gUM should only care about if capture device is started successfully, but not when the first frame comes. Any better idea?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/02/18 02:04:54, braveyao wrote: > On 2017/02/18 01:18:46, miu wrote: > > > https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... > > File media/capture/content/thread_safe_capture_oracle.cc (right): > > > > > https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... > > media/capture/content/thread_safe_capture_oracle.cc:230: if (!client_ || > > !capture_) > > Dropping frames at this stage could break things (there were several similar > > bugs in the past related to dropping frames). > > > > I thought about this whole change a bit more, and I'm not convinced this is > the > > right overall approach to solving the problem (as I understand it). > > > > I think you should remove this logic (and in all the other VCD impls). We > > shouldn't require that OnStarted() be called before any frames can be > delivered. > > This requirement was never necessary before. Instead, the API contract was > that > > frames could be delivered at any point once AllocateAndStart() is called. > > > > If it is a requirement for gUM, then gUM should drop frames at the consumer > end. > > But, that might be a very bad idea: dropping *any* frames can break things > > because not all VCD's continuously produce frames. In other words, there are > > start-up scenarios where a producer will send only one frame and then no > others > > for a long period of time, and if that first frame is dropped, consumers will > > never see a first frame. (again, there were bugs relating to this) > > > > Stepping back, I wonder if it'd be simpler to just scrap all the browser-side > > changes. Remove OnStarted() altogether. Instead, in the render process, could > > gUM simply infer that receiving a first frame means the capture device > > successfully started? The web platform event for "started capture" could be > > dispatched at the time the first frame arrives at the consumer. > > Hi Miu@, > > As a matter of fact, I also agree that We shouldn't require that OnStarted() be > called before any frames can be delivered. Also practically, there won't be > frame captured that quickly. > But frame dropping is quite common in WebRTC, i.e. due to CPU/Network and etc.. > It's BAD of course, especially to the first frame. But at least audio call can > start, which is more important for a call session. > I will talk to chfremer@ again. > > We also discussed about the possibility to infer the first frame as > device-started. This has some drawbacks. > First there is hundreds milliseconds delay between OnStarted event and the first > frame. It's too late for gUM to get the success callback to start the call then. > And if for any reason, no first frame for quite a while, then gUM won't get the > success callback to start the call at all. That's really bad. > I suppose gUM should only care about if capture device is started successfully, > but not when the first frame comes. > > Any better idea? Hmm, they way I understand it, the reason we want to add a OnStarted() event is to avoid the situation where we optimistically report success to the caller of getUserMedia before actually starting the device and then have the actual device start fail (which is what happens currently). I see two possible approaches: 1.) We could take the first arriving frame as a signal of a successful start and wait until then to report the success to the caller of getUserMedia. braveyao@: You are saying that this would be too late. Why is that? As far as I know, getUserMedia is already promise-based, so it should be acceptable to take a while to respond. 2.) We could keep the OnStarted() and also the contract that OnStarted() is called before the first frame is delivered. miu@, I understand the concern about dropping a first frame when no subsequent frame may be sent after it for a long time. But that should only be the case for content capture implementations (tab/desktop/canvas/window). And those implementations should easily be able to coordinate things such that they do not sent that first frame before OnStarted() has been called. Is there a similar concern w.r.t. dropping frames for webcam-like devices?
On 2017/02/18 02:32:26, chfremer wrote: > On 2017/02/18 02:04:54, braveyao wrote: > > On 2017/02/18 01:18:46, miu wrote: > > > > > > https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... > > > File media/capture/content/thread_safe_capture_oracle.cc (right): > > > > > > > > > https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... > > > media/capture/content/thread_safe_capture_oracle.cc:230: if (!client_ || > > > !capture_) > > > Dropping frames at this stage could break things (there were several similar > > > bugs in the past related to dropping frames). > > > > > > I thought about this whole change a bit more, and I'm not convinced this is > > the > > > right overall approach to solving the problem (as I understand it). > > > > > > I think you should remove this logic (and in all the other VCD impls). We > > > shouldn't require that OnStarted() be called before any frames can be > > delivered. > > > This requirement was never necessary before. Instead, the API contract was > > that > > > frames could be delivered at any point once AllocateAndStart() is called. > > > > > > If it is a requirement for gUM, then gUM should drop frames at the consumer > > end. > > > But, that might be a very bad idea: dropping *any* frames can break things > > > because not all VCD's continuously produce frames. In other words, there are > > > start-up scenarios where a producer will send only one frame and then no > > others > > > for a long period of time, and if that first frame is dropped, consumers > will > > > never see a first frame. (again, there were bugs relating to this) > > > > > > Stepping back, I wonder if it'd be simpler to just scrap all the > browser-side > > > changes. Remove OnStarted() altogether. Instead, in the render process, > could > > > gUM simply infer that receiving a first frame means the capture device > > > successfully started? The web platform event for "started capture" could be > > > dispatched at the time the first frame arrives at the consumer. > > > > Hi Miu@, > > > > As a matter of fact, I also agree that We shouldn't require that OnStarted() > be > > called before any frames can be delivered. Also practically, there won't be > > frame captured that quickly. > > But frame dropping is quite common in WebRTC, i.e. due to CPU/Network and > etc.. > > It's BAD of course, especially to the first frame. But at least audio call can > > start, which is more important for a call session. > > I will talk to chfremer@ again. > > > > We also discussed about the possibility to infer the first frame as > > device-started. This has some drawbacks. > > First there is hundreds milliseconds delay between OnStarted event and the > first > > frame. It's too late for gUM to get the success callback to start the call > then. > > And if for any reason, no first frame for quite a while, then gUM won't get > the > > success callback to start the call at all. That's really bad. > > I suppose gUM should only care about if capture device is started > successfully, > > but not when the first frame comes. > > > > Any better idea? > > Hmm, they way I understand it, the reason we want to add a OnStarted() event is > to avoid the situation where we optimistically report success to the caller of > getUserMedia before actually starting the device and then have the actual device > start fail (which is what happens currently). > > I see two possible approaches: > 1.) We could take the first arriving frame as a signal of a successful start and > wait until then to report the success to the caller of getUserMedia. braveyao@: > You are saying that this would be too late. Why is that? As far as I know, > getUserMedia is already promise-based, so it should be acceptable to take a > while to respond. > > 2.) We could keep the OnStarted() and also the contract that OnStarted() is > called before the first frame is delivered. miu@, I understand the concern about > dropping a first frame when no subsequent frame may be sent after it for a long > time. But that should only be the case for content capture implementations > (tab/desktop/canvas/window). And those implementations should easily be able to > coordinate things such that they do not sent that first frame before OnStarted() > has been called. Is there a similar concern w.r.t. dropping frames for > webcam-like devices? Ah, I got an idea: Keep VCD as is, except reporting OnStarted. In case that first frame arrives earlier than Onstarted (though I can't see why it's possible), we can mark it down. Then after OnStarted arrives, we check the mark and ask for a frame refresh if there is any frame dropping(I don't think caching the frame is good idea here). As to camera capture, this request can be ignored because there's always periodically frame-capture. As to screen capture, we can utilize the existing refresh mechanism to deliver another frame if needed. Then gUM can callback with the generated stream info as early as possible and we won't lost a frame at beginning. To chfremer@, I measured the timestamp on platforms, t1 for gUM request to VCI, t2 for OnStarted in VCD and t3 for the first frame captured. The typical gaps between t1--t2 and t2--t3 are 100--300 ms. So t2 is the earliest time we should return gUM request. t3 may be too late already, especially it's not in our control that when we can get the first frame(for the worst case). To miu@, as to screen capture, is there any automatic refresh mechanism currently? Or we have to manually refresh capture? If this is OK, I can implement it in the next cl. This cl is only about reporting OnStarted event. WDYT?
On 2017/02/18 02:32:26, chfremer wrote: > 1.) We could take the first arriving frame as a signal of a successful start and > wait until then to report the success to the caller of getUserMedia. braveyao@: > You are saying that this would be too late. Why is that? As far as I know, > getUserMedia is already promise-based, so it should be acceptable to take a > while to respond. This approach would be my preference, since it simplifies ALL the VCD impls. (They can just assume AllocateAndStart() means frames can be delivered anytime thereafter.) Thinking about this more, it would be acceptable for frames to be dropped. If that is done in the render process, the renderer can request a refresh frame once it has processed the "start event." This will be cheap since, for the screen capture impls, they'll just resurrect the last buffer rather than do an expensive capture. > 2.) We could keep the OnStarted() and also the contract that OnStarted() is > called before the first frame is delivered. miu@, I understand the concern about > dropping a first frame when no subsequent frame may be sent after it for a long > time. But that should only be the case for content capture implementations > (tab/desktop/canvas/window). And those implementations should easily be able to > coordinate things such that they do not sent that first frame before OnStarted() > has been called. Is there a similar concern w.r.t. dropping frames for > webcam-like devices? This is reasonable, but requires VCD impls to do more. Mainly, we shouldn't add requirements to the VCDs because of the behavior of one consumer (in a distant code module) unless there's no cleaner/simpler alternative. I'm not against this approach if #1 wouldn't work well.
On 2017/02/18 19:05:37, braveyao wrote: > Ah, I got an idea: > Keep VCD as is, except reporting OnStarted. > In case that first frame arrives earlier than Onstarted (though I can't see why > it's possible), we can mark it down. Then after OnStarted arrives, we check the > mark and ask for a frame refresh if there is any frame dropping(I don't think > caching the frame is good idea here). As to camera capture, this request can be > ignored because there's always periodically frame-capture. As to screen capture, > we can utilize the existing refresh mechanism to deliver another frame if > needed. Sounds like what I just suggested in my last reply to chfremer@. :) Except, I still don't understand why OnStarted() is needed? Can't gUM just assume the first frame is a "successful start" and drop the frame? Then, it could later request the refresh frame to ensure the application has up-to-date content? Why else would OnStarted() be needed? > To miu@, as to screen capture, is there any automatic refresh mechanism > currently? Or we have to manually refresh capture? Depending on the code, there's an interface in the render process for doing exactly this. https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #7 (id:240047) 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 checked by braveyao@chromium.org to run a CQ dry run
Patchset #7 (id:280001) 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...
On 2017/02/18 21:49:03, miu wrote: > On 2017/02/18 19:05:37, braveyao wrote: > > Ah, I got an idea: > > Keep VCD as is, except reporting OnStarted. > > In case that first frame arrives earlier than Onstarted (though I can't see > why > > it's possible), we can mark it down. Then after OnStarted arrives, we check > the > > mark and ask for a frame refresh if there is any frame dropping(I don't think > > caching the frame is good idea here). As to camera capture, this request can > be > > ignored because there's always periodically frame-capture. As to screen > capture, > > we can utilize the existing refresh mechanism to deliver another frame if > > needed. > > Sounds like what I just suggested in my last reply to chfremer@. :) Except, I > still don't understand why OnStarted() is needed? Can't gUM just assume the > first frame is a "successful start" and drop the frame? Then, it could later > request the refresh frame to ensure the application has up-to-date content? Why > else would OnStarted() be needed? > > > To miu@, as to screen capture, is there any automatic refresh mechanism > > currently? Or we have to manually refresh capture? > > Depending on the code, there's an interface in the render process for doing > exactly this. > https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide... It's not good to utilize the first frame as a successful start because: JS app will usually pend on gUM successful callback to start a call session. So we should notify gUM that device is started ASAP. There will be 100 - 300 ms delay between the moment OnStarted in this cl and the first frame captured in normal cases. That's why I said it's too late. And if for any reason, device starts to deliver frame quite late, gUM will pend even longer. Another reason that OnStarted() is necessary is because devices will be started asynchronously on some platforms. So we can't know the result from AllocalteAndStart() and an extra OnStarted report is wanted here. Combine the above two situations, the OnStarted added in this cl is the earliest moment we can return gUM with real device start status. And I will check how to refresh screen capture frames with the given method in my next cl, which is about to handle the OnStarted event reported in this cl.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/21 18:41:23, braveyao wrote: > On 2017/02/18 21:49:03, miu wrote: > > On 2017/02/18 19:05:37, braveyao wrote: > > > Ah, I got an idea: > > > Keep VCD as is, except reporting OnStarted. > > > In case that first frame arrives earlier than Onstarted (though I can't see > > why > > > it's possible), we can mark it down. Then after OnStarted arrives, we check > > the > > > mark and ask for a frame refresh if there is any frame dropping(I don't > think > > > caching the frame is good idea here). As to camera capture, this request can > > be > > > ignored because there's always periodically frame-capture. As to screen > > capture, > > > we can utilize the existing refresh mechanism to deliver another frame if > > > needed. > > > > Sounds like what I just suggested in my last reply to chfremer@. :) Except, I > > still don't understand why OnStarted() is needed? Can't gUM just assume the > > first frame is a "successful start" and drop the frame? Then, it could later > > request the refresh frame to ensure the application has up-to-date content? > Why > > else would OnStarted() be needed? > > > > > To miu@, as to screen capture, is there any automatic refresh mechanism > > > currently? Or we have to manually refresh capture? > > > > Depending on the code, there's an interface in the render process for doing > > exactly this. > > > https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide... > > It's not good to utilize the first frame as a successful start because: > JS app will usually pend on gUM successful callback to start a call session. So > we should notify gUM that device is started ASAP. There will be 100 - 300 ms > delay between the moment OnStarted in this cl and the first frame captured in > normal cases. That's why I said it's too late. And if for any reason, device > starts to deliver frame quite late, gUM will pend even longer. > Another reason that OnStarted() is necessary is because devices will be started > asynchronously on some platforms. So we can't know the result from > AllocalteAndStart() and an extra OnStarted report is wanted here. > > Combine the above two situations, the OnStarted added in this cl is the earliest > moment we can return gUM with real device start status. > > And I will check how to refresh screen capture frames with the given method in > my next cl, which is about to handle the OnStarted event reported in this cl. Brave and I discussed offline. We went over the following three proposed solutions and discussed advantages and disadvantages: 1.) As soon as AllocateAndStart() is invoked, VideoCaptureDevice may send OnStarted and Frames, and it may do so in any order, even concurrently. VideoCaptureDeviceClient serializes the calls onto the Browser IO Thread. If frames arrived before OnStarted, VideoCaptureDeviceImpl can drop them and send a RequestRefreshFrame as a reaction to OnStarted. 2.) As soon as AllocateAndStart() is invoked, VideoCaptureDevice may send OnStarted and Frames, but it guarantees that they are not called concurrently and that OnStarted is called before the first frame is sent. 3.) As soon as AllocateAndStart() is invoked, VideoCaptureDevice may send Frames. Clients interpret the arrival of the first frame as a signal that the device has started successully. Solution 3 has the most simple interface and contract, but it also has the longest delay between the getUserMedia request and our response. The delay is smaller and the same for Solutions 1 and 2. The downside of Solution 2 is that it puts stricter requirements on VideoCaptureDevice implementations, which requires extra complexity in all VideoCaptureDevice implementations. The downside of Solution 1 is that it allows things to happen in more than one order, which makes the interactions less well-defined and requries all downstream components to handle all cases correctly. This means more cases that need to be tested. After consulting niklase@ for input, we decided to go for Solution 1. The reason to not go with Solution 3 is because we give user experience (shorted delay) higher weight than code simplicity. The reason to choose Solution 1 over 2 is to keep complexity out of the VideoCaptureDevice implementations. miu@: please let us know if you agree with the decision.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #7 (id:300001) 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
PS#7 is based on the final decision made. PTAL! https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_unittest.cc:251: .Times(AtMost(1)); On 2017/02/17 23:51:14, chfremer wrote: > On 2017/02/17 20:37:20, braveyao wrote: > > On 2017/02/16 01:00:50, chfremer wrote: > > > On 2017/02/15 21:47:15, braveyao wrote: > > > > On 2017/02/14 19:21:22, chfremer wrote: > > > > > On 2017/02/14 01:05:49, braveyao wrote: > > > > > > On 2017/02/09 23:34:52, chfremer wrote: > > > > > > > Do we really want to modify the contract between VideoCaptureHost > and > > > > > > > VideoCaptureObserver such that it is legal for VideoCaptureHost to > > > report > > > > > > > STOPPED without first reporting STARTED? > > > > > > > As a user of the interface that would feel unexpected to me. > > > > > > > If a stop request can cancel a start request before it has > completed, > > > > could > > > > > we > > > > > > > make it that in that case neither STARTED nor STOPPED is reported? > > > > > > > > > > > > > > I would prefer that we guarantee that we get either a) STARTED > > followed > > > by > > > > > > > STOPPED or b) no state changes at all. > > > > > > > > > > > > It's natural to me that a stop request can cancel a start request > before > > > it > > > > > has > > > > > > completed. And at this point, only the STOPPED report will be > concerned, > > > as > > > > > the > > > > > > STARTED is not important any more. > > > > > > WDTY? > > > > > > > > > > I think it is confusing for the listener when a STOPPED event can arrive > > > > without > > > > > any preceding STARTED event. If starting is aborted without any visible > > > effect > > > > > > > > > for the listener, there is no reason to send any notification to the > > > listener, > > > > > > > > > not even a STOPPED. If we do want to notify listeners about the aborted > > > attemt > > > > > > > > > to start, we should send a STARTING event followed by a STARTING_ABORTED > > > > event. > > > > > > > > As we discussed, the current processing is OK. Keep it as is. > > > > Let me know if there is any other concern. > > > > > > As per our offline discussion, I understand that the listener requires > > > the STOPPED event to tear down state that it creates even before requesting > > > to start the device. > > > > > > In that case, let us make the contract as clear as possible. > > > It seems that if STARTED is called, it must be called before STOPPED. > > > We should express that in this test using gmock's InSequence. > > I couldn't find this change in PatchSet#6. > Also note that the InSequence constraint should probably go to all the unit > tests that check for the arrival of the STARTED and STOPPED events, not just > this one. > > > > > Then, in the file video_capture.mojom, there is a description comment > > > outlining the protocol of communication between Observer and > VideoCaptureHost. > > > We should make a note there that it is legal that no STARTED event is > > > sent in case a Stop request cancels the preceding Start request. > > > > > I also couldn't find this change in PatchSet#6. > > > > Done. > "InSequence" is in this function already, at line 241. And it can't be added into the "StartCapture" above, which will cause bots error on some platforms. https://codereview.chromium.org/2673373003/diff/250001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_event_handler.h (right): https://codereview.chromium.org/2673373003/diff/250001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_event_handler.h:54: virtual void OnStarted(VideoCaptureControllerID id) = 0; On 2017/02/17 23:51:14, chfremer wrote: > The second sentence of the comment for this method concerns several other > methods. It would probably not be found by a reader only looking at OnError or > OnEnded. Therefore it should go to the class description. Could you please move > it to there? Done. https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... File media/capture/content/thread_safe_capture_oracle.cc (right): https://codereview.chromium.org/2673373003/diff/250001/media/capture/content/... media/capture/content/thread_safe_capture_oracle.cc:230: if (!client_ || !capture_) On 2017/02/18 01:18:45, miu wrote: > Dropping frames at this stage could break things (there were several similar > bugs in the past related to dropping frames). > > I thought about this whole change a bit more, and I'm not convinced this is the > right overall approach to solving the problem (as I understand it). > > I think you should remove this logic (and in all the other VCD impls). We > shouldn't require that OnStarted() be called before any frames can be delivered. > This requirement was never necessary before. Instead, the API contract was that > frames could be delivered at any point once AllocateAndStart() is called. > > If it is a requirement for gUM, then gUM should drop frames at the consumer end. > But, that might be a very bad idea: dropping *any* frames can break things > because not all VCD's continuously produce frames. In other words, there are > start-up scenarios where a producer will send only one frame and then no others > for a long period of time, and if that first frame is dropped, consumers will > never see a first frame. (again, there were bugs relating to this) > > Stepping back, I wonder if it'd be simpler to just scrap all the browser-side > changes. Remove OnStarted() altogether. Instead, in the render process, could > gUM simply infer that receiving a first frame means the capture device > successfully started? The web platform event for "started capture" could be > dispatched at the time the first frame arrives at the consumer. Acknowledged. https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:414: mIsRunning = true; On 2017/02/17 23:51:15, chfremer wrote: > I am not sure we can move mIsRunning = true out from the scope that has > mPreviewBufferLock locked. > Otherwise, setting mIsRunning = true may happen concurrently with the check if > (!mIsRunning) in method onPreviewFrame(). I don't know Java well enough to know > whether or not bools are safe to use concurrently. Could you please confirm that > this is intended and valid? Done. https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/ma... File media/capture/video/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/ma... media/capture/video/mac/video_capture_device_mac.mm:410: if (state_ != kCapturing) On 2017/02/17 23:51:15, chfremer wrote: > Unless this runs on the same thread as AllocateAndStart(), this check may happen > concurrently with AllocateAndStart() changing |state_| to kCapturing. I don't > know if this is a legal atomic operation or if it needs to be synchronized using > locks. Could you please check/confirm? Done. Tried to apply std::atomic<> to all other places. https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/wi... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2673373003/diff/250001/media/capture/video/wi... media/capture/video/win/video_capture_device_win.cc:461: if (state_ != kCapturing) On 2017/02/17 23:51:15, chfremer wrote: > Unless this runs on the same thread as AllocateAndStart(), this check may happen > concurrently with AllocateAndStart() changing |state_| to kCapturing. I don't > know if this is a legal atomic operation or if it needs to be synchronized using > locks. Could you please check/confirm? Done.
Overall I think this approach should work well. Just two more code questions and a few suggestions for what to write in the descriptions. https://codereview.chromium.org/2673373003/diff/320001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2673373003/diff/320001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:476: state_ = VIDEO_CAPTURE_STATE_STARTED; Why are we setting this here? Looking at the usages of |state_|, it only ever takes one of two possible values, VIDEO_CAPTURE_STATE_STARTED and VIDEO_CAPTURE_STATE_ERROR. This means, what this really is is just a flag indicating whether or not OnError() was received (I would actually recommend changinge |state_| to a boolean and naming it |error_flag_| or |is_in_error_state_|). Before this CL, it looks like there was not way to change back from VIDEO_CAPTURE_STATE_ERROR to VIDEO_CAPTURE_STATE_STARTED, and I don't see why we would intend to change this here. The only way to recover from an error state is to discard and re-create the device. https://codereview.chromium.org/2673373003/diff/320001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_event_handler.h (right): https://codereview.chromium.org/2673373003/diff/320001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_event_handler.h:29: // jobs are done before they are handled. directly -> synchronously (to use the more precise technical term) Since we are specifying this for some of the methods, a reader will naturally also want to know about the remaining methods OnBufferCreated, OnBufferDestroyed, OnBufferReady? Can they be forwarded synchronously? https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:409: } Why are we moving this to under the mPreviewBufferLock if this was not needed before? https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/vi... media/capture/video/video_capture_device.h:88: // as soon as AllocateAndStart() is called, despite of OnStarted processing. As discussed offline, the "All clients must implement ..." is not meaningful, since the language already enforces that purely virtual methods must be implemented. Please remove that part. The "despite of OnStarted processing" part is hard to understand. I would write something like this: All methods may be called as soon as AllocateAndStart() of the corresponding VideoCaptureDevice is invoked. The methods for buffer reservation and frame delivery may be called from arbitrary threads but are guaranteed to be called non-concurrently. The status reporting methods (OnStarted, OnLog, OnError) may be called concurrently. https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/vi... media/capture/video/video_capture_device.h:223: // after AllocateAndStart() has returned. According to the latest design, it may actually be called even _before_ AllocateAndStart() has returned. It may be called as soon as AllocateAndStart() is invoked. And since we are now explaining the contract of when this may be called in the class description (at the top), we no longer need to specify it here. Please remove.
The CQ bit was checked by braveyao@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 checked by braveyao@chromium.org to run a CQ dry run
Patchset #8 (id:340001) 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...
Hi chfremer@, thanks for the suggestions. All addressed. PTAL! https://codereview.chromium.org/2673373003/diff/320001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2673373003/diff/320001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:476: state_ = VIDEO_CAPTURE_STATE_STARTED; On 2017/02/22 19:13:58, chfremer wrote: > Why are we setting this here? > > Looking at the usages of |state_|, it only ever takes one of two possible > values, VIDEO_CAPTURE_STATE_STARTED and VIDEO_CAPTURE_STATE_ERROR. This means, > what this really is is just a flag indicating whether or not OnError() was > received (I would actually recommend changinge |state_| to a boolean and naming > it |error_flag_| or |is_in_error_state_|). > > Before this CL, it looks like there was not way to change back from > VIDEO_CAPTURE_STATE_ERROR to VIDEO_CAPTURE_STATE_STARTED, and I don't see why we > would intend to change this here. The only way to recover from an error state is > to discard and re-create the device. Done. You're right. No need here. https://codereview.chromium.org/2673373003/diff/320001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_event_handler.h (right): https://codereview.chromium.org/2673373003/diff/320001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_event_handler.h:29: // jobs are done before they are handled. On 2017/02/22 19:13:58, chfremer wrote: > directly -> synchronously (to use the more precise technical term) > > Since we are specifying this for some of the methods, a reader will naturally > also want to know about the remaining methods OnBufferCreated, > OnBufferDestroyed, OnBufferReady? > Can they be forwarded synchronously? Done. https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:409: } On 2017/02/22 19:13:58, chfremer wrote: > Why are we moving this to under the mPreviewBufferLock if this was not needed > before? Done. No it's not needed. Just didn't want to take lock twice. https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/vi... media/capture/video/video_capture_device.h:88: // as soon as AllocateAndStart() is called, despite of OnStarted processing. On 2017/02/22 19:13:58, chfremer wrote: > As discussed offline, the "All clients must implement ..." is not meaningful, > since the language already enforces that purely virtual methods must be > implemented. Please remove that part. > > The "despite of OnStarted processing" part is hard to understand. I would write > something like this: > All methods may be called as soon as AllocateAndStart() of the corresponding > VideoCaptureDevice is invoked. The methods for buffer reservation and frame > delivery may be called from arbitrary threads but are guaranteed to be called > non-concurrently. The status reporting methods (OnStarted, OnLog, OnError) may > be called concurrently. Done. https://codereview.chromium.org/2673373003/diff/320001/media/capture/video/vi... media/capture/video/video_capture_device.h:223: // after AllocateAndStart() has returned. On 2017/02/22 19:13:58, chfremer wrote: > According to the latest design, it may actually be called even _before_ > AllocateAndStart() has returned. It may be called as soon as AllocateAndStart() > is invoked. > > And since we are now explaining the contract of when this may be called in the > class description (at the top), we no longer need to specify it here. Please > remove. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PS8 lgtm % comment for VideoCaptureCamera.java Thanks for putting in the effort to address all the suggestions. https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:418: mIsRunning = true; Why are we moving the mIsRunning = true down here? I thought this was only needed if the contract was that no frames are allowed to be sent before OnStarted(). Now that we agreed to allow frames being sent before OnStarted(), this should no longer be needed. Couldn't we just call nativeOnStarted() in line 410 right after mCamera.startPreview()?
Thanks chfremer@ for all the help here. Learnt a lot :) And the comment is answered. miu@, Please take another look. https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:418: mIsRunning = true; On 2017/02/23 17:49:53, chfremer wrote: > Why are we moving the mIsRunning = true down here? I thought this was only > needed if the contract was that no frames are allowed to be sent before > OnStarted(). Now that we agreed to allow frames being sent before OnStarted(), > this should no longer be needed. Couldn't we just call nativeOnStarted() in line > 410 right after mCamera.startPreview()? If startPreview fails, it's not right to set mIsRunning = true.
https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/an... File media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/an... media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:418: mIsRunning = true; On 2017/02/23 18:08:09, braveyao wrote: > On 2017/02/23 17:49:53, chfremer wrote: > > Why are we moving the mIsRunning = true down here? I thought this was only > > needed if the contract was that no frames are allowed to be sent before > > OnStarted(). Now that we agreed to allow frames being sent before OnStarted(), > > this should no longer be needed. Couldn't we just call nativeOnStarted() in > line > > 410 right after mCamera.startPreview()? > > If startPreview fails, it's not right to set mIsRunning = true. Ah, I see. Then this is a fix for a bug that existed before and is unrelated to this CL? If so, should we create a bug entry for visibility and tracking?
On 2017/02/21 18:41:23, braveyao wrote: > Another reason that OnStarted() is necessary is because devices will be started > asynchronously on some platforms. So we can't know the result from > AllocalteAndStart() and an extra OnStarted report is wanted here. > > Combine the above two situations, the OnStarted added in this cl is the earliest > moment we can return gUM with real device start status. Ok, I'm convinced now. My other suggestion was going to be: Have AllocateAndStart() return a "success" bool. However, if some platforms do async starts, then I suppose we can't go that route. > And I will check how to refresh screen capture frames with the given method in > my next cl, which is about to handle the OnStarted event reported in this cl. This would only be necessary if frames are ever dropped. FWIW, for WebRTC, there already is a forced-refresh mechanism in-place (mostly as a workaround for a design flaw in the bandwidth detection logic): https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/media_stre...
Was about to sign-off in this, but there seems to be a threading issue in the interface/impls: https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/vi... media/capture/video/video_capture_device.h:90: // (OnStarted, OnLog, OnError) may be called concurrently. FWICT, this is incorrect: The current implementation, media::VideoCaptureDeviceClient blindly calls into its media::VideoFrameReceiver. However, content::VideoCaptureController (the main impl of VideoFrameReceiver) assumes the status-reporting methods are *only* called on the IO thread. Also, we then have examples where the VCD's do not call these methods on the IO thread, such as DesktopCaptureDevice: It calls OnError() from its own task runner: https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... So, we need to fix this. As it is right now, DCHECKs() will trigger under normal conditions. Please make sure, when you are testing this change on your desktop, that you are using "dcheck_always_on = true" in your GN args.
https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/vi... media/capture/video/video_capture_device.h:90: // (OnStarted, OnLog, OnError) may be called concurrently. On 2017/02/24 22:16:45, miu wrote: > FWICT, this is incorrect: The current implementation, > media::VideoCaptureDeviceClient blindly calls into its > media::VideoFrameReceiver. However, content::VideoCaptureController (the main > impl of VideoFrameReceiver) assumes the status-reporting methods are *only* > called on the IO thread. > > Also, we then have examples where the VCD's do not call these methods on the IO > thread, such as DesktopCaptureDevice: It calls OnError() from its own task > runner: > https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... > > So, we need to fix this. As it is right now, DCHECKs() will trigger under normal > conditions. Please make sure, when you are testing this change on your desktop, > that you are using "dcheck_always_on = true" in your GN args. Thanks for raising this concern. I think we may actually be fine, because the VideoFrameReceiver implementation that the VideoCaptureDeviceClient talks to is not VideoCaptureController but VideoFrameReceiverOnIOThread, which is a decorator that posts all incoming calls to the IO thread.
Hi miu@, thanks for the concern. Please check the answers. And I've tried the refreshframe mechanism if frame comes before OnStarted event, in https://codereview.chromium.org/2658643002/diff/280001/content/renderer/media.... It works as expected. https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/vi... media/capture/video/video_capture_device.h:90: // (OnStarted, OnLog, OnError) may be called concurrently. On 2017/02/24 22:25:35, chfremer wrote: > On 2017/02/24 22:16:45, miu wrote: > > FWICT, this is incorrect: The current implementation, > > media::VideoCaptureDeviceClient blindly calls into its > > media::VideoFrameReceiver. However, content::VideoCaptureController (the main > > impl of VideoFrameReceiver) assumes the status-reporting methods are *only* > > called on the IO thread. > > > > Also, we then have examples where the VCD's do not call these methods on the > IO > > thread, such as DesktopCaptureDevice: It calls OnError() from its own task > > runner: > > > https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... > > > > So, we need to fix this. As it is right now, DCHECKs() will trigger under > normal > > conditions. Please make sure, when you are testing this change on your > desktop, > > that you are using "dcheck_always_on = true" in your GN args. > > Thanks for raising this concern. I think we may actually be fine, because the > VideoFrameReceiver implementation that the VideoCaptureDeviceClient talks to is > not VideoCaptureController but VideoFrameReceiverOnIOThread, which is a > decorator that posts all incoming calls to the IO thread. Thanks chfremer@ for replying. Yes, they are all posted to IO thread here, https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide..., before going into VideoCaptureController.
lgtm https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/vi... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/vi... media/capture/video/video_capture_device.h:90: // (OnStarted, OnLog, OnError) may be called concurrently. On 2017/02/24 22:53:36, braveyao wrote: > On 2017/02/24 22:25:35, chfremer wrote: > > On 2017/02/24 22:16:45, miu wrote: > > > FWICT, this is incorrect: The current implementation, > > > media::VideoCaptureDeviceClient blindly calls into its > > > media::VideoFrameReceiver. However, content::VideoCaptureController (the > main > > > impl of VideoFrameReceiver) assumes the status-reporting methods are *only* > > > called on the IO thread. > > > > > > Also, we then have examples where the VCD's do not call these methods on the > > IO > > > thread, such as DesktopCaptureDevice: It calls OnError() from its own task > > > runner: > > > > > > https://cs.chromium.org/chromium/src/content/browser/media/capture/desktop_ca... > > > > > > So, we need to fix this. As it is right now, DCHECKs() will trigger under > > normal > > > conditions. Please make sure, when you are testing this change on your > > desktop, > > > that you are using "dcheck_always_on = true" in your GN args. > > > > Thanks for raising this concern. I think we may actually be fine, because the > > VideoFrameReceiver implementation that the VideoCaptureDeviceClient talks to > is > > not VideoCaptureController but VideoFrameReceiverOnIOThread, which is a > > decorator that posts all incoming calls to the IO thread. > > Thanks chfremer@ for replying. > Yes, they are all posted to IO thread here, > https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide..., > before going into VideoCaptureController. Ah, yes, I forgot about that.
braveyao@chromium.org changed reviewers: + dcheng@chromium.org
+ dcheng@, for OWNER's review to services/video_capture/public/interfaces/receiver.mojom. Thanks a lot!
LGTM with nits addressed Also, please fix typos in CL description meida => media, foucs => focus, and perhaps provide a brief summary of issues with the previous approach in the CL description (it states that it causes several kinds of problems, but doesn't mention at all what they are) https://codereview.chromium.org/2673373003/diff/360001/content/browser/media/... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/360001/content/browser/media/... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:638: auto client = client_observer()->PassClient(); Nit: I don't find the return type particularly obvious here, so please write it out (ditto for 652, 698, etc) https://codereview.chromium.org/2673373003/diff/360001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_event_handler.h (right): https://codereview.chromium.org/2673373003/diff/360001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_event_handler.h:27: // OnError&OnEnded need to be scheduled to the end of message queue to OnError and OnEnded (Otherwise, this is a bit harder to read, since it's not clear if & means bitwise and or not without reading more) https://codereview.chromium.org/2673373003/diff/360001/content/browser/render... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/360001/content/browser/render... content/browser/renderer_host/media/video_capture_unittest.cc:248: // |STARTED| is reported asynchronously, which may not be got if capture Nit: got => received or got => called Ditto elsewhere https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/fa... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/fa... media/capture/video/fake_video_capture_device_unittest.cc:556: auto client = CreateClient(); Ditto about auto: it's not really obvious what CreateClient() is returning, IMO.
The CQ bit was checked by braveyao@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...
Description was changed from ========== getUserMeida: report device starting states Chromium used to report device started immediately at requesting user meida, despite the device operation results. This causes several kinds of problems. So we decide to make gUM return the real device starting states (here we foucs on the video capturing). This is the first part of the complete modification. The design doc is here, https://goo.gl/WTZkEl The complete CL is here, https://codereview.chromium.org/2658643002 This CL implements the device-started-state reporting part. Each device will report OnStarted event all the way back to VideoCaptureImpl when it's started successfully(Error report has been done already). This CL also includes all the necessary modifications due to the interface change. This CL will take no effect to getUserMedia behavior at present until another CL which will handle the OnStarted event is landed. BUG=674959, 651910 ========== to ========== getUserMeida: report device starting states Chromium used to report device started immediately at requesting user media, despite the device operation results. This causes several kinds of problems. - Notification of capture starting is shown before device starts. - gUM gets successful callback even if the device fails to start. So we decide to make gUM return the real device starting states (here we focus on the video capturing). This is the first part of the complete modification. The design doc is here, https://goo.gl/WTZkEl The complete CL is here, https://codereview.chromium.org/2658643002 This CL implements the device-started-state reporting part. Each device will report OnStarted event all the way back to VideoCaptureImpl when it's started successfully(Error report has been done already). This CL also includes all the necessary modifications due to the interface change. This CL will take no effect to getUserMedia behavior at present until another CL which will handle the OnStarted event is landed. BUG=674959, 651910 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 braveyao@chromium.org to run a CQ dry run
Patchset #9 (id:380001) 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: This issue passed the CQ dry run.
The CQ bit was checked by braveyao@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.
Thanks so much dcheng@. All comments addressed/answered. https://codereview.chromium.org/2673373003/diff/360001/content/browser/media/... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/360001/content/browser/media/... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:638: auto client = client_observer()->PassClient(); On 2017/02/25 07:27:03, dcheng wrote: > Nit: I don't find the return type particularly obvious here, so please write it > out (ditto for 652, 698, etc) auto is used a lot in this unittest. And in this cl the only purpose is to add a EXPECT_CALL to the client obj. Its return type is not the concern here. So I think it's OK. https://codereview.chromium.org/2673373003/diff/360001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_event_handler.h (right): https://codereview.chromium.org/2673373003/diff/360001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_event_handler.h:27: // OnError&OnEnded need to be scheduled to the end of message queue to On 2017/02/25 07:27:03, dcheng wrote: > OnError and OnEnded > > (Otherwise, this is a bit harder to read, since it's not clear if & means > bitwise and or not without reading more) Done. https://codereview.chromium.org/2673373003/diff/360001/content/browser/render... File content/browser/renderer_host/media/video_capture_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/360001/content/browser/render... content/browser/renderer_host/media/video_capture_unittest.cc:248: // |STARTED| is reported asynchronously, which may not be got if capture On 2017/02/25 07:27:03, dcheng wrote: > Nit: got => received or got => called > > Ditto elsewhere Done. https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/fa... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/fa... media/capture/video/fake_video_capture_device_unittest.cc:556: auto client = CreateClient(); On 2017/02/25 07:27:03, dcheng wrote: > Ditto about auto: it's not really obvious what CreateClient() is returning, IMO. auto is used a lot in this unittest. And in this cl the only purpose is to add a EXPECT_CALL to client obj. Its type is not the concern here. So I think it's OK.
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, dcheng@chromium.org, chfremer@chromium.org Link to the patchset: https://codereview.chromium.org/2673373003/#ps420001 (title: "address comments on PS#8")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/fa... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/fa... media/capture/video/fake_video_capture_device_unittest.cc:556: auto client = CreateClient(); On 2017/02/27 23:18:53, braveyao wrote: > On 2017/02/25 07:27:03, dcheng wrote: > > Ditto about auto: it's not really obvious what CreateClient() is returning, > IMO. > > auto is used a lot in this unittest. And in this cl the only purpose is to add a > EXPECT_CALL to client obj. Its type is not the concern here. > So I think it's OK. I understand that, but I think the style in this rest of this file is probably wrong (it should not be using auto, per our style guidelines). It'd be OK to address in a followup, if you want to keep consistency.
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1488237546229220,
"parent_rev": "b4ab2c32985eb0b6af0556c2376a8598e2b0c134", "commit_rev":
"a18b95669ed38c0931818534ce17f131a006e476"}
Message was sent while issue was closed.
Description was changed from ========== getUserMeida: report device starting states Chromium used to report device started immediately at requesting user media, despite the device operation results. This causes several kinds of problems. - Notification of capture starting is shown before device starts. - gUM gets successful callback even if the device fails to start. So we decide to make gUM return the real device starting states (here we focus on the video capturing). This is the first part of the complete modification. The design doc is here, https://goo.gl/WTZkEl The complete CL is here, https://codereview.chromium.org/2658643002 This CL implements the device-started-state reporting part. Each device will report OnStarted event all the way back to VideoCaptureImpl when it's started successfully(Error report has been done already). This CL also includes all the necessary modifications due to the interface change. This CL will take no effect to getUserMedia behavior at present until another CL which will handle the OnStarted event is landed. BUG=674959, 651910 ========== to ========== getUserMeida: report device starting states Chromium used to report device started immediately at requesting user media, despite the device operation results. This causes several kinds of problems. - Notification of capture starting is shown before device starts. - gUM gets successful callback even if the device fails to start. So we decide to make gUM return the real device starting states (here we focus on the video capturing). This is the first part of the complete modification. The design doc is here, https://goo.gl/WTZkEl The complete CL is here, https://codereview.chromium.org/2658643002 This CL implements the device-started-state reporting part. Each device will report OnStarted event all the way back to VideoCaptureImpl when it's started successfully(Error report has been done already). This CL also includes all the necessary modifications due to the interface change. This CL will take no effect to getUserMedia behavior at present until another CL which will handle the OnStarted event is landed. BUG=674959, 651910 Review-Url: https://codereview.chromium.org/2673373003 Cr-Commit-Position: refs/heads/master@{#453388} Committed: https://chromium.googlesource.com/chromium/src/+/a18b95669ed38c0931818534ce17... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:420001) as https://chromium.googlesource.com/chromium/src/+/a18b95669ed38c0931818534ce17...
Message was sent while issue was closed.
https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/fa... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2673373003/diff/360001/media/capture/video/fa... media/capture/video/fake_video_capture_device_unittest.cc:556: auto client = CreateClient(); On 2017/02/27 23:22:31, dcheng wrote: > On 2017/02/27 23:18:53, braveyao wrote: > > On 2017/02/25 07:27:03, dcheng wrote: > > > Ditto about auto: it's not really obvious what CreateClient() is returning, > > IMO. > > > > auto is used a lot in this unittest. And in this cl the only purpose is to add > a > > EXPECT_CALL to client obj. Its type is not the concern here. > > So I think it's OK. > > I understand that, but I think the style in this rest of this file is probably > wrong (it should not be using auto, per our style guidelines). It'd be OK to > address in a followup, if you want to keep consistency. Acknowledged. Addressed in the following up cl. Will add you as reviewer soon. |
