Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815643002:
Make V4L2CaptureDelegate non-ref-counted (Closed)
|
Created:
3 years, 8 months ago by Chandan Modified:
3 years, 8 months ago CC:
chromium-reviews, posciak+watch_chromium.org, chfremer+watch_chromium.org, xjz+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake V4L2CaptureDelegate non-ref-counted
V4L2CaptureDelegate is created and destroyed by VideoCaptureDeviceLinux
only. Hence, its not necessary for refcounting V4L2CaptureDelegate.
BUG=None
Review-Url: https://codereview.chromium.org/2815643002
Cr-Commit-Position: refs/heads/master@{#464927}
Committed: https://chromium.googlesource.com/chromium/src/+/cd729c2a26e840d2bac79edc3b6366f9d319a76b
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added weak pointer to V4L2CaptureDelegate #
Total comments: 6
Patch Set 3 : Addressed review comments #
Total comments: 2
Patch Set 4 : mcasas@ comment #
Messages
Total messages: 28 (15 generated)
c.padhi@samsung.com changed reviewers: + mcasas@chromium.org, posciak@chromium.org
PTAL. Thank you.
mcasas@chromium.org changed reviewers: + chfremer@chromium.org
+chfremer@ for review https://codereview.chromium.org/2815643002/diff/1/media/capture/video/linux/v... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2815643002/diff/1/media/capture/video/linux/v... media/capture/video/linux/v4l2_capture_delegate.cc:510: base::Bind(&V4L2CaptureDelegate::DoCapture, base::Unretained(this))); How would you guarantee that |this| is alive when this Task gets scheduled, if V4L2CaptureDelegate is "created and destroyed on the owner's thread" [1] which is, presumably, different from |v4l2_task_runner_| ? [1] https://cs.chromium.org/chromium/src/media/capture/video/linux/v4l2_capture_d...
https://codereview.chromium.org/2815643002/diff/1/media/capture/video/linux/v... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2815643002/diff/1/media/capture/video/linux/v... media/capture/video/linux/v4l2_capture_delegate.cc:510: base::Bind(&V4L2CaptureDelegate::DoCapture, base::Unretained(this))); On 2017/04/11 17:17:34, mcasas wrote: > How would you guarantee that |this| is alive > when this Task gets scheduled, if V4L2CaptureDelegate > is "created and destroyed on the owner's thread" [1] > which is, presumably, different from > |v4l2_task_runner_| ? > > [1] > https://cs.chromium.org/chromium/src/media/capture/video/linux/v4l2_capture_d... Need to use weak pointer then? What do you suggest?
On 2017/04/11 19:46:43, Chandan wrote: > https://codereview.chromium.org/2815643002/diff/1/media/capture/video/linux/v... > File media/capture/video/linux/v4l2_capture_delegate.cc (right): > > https://codereview.chromium.org/2815643002/diff/1/media/capture/video/linux/v... > media/capture/video/linux/v4l2_capture_delegate.cc:510: > base::Bind(&V4L2CaptureDelegate::DoCapture, base::Unretained(this))); > On 2017/04/11 17:17:34, mcasas wrote: > > How would you guarantee that |this| is alive > > when this Task gets scheduled, if V4L2CaptureDelegate > > is "created and destroyed on the owner's thread" [1] > > which is, presumably, different from > > |v4l2_task_runner_| ? > > > > [1] > > > https://cs.chromium.org/chromium/src/media/capture/video/linux/v4l2_capture_d... > > Need to use weak pointer then? What do you suggest? WeakPtr is a possible solution :-)
Uploaded patch set 2. Please review.
lgtm % update some comments https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.h (right): https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.h:30: // on the owner's thread, otherwise living and operating on |v4l2_task_runner_|. No longer accurate. We are now destroying the instances on the |v4l2_task_runner_| as well, which is essential for the correctness of the usage of the WeakPtrFactory. PLease update the comment. https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:72: params.requested_format.frame_rate, base::Passed(&client))); Please add a comment explaining why the use of base::Unretained() is safe. We should do that either for all sites where we do this, or maybe we can bundle this information into a class-level comment, in order to avoid duplicating the comment several times.
https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:69: base::Unretained(capture_impl_.get()), Get a WeakPtr to |capture_impl_| to be used on |v4l2_thread_| for these PostTask()s.
PTAL. Thank you. https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.h (right): https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.h:30: // on the owner's thread, otherwise living and operating on |v4l2_task_runner_|. On 2017/04/12 17:13:46, chfremer wrote: > No longer accurate. We are now destroying the instances on the > |v4l2_task_runner_| as well, which is essential for the correctness of the usage > of the WeakPtrFactory. PLease update the comment. Done. https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:69: base::Unretained(capture_impl_.get()), On 2017/04/12 17:32:55, mcasas wrote: > Get a WeakPtr to |capture_impl_| to be used on |v4l2_thread_| > for these PostTask()s. Done. https://codereview.chromium.org/2815643002/diff/20001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:72: params.requested_format.frame_rate, base::Passed(&client))); On 2017/04/12 17:13:46, chfremer wrote: > Please add a comment explaining why the use of base::Unretained() is safe. We > should do that either for all sites where we do this, or maybe we can bundle > this information into a class-level comment, in order to avoid duplicating the > comment several times. We now use WeakPtr to |capture_impl_| instead of base::Unretained().
lgtm % my comments and happy bots https://codereview.chromium.org/2815643002/diff/40001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (left): https://codereview.chromium.org/2815643002/diff/40001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:86: capture_impl_ = nullptr; I think we should still keep this guy around, otherwise a cycle of AllocateAndStart() StopAndDeAllocate() AllocateAndStart() <-- will hit the DCHECK in l.53.
c.padhi@samsung.com changed reviewers: + a.suchit@chromium.org
Please review. https://codereview.chromium.org/2815643002/diff/40001/media/capture/video/lin... File media/capture/video/linux/video_capture_device_linux.cc (left): https://codereview.chromium.org/2815643002/diff/40001/media/capture/video/lin... media/capture/video/linux/video_capture_device_linux.cc:86: capture_impl_ = nullptr; On 2017/04/13 17:43:19, mcasas wrote: > I think we should still keep this guy around, otherwise a cycle > of > AllocateAndStart() > StopAndDeAllocate() > AllocateAndStart() <-- will hit the DCHECK in l.53. Done.
The CQ bit was checked by a.suchit@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 mcasas@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...
On 2017/04/17 10:19:31, Chandan wrote: > Please review. > > https://codereview.chromium.org/2815643002/diff/40001/media/capture/video/lin... > File media/capture/video/linux/video_capture_device_linux.cc (left): > > https://codereview.chromium.org/2815643002/diff/40001/media/capture/video/lin... > media/capture/video/linux/video_capture_device_linux.cc:86: capture_impl_ = > nullptr; > On 2017/04/13 17:43:19, mcasas wrote: > > I think we should still keep this guy around, otherwise a cycle > > of > > AllocateAndStart() > > StopAndDeAllocate() > > AllocateAndStart() <-- will hit the DCHECK in l.53. > > Done. still lgtm
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 c.padhi@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from chfremer@chromium.org Link to the patchset: https://codereview.chromium.org/2815643002/#ps60001 (title: "mcasas@ comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492445329872980,
"parent_rev": "66ab38ef527bbc588352b6fe547dab8b28a61c6a", "commit_rev":
"cd729c2a26e840d2bac79edc3b6366f9d319a76b"}
Message was sent while issue was closed.
Description was changed from ========== Make V4L2CaptureDelegate non-ref-counted V4L2CaptureDelegate is created and destroyed by VideoCaptureDeviceLinux only. Hence, its not necessary for refcounting V4L2CaptureDelegate. BUG=None ========== to ========== Make V4L2CaptureDelegate non-ref-counted V4L2CaptureDelegate is created and destroyed by VideoCaptureDeviceLinux only. Hence, its not necessary for refcounting V4L2CaptureDelegate. BUG=None Review-Url: https://codereview.chromium.org/2815643002 Cr-Commit-Position: refs/heads/master@{#464927} Committed: https://chromium.googlesource.com/chromium/src/+/cd729c2a26e840d2bac79edc3b63... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/cd729c2a26e840d2bac79edc3b63... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
