ImageReader should not be closed until all Image have been returned [145956416]

Fixed

Bug

Status Update

No update yet.

Description

ni...@google.com

created issue #1

Dec 10, 2019 02:57AM

Right now in most places where setOnSurfaceDetachedListener() is being used in the UseCases to close the ImageReaders, the ImageReader is immediately closed.

However, if there are outstanding Images which have not yet been closed (for example given out by ImageCapture or ImageAnalysis) then they will become invalid. This will cause problems for users of the public API.

This also affects OEM extensions because they access the Image via the CaptureProcessor.

In ImageCapture, ImageAnalysis, and Preview the ImageReaderProxy can only be closed after the ImageProxy have been return to CameraX ownership before we can safely close the ImageReaderProxy.

Comments

an...@google.com <an...@google.com> #2Apr 14, 2020 02:41AM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

er...@google.com <er...@google.com> #3Apr 16, 2020 05:20PM

Right now there isn't an update. It's a p3 bug marked on our known issues and backlog.

hu...@gmail.com <hu...@gmail.com> #4Apr 16, 2020 11:37PM

In case it helps, my issue of https://issuetracker.google.com/issues/154155377 has been classified as a duplicate of this issue. I'm using CameraX in a production scenario and this is definitely causing me pain. If there are any workarounds then it'd be great to understand them. Thanks.

an...@google.com <an...@google.com> #5Apr 27, 2020 11:46PM

Hi camerax-bugs@google.com, This is a high priority bug. Can you please take a look and respond by tomorrow (or let us know if this should be assigned to someone else)? Thank you.

ni...@google.com <ni...@google.com> May 12, 2020 06:11PM

Reassigned to ni...@google.com.

ni...@google.com <ni...@google.com> May 14, 2020 08:24PM

Accepted by ni...@google.com.

ap...@google.com <ap...@google.com> #6May 15, 2020 01:53AM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit e8c0fe0ed7331933c1277535245d9de33c2303c1
Author: Franklin Wu <nilknarfuw@google.com>
Date: Tue May 12 15:29:25 2020

Fixed issue where ImageProxyReader in ImageAnalysis is closed prematurely

Added the SafeCloseImageReaderProxy which provides a method to wait
until all acquired ImageProxy are closed before finally closing the
underlying ImageReaderProxy.

Switched ImageAnalysis to use SafeCloseImageReaderProxy and also cleaned
up ImageAnalysis to using lambdas.

Relnote: "Fixes crash in ImageAnalysis where the ImageProxy is accessed
after the ImageReaderProxy has been closed. This also makes it so that
all ImageProxy received by the Analyzer must be closed before the
ImageReaderProxy will be closed."

Bug:

b/145956416

b/154155377

b/156357269

Test: androidx.camera.camera2.ImageAnalysisTest
androidx.camera.core.ImageAnalysisTest

Change-Id: I4b29996bcaf7549df6ababf080ea367326ba0dd3

A camera/camera-core/src/androidTest/java/androidx/camera/core/SafeCloseImageReaderProxyTest.java
M camera/camera-core/src/main/java/androidx/camera/core/ImageAnalysis.java
A camera/camera-core/src/main/java/androidx/camera/core/SafeCloseImageReaderProxy.java
M camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageReaderProxy.java

https://android-review.googlesource.com/1309661

hu...@gmail.com <hu...@gmail.com> #7Jun 4, 2020 03:42AM

Confirming this as fixed having been applied to my builds and tested. Thanks for attending to it!

ap...@google.com <ap...@google.com> #8Jul 1, 2020 12:14AM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit d30c289675deea190f654bfba3aa91e5e1ffdeb6
Author: Franklin Wu <nilknarfuw@google.com>
Date: Tue Jun 23 18:14:00 2020

Make the ImageProxy acquired from ImageCapture safe

Safely close the ImageReaderProxy in ImageCapture, by waiting for the
ImageProxy which has been sent out via OnImageCapturedCallback to be
closed before closing the ImageReaderProxy.

Bug:

b/145956416

Test: camera-* unit/robolectric tests
Change-Id: I2b2d31fceecc9c270fdee11dfead77da5bb281e9

M camera/camera-core/src/main/java/androidx/camera/core/ImageCapture.java
M camera/camera-core/src/test/java/androidx/camera/core/ImageCaptureTest.kt
M camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageProxy.java
M camera/camera-testing/src/main/java/androidx/camera/testing/fakes/FakeImageReaderProxy.java

https://android-review.googlesource.com/1346752

ni...@google.com <ni...@google.com> #9Jul 1, 2020 12:51AM

Marked as fixed.

Preview which uses ProcessingSurface is safe since the ImageProxy is always closed after processing is done and the ImageProxy usage is synchronized with closing of ImageReaderProxy. So all UseCases are addressed.