Threading issues in VideoCaptureCamera2.java |
||||||
Issue descriptionIn its current state, the class VideoCaptureCamera2.java involves several threads: 1. The thread from which the constructor and the public API is called. This is typically a video capture thread from the Chromium native code side. 2. The Android application main thread (UI thread), which certain tasks/callbacks are posted to. 3. For each live preview session, a new background thread is started. 4. For each single still image photo session, a new background thread is started. There are several issues with the threading here: * The various member variables of class VideoCaptureCamera2 are accessed across these threads, and there appears to be nothing preventing concurrent access. * Android video capture APIs may get called concurrently as well, and it is not clear whether or not that is safe. * The Loopers of the HandlerThreads that are created at 3. and 4. are never quit, which means the corresponding threads might never quit and the number of threads might build up over time. * Using so many different threads makes it difficult to understanding the behavior of the class.
,
Jul 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5240a33ded02b4e0b90b235dae805e07f5cd39a4 commit 5240a33ded02b4e0b90b235dae805e07f5cd39a4 Author: Christian Fremerey <chfremer@chromium.org> Date: Tue Jul 17 17:21:57 2018 [Image Capture] Add stress test that takes 10 photos in a row The purpose of the added test is to ensure the reliability of the feature as well as detecting flakiness that could be caused by race conditions. The test is marked as MANUAL_ to avoid it running on the regular commit queue trybots. To run the test manually on Android, increasing various timeouts is needed: out/android_release/bin/run_content_browsertests --gtest_filter=*WebRtcImageCaptureStressBrowserTest* --test-launcher-retry-limit=0 --ui-test-action-max-timeout=60000 --shard-timeout=60 The above sets the timeout to 1 minute, which should be more than enough for 10 photos. Bug: 857530 Change-Id: Ibf4f78d43dc3cbe24687710f48bf311f7bb0bfd4 Reviewed-on: https://chromium-review.googlesource.com/1135703 Commit-Queue: Christian Fremerey <chfremer@chromium.org> Reviewed-by: Emircan Uysaler <emircan@chromium.org> Cr-Commit-Position: refs/heads/master@{#575697} [add] https://crrev.com/5240a33ded02b4e0b90b235dae805e07f5cd39a4/content/browser/webrtc/webrtc_stress_image_capture_browsertest.cc [modify] https://crrev.com/5240a33ded02b4e0b90b235dae805e07f5cd39a4/content/test/BUILD.gn [add] https://crrev.com/5240a33ded02b4e0b90b235dae805e07f5cd39a4/content/test/data/media/image_capture_stress_test.html [modify] https://crrev.com/5240a33ded02b4e0b90b235dae805e07f5cd39a4/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87b3a36af3401d404b378f0e294be0ef47779862 commit 87b3a36af3401d404b378f0e294be0ef47779862 Author: Christian Fremerey <chfremer@chromium.org> Date: Wed Jul 18 19:13:09 2018 [Image Capture, Android] Fix various threading issues This CL simplifies the threading of class VideoCaptureCamera2.java and by doing so resolves potential issues caused by concurrent access to member variables and Android video API calls. See https://bugs.chromium.org/p/chromium/issues/detail?id=857530 for details on issues. The simplified model is to (still) have the constructor and public API calls happen on a native thread, and to use a single dedicated thread owned by the class instance to post to, do work on, and call back into the native code. This CL is part of a series, see Design Doc at https://docs.google.com/document/d/1h1kva4VR1gaV3HVXaSYZFY41icfaB58j-WnHmJdyqc8/edit?usp=sharing Bug: 857530 Change-Id: I75ffcc4a14f2395d833d80f300acef7b456676e8 Reviewed-on: https://chromium-review.googlesource.com/1117857 Commit-Queue: Christian Fremerey <chfremer@chromium.org> Reviewed-by: Emircan Uysaler <emircan@chromium.org> Cr-Commit-Position: refs/heads/master@{#576155} [modify] https://crrev.com/87b3a36af3401d404b378f0e294be0ef47779862/media/capture/video/android/java/src/org/chromium/media/VideoCapture.java [modify] https://crrev.com/87b3a36af3401d404b378f0e294be0ef47779862/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java [modify] https://crrev.com/87b3a36af3401d404b378f0e294be0ef47779862/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java [modify] https://crrev.com/87b3a36af3401d404b378f0e294be0ef47779862/media/capture/video/android/video_capture_device_android.cc [modify] https://crrev.com/87b3a36af3401d404b378f0e294be0ef47779862/media/capture/video/android/video_capture_device_android.h
,
Aug 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c8198744b21d85984478c10a765e808ca9b49610 commit c8198744b21d85984478c10a765e808ca9b49610 Author: Christian Fremerey <chfremer@chromium.org> Date: Wed Aug 22 19:29:31 2018 Revert "[Image Capture, Android] Fix various threading issues" This reverts commit 87b3a36af3401d404b378f0e294be0ef47779862. Reason for revert: Causes crash issue https://bugs.chromium.org/p/chromium/issues/detail?id=874745 Original change's description: > [Image Capture, Android] Fix various threading issues > > This CL simplifies the threading of class VideoCaptureCamera2.java > and by doing so resolves potential issues caused by concurrent > access to member variables and Android video API calls. > > See https://bugs.chromium.org/p/chromium/issues/detail?id=857530 for > details on issues. > > The simplified model is to (still) have the constructor and public API > calls happen on a native thread, and to use a single dedicated thread > owned by the class instance to post to, do work on, and call back into > the native code. > > This CL is part of a series, see Design Doc at > https://docs.google.com/document/d/1h1kva4VR1gaV3HVXaSYZFY41icfaB58j-WnHmJdyqc8/edit?usp=sharing > > Bug: 857530 > Change-Id: I75ffcc4a14f2395d833d80f300acef7b456676e8 > Reviewed-on: https://chromium-review.googlesource.com/1117857 > Commit-Queue: Christian Fremerey <chfremer@chromium.org> > Reviewed-by: Emircan Uysaler <emircan@chromium.org> > Cr-Commit-Position: refs/heads/master@{#576155} TBR=emircan@chromium.org,chfremer@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 857530 Change-Id: Ifc02ccd8b8282b0ec31e2f4f36dbe831eee551a2 Reviewed-on: https://chromium-review.googlesource.com/1185262 Reviewed-by: Christian Fremerey <chfremer@chromium.org> Commit-Queue: Christian Fremerey <chfremer@chromium.org> Cr-Commit-Position: refs/heads/master@{#585199} [modify] https://crrev.com/c8198744b21d85984478c10a765e808ca9b49610/media/capture/video/android/java/src/org/chromium/media/VideoCapture.java [modify] https://crrev.com/c8198744b21d85984478c10a765e808ca9b49610/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java [modify] https://crrev.com/c8198744b21d85984478c10a765e808ca9b49610/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java [modify] https://crrev.com/c8198744b21d85984478c10a765e808ca9b49610/media/capture/video/android/video_capture_device_android.cc [modify] https://crrev.com/c8198744b21d85984478c10a765e808ca9b49610/media/capture/video/android/video_capture_device_android.h
,
Aug 22
,
Aug 22
This bug requires manual review: We are only 12 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 23
Approved for merge into 69, branch 3497.
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b6961a63b4e3ce379a0911c7c49fe258357fe7f8 commit b6961a63b4e3ce379a0911c7c49fe258357fe7f8 Author: Christian Fremerey <chfremer@chromium.org> Date: Thu Aug 23 15:54:50 2018 Revert "[Image Capture, Android] Fix various threading issues" This reverts commit 87b3a36af3401d404b378f0e294be0ef47779862. Reason for revert: Causes crash issue https://bugs.chromium.org/p/chromium/issues/detail?id=874745 Original change's description: > [Image Capture, Android] Fix various threading issues > > This CL simplifies the threading of class VideoCaptureCamera2.java > and by doing so resolves potential issues caused by concurrent > access to member variables and Android video API calls. > > See https://bugs.chromium.org/p/chromium/issues/detail?id=857530 for > details on issues. > > The simplified model is to (still) have the constructor and public API > calls happen on a native thread, and to use a single dedicated thread > owned by the class instance to post to, do work on, and call back into > the native code. > > This CL is part of a series, see Design Doc at > https://docs.google.com/document/d/1h1kva4VR1gaV3HVXaSYZFY41icfaB58j-WnHmJdyqc8/edit?usp=sharing > > Bug: 857530 > Change-Id: I75ffcc4a14f2395d833d80f300acef7b456676e8 > Reviewed-on: https://chromium-review.googlesource.com/1117857 > Commit-Queue: Christian Fremerey <chfremer@chromium.org> > Reviewed-by: Emircan Uysaler <emircan@chromium.org> > Cr-Commit-Position: refs/heads/master@{#576155} TBR=emircan@chromium.org,chfremer@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 857530 Change-Id: Ifc02ccd8b8282b0ec31e2f4f36dbe831eee551a2 Reviewed-on: https://chromium-review.googlesource.com/1185262 Reviewed-by: Christian Fremerey <chfremer@chromium.org> Commit-Queue: Christian Fremerey <chfremer@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#585199}(cherry picked from commit c8198744b21d85984478c10a765e808ca9b49610) Reviewed-on: https://chromium-review.googlesource.com/1187041 Cr-Commit-Position: refs/branch-heads/3497@{#787} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/b6961a63b4e3ce379a0911c7c49fe258357fe7f8/media/capture/video/android/java/src/org/chromium/media/VideoCapture.java [modify] https://crrev.com/b6961a63b4e3ce379a0911c7c49fe258357fe7f8/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java [modify] https://crrev.com/b6961a63b4e3ce379a0911c7c49fe258357fe7f8/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java [modify] https://crrev.com/b6961a63b4e3ce379a0911c7c49fe258357fe7f8/media/capture/video/android/video_capture_device_android.cc [modify] https://crrev.com/b6961a63b4e3ce379a0911c7c49fe258357fe7f8/media/capture/video/android/video_capture_device_android.h
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a9e7de87f5afe6f5161e55ea0b4fd15b633057b commit 5a9e7de87f5afe6f5161e55ea0b4fd15b633057b Author: Christian Fremerey <chfremer@chromium.org> Date: Thu Aug 23 23:30:36 2018 Reland [Image Capture, Android] Fix various threading issues This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1185262 Patch Set 1 is the same as the reverted CL % resolved merge conflicts. Patch Set 3 adds a fix for the issue that caused the revert. Issue Analysis: There was an (existing) race condition where video frames could still get delivered from the camera after the device had been stopped and the native object destroyed. The CL made this condition more likely to happen. -------------- Original CL description: This CL simplifies the threading of class VideoCaptureCamera2.java and by doing so resolves potential issues caused by concurrent access to member variables and Android video API calls. See https://bugs.chromium.org/p/chromium/issues/detail?id=857530 for details on issues. The simplified model is to (still) have the constructor and public API calls happen on a native thread, and to use a single dedicated thread owned by the class instance to post to, do work on, and call back into the native code. This CL is part of a series, see Design Doc at https://docs.google.com/document/d/1h1kva4VR1gaV3HVXaSYZFY41icfaB58j-WnHmJdyqc8/edit?usp=sharing Bug: 857530 Change-Id: I7e914fead264990fdd644c32b9ba6e681fa39dec Reviewed-on: https://chromium-review.googlesource.com/1187324 Reviewed-by: Emircan Uysaler <emircan@chromium.org> Commit-Queue: Christian Fremerey <chfremer@chromium.org> Cr-Commit-Position: refs/heads/master@{#585653} [modify] https://crrev.com/5a9e7de87f5afe6f5161e55ea0b4fd15b633057b/media/capture/video/android/java/src/org/chromium/media/VideoCapture.java [modify] https://crrev.com/5a9e7de87f5afe6f5161e55ea0b4fd15b633057b/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java [modify] https://crrev.com/5a9e7de87f5afe6f5161e55ea0b4fd15b633057b/media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java [modify] https://crrev.com/5a9e7de87f5afe6f5161e55ea0b4fd15b633057b/media/capture/video/android/video_capture_device_android.cc [modify] https://crrev.com/5a9e7de87f5afe6f5161e55ea0b4fd15b633057b/media/capture/video/android/video_capture_device_android.h
,
Aug 28
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by chfremer@chromium.org
, Jul 9