TakePhoto() callback happening on wrong thread |
||||
Issue descriptionThe behavior specification for interface media::VideoCaptureDevice makes a guarantee that callbacks to TakePhoto() are invoked on the same thread as the invocation to TakePhoto() itself [1]. The implementations VideoCaptureDeviceWin [2] and VideoCaptureDeviceMFWin [3] do not currently honor that guarantee. We either need to determine that the guarantee is not needed, and then drop it from the spec, or we need to fix the implementations. We may want to consider adding a test that verifies the guarantee if we decide to keep it. [1] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device.h?q=SetPhotoOptions&l=289 [2] https://cs.chromium.org/chromium/src/media/capture/video/win/video_capture_device_win.cc?l=860 [3] https://cs.chromium.org/chromium/src/media/capture/video/win/video_capture_device_mf_win.cc?l=883
,
Jan 31 2018
TakePhotoCallback (and the others) are a smart callback that takes care of jumping back to the appropriate thread to be Run(): https://cs.chromium.org/chromium/src/content/browser/image_capture/image_capture_impl.cc?type=cs&q=TakePhotoCallback&sq=package:chromium&l=141
,
Jan 31 2018
Re #2: Ah, I see. Passing in these smart callbacks essentially means that the responsibility of specifying a particular thread/sequence is on the caller, and not the implementation. With that, we may be able to drop the guarantee made at [1]. We would only need to make sure that all invocations arriving at the media::VideoCaptureDevice API are okay with that change of contract. But I believe that, aside from tests, ImageCaptureImpl is probably currently the only path that calls into this API. [1] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device.h?l=289
,
Feb 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/81c92b46714dc2433fd149897a4593036096de10 commit 81c92b46714dc2433fd149897a4593036096de10 Author: Réda Housni Alaoui <alaoui.rda@gmail.com> Date: Mon Feb 05 19:37:37 2018 Make FileVideoCaptureDevice implement ImageCapture API Implement GetPhotoState, SetPhotoOptions and TakePhoto in FileVideoCaptureDevice to make TakePhoto works with video file as source. Bug: 806520 , 807441 Change-Id: I487d5760162a13c34b5195d854b4285d0e2753dc Reviewed-on: https://chromium-review.googlesource.com/890739 Commit-Queue: Christian Fremerey <chfremer@chromium.org> Reviewed-by: Christian Fremerey <chfremer@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Cr-Commit-Position: refs/heads/master@{#534462} [modify] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/content/browser/image_capture/image_capture_impl.cc [modify] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/BUILD.gn [modify] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/mojo/BUILD.gn [add] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/mojo/image_capture_types.cc [add] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/mojo/image_capture_types.h [modify] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/video/file_video_capture_device.cc [modify] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/video/file_video_capture_device.h [add] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/video/file_video_capture_device_unittest.cc [modify] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/video/mac/video_capture_device_mac.mm [modify] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/video/video_capture_device.h [modify] https://crrev.com/81c92b46714dc2433fd149897a4593036096de10/media/capture/video/win/video_capture_device_mf_win.cc
,
Feb 9 2018
,
Feb 9 2018
Actually, we have decided to change the behavior contract such that now the callbacks are allowed to happen on any thread. -> Marking as wontfix |
||||
►
Sign in to add a comment |
||||
Comment 1 by chfremer@chromium.org
, Jan 30 2018