Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1477363002:
Add logging of video device details on device open and close. (Closed)
|
Created:
5 years ago by nisse-chromium (ooo August 14) Modified:
5 years ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd logging of video device details on device open and close.
Refactor handling of device_info_cache_, to confine all access to the
IO-thread. There used to be a lookup of the cache on the device thread
in DoStartDeviceOnDeviceThread, causing a race with the non-atomic
assignment in OnDevicesInfoEnumerated.
BUG=406256
Committed: https://crrev.com/cbe916974f8558032775f461b03b33358911f7c2
Cr-Commit-Position: refs/heads/master@{#362976}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed mcasas' comments. #Patch Set 3 : Fixed VideoCaptureManagerTest.OpenNotExisting test. #
Total comments: 1
Messages
Total messages: 34 (16 generated)
Description was changed from ========== Add logging of video device details on device open and close. Refactor handling of device_info_cache_, to confine all access to the IO-thread. There used to be a lookup of the cache on the device thread in DoStartDeviceOnDeviceThread, causing a race with the non-atomic assignment in OnDevicesInfoEnumerated. BUG=406256 ========== to ========== Add logging of video device details on device open and close. Refactor handling of device_info_cache_, to confine all access to the IO-thread. There used to be a lookup of the cache on the device thread in DoStartDeviceOnDeviceThread, causing a race with the non-atomic assignment in OnDevicesInfoEnumerated. This is the same changes as in the previous cl registered to my webrtc account. perkj lgtm:ed it yesterday. BUG=406256 ==========
nisse@chromium.org changed reviewers: + jansson@chromium.org, mcasas@chromium.org, perkj@chromium.org, tommi@chromium.org
Description was changed from ========== Add logging of video device details on device open and close. Refactor handling of device_info_cache_, to confine all access to the IO-thread. There used to be a lookup of the cache on the device thread in DoStartDeviceOnDeviceThread, causing a race with the non-atomic assignment in OnDevicesInfoEnumerated. This is the same changes as in the previous cl registered to my webrtc account. perkj lgtm:ed it yesterday. BUG=406256 ========== to ========== Add logging of video device details on device open and close. Refactor handling of device_info_cache_, to confine all access to the IO-thread. There used to be a lookup of the cache on the device thread in DoStartDeviceOnDeviceThread, causing a race with the non-atomic assignment in OnDevicesInfoEnumerated. BUG=406256 ==========
This is the same changes as in the previous cl registered to my webrtc account. perkj lgtm:ed it yesterday.
lgtm
lgtm
The CQ bit was checked by nisse@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477363002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477363002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Bringing some comments otherwise LGTM. https://codereview.chromium.org/1477363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/1477363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:334: So my suggestion was to add here: base::Callback<scoped_ptr<media::VideoCaptureDevice>( const std::string &, const media::VideoCaptureParams &, scoped_ptr<media::VideoCaptureDevice::Client>)> start_capture_function; then in each case statement update it, e.g. l.350: start_capture_function = base::bind( &VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread, this, found->name, request->params(), base::Passed(entry->video_capture_controller()->NewDeviceClient())); and finally, out of the switch-case: if (start_capture_function.is_null()) return; base::PostTaskAndReplyWithResult( device_task_runner_.get(), FROM_HERE, start_capture_function, base::Bind(&VideoCaptureManager::OnDeviceStarted, this, request->serial_id())); Not sure if it would read better but it'd take 3 similar superstatements out of the cases... up to you. https://codereview.chromium.org/1477363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:501: } l.500 should go after the closing bracket.
https://codereview.chromium.org/1477363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/1477363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:334: On 2015/12/01 15:30:38, mcasas wrote: > So my suggestion was to add here: > > base::Callback<scoped_ptr<media::VideoCaptureDevice>( > const std::string &, const media::VideoCaptureParams &, > scoped_ptr<media::VideoCaptureDevice::Client>)> start_capture_function; > > then in each case statement update it, e.g. l.350: > > start_capture_function = base::bind( > &VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread, this, found->name, > request->params(), > base::Passed(entry->video_capture_controller()->NewDeviceClient())); > > and finally, out of the switch-case: > if (start_capture_function.is_null()) > return; > base::PostTaskAndReplyWithResult( > device_task_runner_.get(), FROM_HERE, start_capture_function, > base::Bind(&VideoCaptureManager::OnDeviceStarted, this, > request->serial_id())); > > Not sure if it would read better but it'd take 3 similar superstatements > out of the cases... up to you. Done. Please have a look and see if it seems right. I don't quite understand the base::Bind machinery. https://codereview.chromium.org/1477363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:501: } On 2015/12/01 15:30:38, mcasas wrote: > l.500 should go after the closing bracket. Done.
The CQ bit was checked by nisse@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477363002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tommi@chromium.org
lgtm https://codereview.chromium.org/1477363002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/1477363002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_manager.cc:334: On 2015/12/02 14:26:00, nisse-chromium wrote: > On 2015/12/01 15:30:38, mcasas wrote: > > So my suggestion was to add here: > > > > base::Callback<scoped_ptr<media::VideoCaptureDevice>( > > const std::string &, const media::VideoCaptureParams &, > > scoped_ptr<media::VideoCaptureDevice::Client>)> start_capture_function; > > > > then in each case statement update it, e.g. l.350: > > > > start_capture_function = base::bind( > > &VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread, this, > found->name, > > request->params(), > > base::Passed(entry->video_capture_controller()->NewDeviceClient())); > > > > and finally, out of the switch-case: > > if (start_capture_function.is_null()) > > return; > > base::PostTaskAndReplyWithResult( > > device_task_runner_.get(), FROM_HERE, start_capture_function, > > base::Bind(&VideoCaptureManager::OnDeviceStarted, this, > > request->serial_id())); > > > > Not sure if it would read better but it'd take 3 similar superstatements > > out of the cases... up to you. > > Done. Please have a look and see if it seems right. I don't quite understand the > base::Bind machinery. This is good
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1477363002/#ps20001 (title: "Addressed mcasas' comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477363002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nisse@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477363002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477363002/40001
Fixed a bug in the reworked error handling. In the VideoCaptureManagerTest.OpenNotExisting, we now get events in the order OnError, Opened and Closed (used to be Opened, OnError, Closed), not sure if that matters or not. The testcases calls vcm->Open (which generates an open event, but routed via PostTask, which delays it), followed by vcm->StartCaptureForclient, which generates the OnError before returning. https://codereview.chromium.org/1477363002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager_unittest.cc (right): https://codereview.chromium.org/1477363002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager_unittest.cc:469: EXPECT_CALL(*listener_, Opened(MEDIA_DEVICE_VIDEO_CAPTURE, _)); Not sure from when the Opened event is called, and why it now occurs after the OnError.
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 nisse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, mcasas@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1477363002/#ps40001 (title: "Fixed VideoCaptureManagerTest.OpenNotExisting test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477363002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477363002/40001
Message was sent while issue was closed.
Description was changed from ========== Add logging of video device details on device open and close. Refactor handling of device_info_cache_, to confine all access to the IO-thread. There used to be a lookup of the cache on the device thread in DoStartDeviceOnDeviceThread, causing a race with the non-atomic assignment in OnDevicesInfoEnumerated. BUG=406256 ========== to ========== Add logging of video device details on device open and close. Refactor handling of device_info_cache_, to confine all access to the IO-thread. There used to be a lookup of the cache on the device thread in DoStartDeviceOnDeviceThread, causing a race with the non-atomic assignment in OnDevicesInfoEnumerated. BUG=406256 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add logging of video device details on device open and close. Refactor handling of device_info_cache_, to confine all access to the IO-thread. There used to be a lookup of the cache on the device thread in DoStartDeviceOnDeviceThread, causing a race with the non-atomic assignment in OnDevicesInfoEnumerated. BUG=406256 ========== to ========== Add logging of video device details on device open and close. Refactor handling of device_info_cache_, to confine all access to the IO-thread. There used to be a lookup of the cache on the device thread in DoStartDeviceOnDeviceThread, causing a race with the non-atomic assignment in OnDevicesInfoEnumerated. BUG=406256 Committed: https://crrev.com/cbe916974f8558032775f461b03b33358911f7c2 Cr-Commit-Position: refs/heads/master@{#362976} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cbe916974f8558032775f461b03b33358911f7c2 Cr-Commit-Position: refs/heads/master@{#362976} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
