-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lightos finish processing photos before teardown #3
base: lightos
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukin87 I can see the photos being processed but I found that the images weren't all saved still
Log.i(LP3_TAG, "Closing CameraSession") | ||
val coroutineScope = CoroutineScope(Dispatchers.Main) | ||
coroutineScope.launch { | ||
for (i in 1..10) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are looping 10 times to total 5 seconds of wait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels safer to have a way out in case it ends up in a weird state but the total wait is TBD depending on if we allow a queue or just single photo processing.
override fun close() { | ||
Log.i(TAG, "Closing CameraSession...") | ||
isDestroyed = true | ||
orientationManager.stopOrientationUpdates() | ||
runOnUiThread { | ||
lifecycleRegistry.currentState = Lifecycle.State.DESTROYED | ||
Log.i(LP3_TAG, "Closing CameraSession") | ||
val coroutineScope = CoroutineScope(Dispatchers.Main) | ||
coroutineScope.launch { | ||
for (i in 1..10) { | ||
Log.i(LP3_TAG, "photosBeingProcessed: ${photosBeingProcessed}") | ||
if (photosBeingProcessed <= 0) { | ||
break; | ||
} | ||
delay(500) | ||
} | ||
Log.i(LP3_TAG, "No more photos to process") | ||
|
||
isDestroyed = true | ||
orientationManager.stopOrientationUpdates() | ||
runOnUiThread { | ||
lifecycleRegistry.currentState = Lifecycle.State.DESTROYED | ||
} | ||
Log.i(LP3_TAG, "Closed CameraSession") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these coroutines parallel? If that's the case it'd be nice to introduce some thread safety mechanisms here.
It's a pretty good case to use a CompletableDeferred and a Mutex instead of a loop to control the flow.
The deferred works like a JS promise, so we just need to wrap the counter in a mutex:
internal val photosBeingProcessed = 0
internal val photosBeingProcessedMutex = Mutex()
internal val allPhotosProcessed = CompletableDeferred()
// these methods would be used in `CameraSession+Photo.kt`:
fun incrementPhotosBeingProcessed() {
mutex.withLock {
photosBeingProcessed++
}
}
fun decrementPhotosBeingProcessed() {
mutex.withLock {
val count = photosBeingProcessed--
if count == 0 {
allPhotosProcessed.complete()
}
}
}
override fun close() {
Log.i(TAG, "Closing CameraSession...")
Log.i(LP3_TAG, "Closing CameraSession")
// we still need to check before waiting, because no photos might have been taken?
if (photosBeingProcessed > 0) {
allPhotosProcessed.wait()
}
Log.i(LP3_TAG, "No more photos to process")
isDestroyed = true
orientationManager.stopOrientationUpdates()
runOnUiThread {
lifecycleRegistry.currentState = Lifecycle.State.DESTROYED
}
Log.i(LP3_TAG, "Closed CameraSession")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right that some thread safety here is a good addition: if a photo completes processing in the moment another is taken, photosBeingProcessed
becomes unknown - the mutex'll prevent that.
But the above logic won't work as intended here though since the first time the queue is emptied, allPhotosProcessed
will be marked completed even if the queue is repopulated later. But not a big deal, the mutex would be thread safe enough
The (new) current proposed approach though will only allow for one photo processing at a time so this implementation will become simpler
Do you have any extra details on this?? |
What
This PR addresses the problem from this ticket: processing a photo takes ~2-5 seconds. If a user closes the camera (by entering the gallery for e.g.) then the camera is torn down, processing interrupted and the photo never gets saved.
This now increases the CameraSession lifecycle so it doesn't get torn down until after any pending photos have finished processing or a timeout (currently 5 seconds) is reached. This has to be done on a background thread otherwise the camera UI freezes until processing is finished.
To Test
yarn install
,yarn run-lp3
etcLP3_Camera
Concerns
This is not a perfect solution and probably needs some more thought. Issues: