New issue
Advanced search Search tips

Issue 711754 link

Starred by 1 user
Status: Fixed
Owner:
chfremer@chromium.org
Closed: May 2017
Cc:
emir...@chromium.org
guidou@chromium.org
niklase@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Add test or remove fallback mechanism in destructor of InProcessLaunchedVideoCaptureDevice

Project Member Reported by chfremer@chromium.org, Apr 14 2017 
The class InProcessLaunchedVideoCaptureDevice operates a media::VideoCaptureDevice on a |device_task_runner_|. In the destructor, there is a fallback mechanism that first tries to schedule a task for stopping the device to |device_task_runner_| and, if that fails, performs the task directly.

It is currently unclear why this fallback mechanism was introduced, if it works, and if it is needed. It does not seem to be covered by any tests. 

Proposal: Either add a test case that demonstrates that it works and does something useful or remove the fallback mechanism.

Comment 1 by chfremer@chromium.org, Apr 14 2017

Components: Blink>GetUserMedia>Webcam

Comment 2 by chfremer@chromium.org, Apr 14 2017

Cc: -chfremer@chromium.org
Owner: chfremer@chromium.org
Status: Started (was: Available)

Comment 3 by chfremer@chromium.org, Apr 14 2017 

I dug a little in the git history. The fallback mechanism was added in [1] but not specific reason was given as for why it is needed. And neither was a test case added for it. Therefore, I assume this was just defensive programming.

Considering that, I think it is fine to remove, and I will do this as part of [2].

[1] https://codereview.chromium.org/801363002/diff/40001/content/browser/renderer_host/media/video_capture_manager.cc
[2] https://codereview.chromium.org/2803483003/
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c914815faeda1e930178e47c3458d387e30b8876

commit c914815faeda1e930178e47c3458d387e30b8876
Author: chfremer <chfremer@chromium.org>
Date: Mon Apr 17 17:10:26 2017

[Mojo Video Capture] Split interface BuildableVideoCaptureDevice into “device launcher” and “launched device”

This is a cleanup/simplification CL.
This CL is supposed to be a pure refactoring. There should be no changes to
existing behavior.

Changes in this CL:
* Split interface BuildableVideoCaptureDevice into interfaces
  VideoCaptureDeviceLauncher and LaunchedVideoCaptureDevice.
* Split implementations accordingly.
* VideoCaptureController is freed from knowledge of
  VideoFrameConsumerFeedbackObserver and VideoCaptureDeviceLauncher::Callbacks.
* Moved mock implementations to a separate file, since they are used by more
  than one test.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL21.

BUG=584797,  711754 
TEST=
  capture_unittests --gtest_filter="*Video*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing

Review-Url: https://codereview.chromium.org/2803483003
Cr-Commit-Position: refs/heads/master@{#464935}

[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/BUILD.gn
[delete] https://crrev.com/0e7bbf149b075810960c71f8eb873d08f3984b89/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc
[delete] https://crrev.com/0e7bbf149b075810960c71f8eb873d08f3984b89/content/browser/renderer_host/media/in_process_buildable_video_capture_device.h
[add] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/in_process_launched_video_capture_device.cc
[add] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/in_process_launched_video_capture_device.h
[add] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc
[add] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/in_process_video_capture_device_launcher.h
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/in_process_video_capture_provider.cc
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/in_process_video_capture_provider.h
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
[add] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/mock_video_capture_provider.cc
[add] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/mock_video_capture_provider.h
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/video_capture_controller.cc
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/video_capture_controller.h
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/video_capture_controller_unittest.cc
[add] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/video_capture_device_launch_observer.h
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/video_capture_manager.cc
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/video_capture_manager.h
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/browser/renderer_host/media/video_capture_provider.h
[modify] https://crrev.com/c914815faeda1e930178e47c3458d387e30b8876/content/test/BUILD.gn

Comment 5 by chfremer@chromium.org, May 23 2017

Status: Fixed (was: Started)
►

Sign in to add a comment