Add OCR quality toggle with ML Kit and Tesseract options#1
Conversation
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
|
/gemini code review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively adds Google's ML Kit as a high-quality OCR alternative to Tesseract, introducing a user-configurable quality setting. The implementation of the OcrEngine as a facade to abstract the two OCR backends is a solid design choice. It's also a good decision to continue using Tesseract for PDF processing to retain its broader language support.
My review includes a few important suggestions:
- A critical fix for a thread-safety issue in the new
MlKitOcrHelper. - A high-severity fix to prevent potential memory leaks in the PDF processing logic by using a lifecycle-aware coroutine scope.
- A medium-severity refactoring suggestion to improve maintainability on the settings screen by removing hardcoded strings.
Overall, this is a great feature addition, and with these changes, it will be more robust and maintainable.
| private suspend fun recognizeFromInputImage(inputImage: InputImage): OcrResult? { | ||
| return suspendCancellableCoroutine { continuation -> | ||
| val startTime = System.currentTimeMillis() | ||
|
|
||
| recognizer?.process(inputImage) | ||
| ?.addOnSuccessListener { visionText -> | ||
| val processingTime = System.currentTimeMillis() - startTime | ||
|
|
||
| val text = visionText.text | ||
|
|
||
| // Calculate confidence from block-level confidence | ||
| val confidences = visionText.textBlocks.mapNotNull { block -> | ||
| block.lines.flatMap { line -> | ||
| line.elements.mapNotNull { it.confidence } | ||
| } | ||
| }.flatten() | ||
|
|
||
| val avgConfidence = if (confidences.isNotEmpty()) { | ||
| (confidences.average() * 100).toInt() | ||
| } else { | ||
| 0 | ||
| } | ||
|
|
||
| Log.d(TAG, "OCR completed in ${processingTime}ms, confidence: $avgConfidence%") | ||
|
|
||
| continuation.resume( | ||
| OcrResult( | ||
| text = text, | ||
| confidence = avgConfidence, | ||
| languages = listOf("eng"), // ML Kit Latin | ||
| processingTimeMs = processingTime | ||
| ) | ||
| ) | ||
| } | ||
| ?.addOnFailureListener { e -> | ||
| Log.e(TAG, "ML Kit text recognition failed", e) | ||
| continuation.resume(null) | ||
| } | ||
| ?: continuation.resume(null) | ||
| } | ||
| } |
There was a problem hiding this comment.
According to the official ML Kit documentation, TextRecognizer instances are not thread-safe and should not be accessed from multiple threads concurrently. Since the recognizeText methods can be called from multiple coroutines on Dispatchers.IO, there is a risk of a race condition when calling recognizer.process().
To ensure thread safety, you should wrap the logic in recognizeFromInputImage within a mutex.withLock block. This will serialize access to the recognizer, preventing crashes and unpredictable behavior.
return mutex.withLock {
suspendCancellableCoroutine { continuation ->
val startTime = System.currentTimeMillis()
recognizer?.process(inputImage)
?.addOnSuccessListener { visionText ->
val processingTime = System.currentTimeMillis() - startTime
val text = visionText.text
// Calculate confidence from block-level confidence
val confidences = visionText.textBlocks.mapNotNull { block ->
block.lines.flatMap { line ->
line.elements.mapNotNull { it.confidence }
}
}.flatten()
val avgConfidence = if (confidences.isNotEmpty()) {
(confidences.average() * 100).toInt()
} else {
0
}
Log.d(TAG, "OCR completed in ${processingTime}ms, confidence: $avgConfidence%")
continuation.resume(
OcrResult(
text = text,
confidence = avgConfidence,
languages = listOf("eng"), // ML Kit Latin
processingTimeMs = processingTime
)
)
}
?.addOnFailureListener { e ->
Log.e(TAG, "ML Kit text recognition failed", e)
continuation.resume(null)
}
?: continuation.resume(null)
}
}| @@ -87,10 +91,13 @@ fun ScanelyNavigation( | |||
|
|
|||
| kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch { | |||
There was a problem hiding this comment.
Using CoroutineScope(Dispatchers.IO) creates a new coroutine scope that is not tied to the lifecycle of this composable. If the user navigates away while the PDF is being processed, this coroutine will not be cancelled, leading to potential memory leaks and wasted resources. It's better to use a scope that is aware of the composable's lifecycle.
You should use rememberCoroutineScope() to get a lifecycle-aware scope and launch your coroutine from it. You'll need to add val scope = rememberCoroutineScope() at the top of the ScanelyNavigation composable (e.g., around line 54).
| kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch { | |
| scope.launch(kotlinx.coroutines.Dispatchers.IO) { |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@copilot look at the gemini code review comment and resolve the issues |
There was a problem hiding this comment.
Pull request overview
This PR adds a user-configurable OCR quality toggle to address poor accuracy issues with the current Tesseract-only implementation. It introduces Google ML Kit Text Recognition as a higher-quality alternative while maintaining Tesseract for multi-language support. The implementation uses a facade pattern (OcrEngine) to abstract the two OCR backends, with user preferences persisted via SharedPreferences.
Key Changes:
- New OCR infrastructure with
OcrQualityenum,MlKitOcrHelperwrapper, and unifiedOcrEnginefacade - Settings UI additions for OCR quality selection with radio toggle between Fast (Tesseract) and Best (ML Kit) modes
- State management and persistence of OCR quality preference alongside existing theme and language settings
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Adds ML Kit Text Recognition dependency version 16.0.1 |
| app/build.gradle.kts | Implements ML Kit Text Recognition library |
| app/src/main/java/com/skeler/scanely/ocr/OcrQuality.kt | Defines FAST and BEST quality enum with labels and descriptions |
| app/src/main/java/com/skeler/scanely/ocr/MlKitOcrHelper.kt | Wraps ML Kit Text Recognition API with coroutine-based interface matching OcrHelper |
| app/src/main/java/com/skeler/scanely/ocr/OcrEngine.kt | Facade that delegates to either Tesseract or ML Kit based on quality setting |
| app/src/main/java/com/skeler/scanely/ui/screens/SettingsScreen.kt | Adds OCR Quality section with radio options and descriptions |
| app/src/main/java/com/skeler/scanely/navigation/Navigation.kt | Updates to use OcrEngine facade, maintains Tesseract for PDF processing |
| app/src/main/java/com/skeler/scanely/MainActivity.kt | Loads and persists OCR quality preference from SharedPreferences |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get Tesseract helper for PDF processing (always use Tesseract for multi-language PDFs) | ||
| val tesseractHelper = ocrEngine.getTesseractHelper() |
There was a problem hiding this comment.
The comment explains that PDF processing uses Tesseract for multi-language support, but this creates an inconsistency in user experience. If a user selects "Best (ML Kit)" quality and then processes a Latin-script PDF, they would expect higher quality OCR, but they'll get Tesseract instead. This should either be documented in the UI (e.g., "Note: PDFs always use Tesseract engine") or the logic should respect the user's quality preference for single-language PDFs.
| quality: OcrQuality, | ||
| currentQuality: OcrQuality, | ||
| onSelect: (OcrQuality) -> Unit | ||
| ) { | ||
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .clickable { onSelect(quality) } | ||
| .padding(vertical = 8.dp), | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| RadioButton( | ||
| selected = (quality == currentQuality), | ||
| onClick = { onSelect(quality) } | ||
| ) | ||
| Column( | ||
| modifier = Modifier.padding(start = 8.dp) | ||
| ) { | ||
| Text( | ||
| text = label, | ||
| style = MaterialTheme.typography.bodyLarge, | ||
| color = MaterialTheme.colorScheme.onSurface | ||
| ) | ||
| Text( | ||
| text = description, | ||
| style = MaterialTheme.typography.bodySmall, |
There was a problem hiding this comment.
The OcrQualityOptionRow composable is missing semantic information for accessibility. The Row should have role and state information so screen readers can properly announce that these are radio button options. Consider using Modifier.semantics or Modifier.selectable with the appropriate role to improve accessibility for users with disabilities.
| OcrQuality.entries.forEach { quality -> | ||
| val label = when (quality) { | ||
| OcrQuality.FAST -> "Fast (Tesseract)" | ||
| OcrQuality.BEST -> "Best (ML Kit)" | ||
| } | ||
| val description = when (quality) { | ||
| OcrQuality.FAST -> "Faster, supports more languages including Arabic" | ||
| OcrQuality.BEST -> "Higher accuracy for Latin text" | ||
| } | ||
|
|
||
| OcrQualityOptionRow( | ||
| label = label, | ||
| description = description, | ||
| quality = quality, |
There was a problem hiding this comment.
The UI hardcodes OCR quality labels and descriptions instead of using the label and description properties defined in the OcrQuality enum. This creates duplication and inconsistency - the enum properties are never used, and changes to the labels would need to be made in multiple places. Consider either using the enum properties directly or removing them from the enum if they're not needed.
| suspend fun initialize(quality: OcrQuality, languages: List<String>): Boolean { | ||
| Log.d(TAG, "Initializing OCR engine with quality=$quality, languages=$languages") | ||
|
|
||
| currentQuality = quality | ||
| currentLanguages = languages | ||
|
|
||
| val success = when (quality) { | ||
| OcrQuality.FAST -> tesseractHelper.initialize(languages) | ||
| OcrQuality.BEST -> mlKitHelper.initialize(languages) | ||
| } | ||
|
|
||
| isInitialized = success | ||
| return success | ||
| } |
There was a problem hiding this comment.
The initialize method doesn't clean up the previously initialized helper when switching quality modes. If a user initializes with FAST mode (Tesseract), then reinitializes with BEST mode (ML Kit), the Tesseract resources remain allocated. Consider releasing the non-active helper's resources during initialization to prevent unnecessary memory usage.
| // Note: ocrQuality is intentionally NOT in dependencies - quality changes only affect new scans | ||
| LaunchedEffect(selectedImageUri, isPdfProcessing, shouldAutoScan) { |
There was a problem hiding this comment.
The comment states "ocrQuality is intentionally NOT in dependencies" but this could lead to unexpected behavior. When a user changes OCR quality in settings and returns to the main screen with a previously selected image, the quality change won't take effect until they select a new image. This behavior should be clearly documented in the UI or the dependency should be added to reinitialize when quality changes.
| // Note: ocrQuality is intentionally NOT in dependencies - quality changes only affect new scans | |
| LaunchedEffect(selectedImageUri, isPdfProcessing, shouldAutoScan) { | |
| LaunchedEffect(selectedImageUri, isPdfProcessing, shouldAutoScan, ocrQuality) { |
|
|
||
| recognizer?.process(inputImage) | ||
| ?.addOnSuccessListener { visionText -> | ||
| val processingTime = System.currentTimeMillis() - startTime | ||
|
|
||
| val text = visionText.text | ||
|
|
||
| // Calculate confidence from block-level confidence | ||
| val confidences = visionText.textBlocks.mapNotNull { block -> | ||
| block.lines.flatMap { line -> | ||
| line.elements.mapNotNull { it.confidence } | ||
| } | ||
| }.flatten() | ||
|
|
||
| val avgConfidence = if (confidences.isNotEmpty()) { | ||
| (confidences.average() * 100).toInt() | ||
| } else { | ||
| 0 | ||
| } | ||
|
|
||
| Log.d(TAG, "OCR completed in ${processingTime}ms, confidence: $avgConfidence%") | ||
|
|
||
| continuation.resume( | ||
| OcrResult( | ||
| text = text, | ||
| confidence = avgConfidence, | ||
| languages = listOf("eng"), // ML Kit Latin | ||
| processingTimeMs = processingTime | ||
| ) | ||
| ) | ||
| } | ||
| ?.addOnFailureListener { e -> | ||
| Log.e(TAG, "ML Kit text recognition failed", e) | ||
| continuation.resume(null) | ||
| } | ||
| ?: continuation.resume(null) |
There was a problem hiding this comment.
The suspendCancellableCoroutine is not properly handling cancellation. When the coroutine is cancelled, the ML Kit process should be cancelled as well. Consider adding continuation.invokeOnCancellation to properly clean up the ML Kit task when the coroutine is cancelled, preventing resource leaks and unnecessary processing.
| recognizer?.process(inputImage) | |
| ?.addOnSuccessListener { visionText -> | |
| val processingTime = System.currentTimeMillis() - startTime | |
| val text = visionText.text | |
| // Calculate confidence from block-level confidence | |
| val confidences = visionText.textBlocks.mapNotNull { block -> | |
| block.lines.flatMap { line -> | |
| line.elements.mapNotNull { it.confidence } | |
| } | |
| }.flatten() | |
| val avgConfidence = if (confidences.isNotEmpty()) { | |
| (confidences.average() * 100).toInt() | |
| } else { | |
| 0 | |
| } | |
| Log.d(TAG, "OCR completed in ${processingTime}ms, confidence: $avgConfidence%") | |
| continuation.resume( | |
| OcrResult( | |
| text = text, | |
| confidence = avgConfidence, | |
| languages = listOf("eng"), // ML Kit Latin | |
| processingTimeMs = processingTime | |
| ) | |
| ) | |
| } | |
| ?.addOnFailureListener { e -> | |
| Log.e(TAG, "ML Kit text recognition failed", e) | |
| continuation.resume(null) | |
| } | |
| ?: continuation.resume(null) | |
| val task = recognizer?.process(inputImage) | |
| if (task == null) { | |
| continuation.resume(null) | |
| return@suspendCancellableCoroutine | |
| } | |
| // Ensure ML Kit processing is cancelled if the coroutine is cancelled | |
| continuation.invokeOnCancellation { | |
| task.cancel() | |
| } | |
| task | |
| .addOnSuccessListener { visionText -> | |
| val processingTime = System.currentTimeMillis() - startTime | |
| val text = visionText.text | |
| // Calculate confidence from block-level confidence | |
| val confidences = visionText.textBlocks.mapNotNull { block -> | |
| block.lines.flatMap { line -> | |
| line.elements.mapNotNull { it.confidence } | |
| } | |
| }.flatten() | |
| val avgConfidence = if (confidences.isNotEmpty()) { | |
| (confidences.average() * 100).toInt() | |
| } else { | |
| 0 | |
| } | |
| Log.d(TAG, "OCR completed in ${processingTime}ms, confidence: $avgConfidence%") | |
| if (continuation.isActive) { | |
| continuation.resume( | |
| OcrResult( | |
| text = text, | |
| confidence = avgConfidence, | |
| languages = listOf("eng"), // ML Kit Latin | |
| processingTimeMs = processingTime | |
| ) | |
| ) | |
| } | |
| } | |
| .addOnFailureListener { e -> | |
| Log.e(TAG, "ML Kit text recognition failed", e) | |
| if (continuation.isActive) { | |
| continuation.resume(null) | |
| } | |
| } |
| /** | ||
| * ML Kit Text Recognition OCR helper. | ||
| * Provides high-quality text recognition for Latin script. | ||
| * | ||
| * Uses Google's ML Kit which runs on-device for privacy. |
There was a problem hiding this comment.
The documentation states that both OCR engines run locally/offline and maintain privacy-first design, but there's no mention that ML Kit may download models on first use. While ML Kit does support on-device processing, it typically needs to download the text recognition model initially, which requires an internet connection. This should be documented to set proper user expectations about the "offline" capability.
| var shouldAutoScan by remember { mutableStateOf(true) } | ||
|
|
||
| val ocrHelper = remember { OcrHelper(context) } | ||
| val ocrEngine = remember { OcrEngine(context) } |
There was a problem hiding this comment.
The OcrEngine is created with remember but never released. Unlike the old single OcrHelper, the OcrEngine now manages two OCR helpers (Tesseract and ML Kit) that both allocate native resources. These resources should be released when the composable leaves composition using DisposableEffect to prevent memory leaks.
| val confidences = visionText.textBlocks.mapNotNull { block -> | ||
| block.lines.flatMap { line -> | ||
| line.elements.mapNotNull { it.confidence } | ||
| } | ||
| }.flatten() |
There was a problem hiding this comment.
The confidence calculation is inefficient. Using mapNotNull followed by flatten() creates intermediate lists. This should use flatMap directly to map and flatten in one operation.
| val confidences = visionText.textBlocks.mapNotNull { block -> | |
| block.lines.flatMap { line -> | |
| line.elements.mapNotNull { it.confidence } | |
| } | |
| }.flatten() | |
| val confidences = visionText.textBlocks.flatMap { block -> | |
| block.lines.flatMap { line -> | |
| line.elements.mapNotNull { it.confidence } | |
| } | |
| } |
| tesseractHelper.release() | ||
| mlKitHelper.release() |
There was a problem hiding this comment.
The release() method unconditionally releases both Tesseract and ML Kit helpers, even though only one is actively initialized based on the current quality mode. While this isn't strictly a bug since both helpers handle release when not initialized, it's inefficient and could cause unnecessary cleanup operations. Consider only releasing the currently active helper.
| tesseractHelper.release() | |
| mlKitHelper.release() | |
| when (currentQuality) { | |
| OcrQuality.FAST -> tesseractHelper.release() | |
| OcrQuality.BEST -> mlKitHelper.release() | |
| else -> { | |
| // Fallback: release both if quality is unknown | |
| tesseractHelper.release() | |
| mlKitHelper.release() | |
| } | |
| } |
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Fixed the Gemini code review issues in commit 15fb8bc:
The third issue (hardcoded labels in SettingsScreen) was already marked as resolved/outdated. |
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
|
/gemini code review |
There was a problem hiding this comment.
Code Review
This pull request successfully adds a high-quality OCR option using Google's ML Kit and provides a toggle to switch between it and the existing Tesseract engine. The implementation is well-structured, introducing an OcrEngine to abstract the two backends and correctly updating the UI and data persistence logic. The refactoring to use rememberCoroutineScope is also a great improvement for lifecycle management.
I've provided a few suggestions to further enhance maintainability and performance. These include refactoring the settings screen to use the OcrQuality enum as a single source of truth for UI strings, optimizing the OCR initialization logic to avoid redundant calls, and clarifying a comment regarding thread safety in the new ML Kit helper. Overall, this is a solid contribution that significantly improves the app's core functionality.
| val name = prefs.getString("ocr_quality", OcrQuality.FAST.name) ?: OcrQuality.FAST.name | ||
| val quality = try { OcrQuality.valueOf(name) } catch (e: Exception) { OcrQuality.FAST } |
There was a problem hiding this comment.
The logic to load the OCR quality preference from SharedPreferences can be simplified. The getString method with a default value will never return null, making the elvis operator (?:) on line 39 redundant. You can make this logic more concise and idiomatic using runCatching.
| val name = prefs.getString("ocr_quality", OcrQuality.FAST.name) ?: OcrQuality.FAST.name | |
| val quality = try { OcrQuality.valueOf(name) } catch (e: Exception) { OcrQuality.FAST } | |
| val name = prefs.getString("ocr_quality", OcrQuality.FAST.name)!! | |
| val quality = runCatching { OcrQuality.valueOf(name) }.getOrDefault(OcrQuality.FAST) |
|
|
||
| /** | ||
| * Recognize text from a bitmap. | ||
| * Note: ML Kit handles thread safety internally, so no mutex is needed. |
There was a problem hiding this comment.
This comment is misleading. According to the official ML Kit documentation, TextRecognizer instances are not thread-safe. Your implementation correctly uses a Mutex in recognizeFromInputImage to ensure thread safety, which contradicts this comment. To avoid confusion for future maintenance, this comment should be removed.
| OcrQuality.entries.forEach { quality -> | ||
| val label = when (quality) { | ||
| OcrQuality.FAST -> "Fast (Tesseract)" | ||
| OcrQuality.BEST -> "Best (ML Kit)" | ||
| } | ||
| val description = when (quality) { | ||
| OcrQuality.FAST -> "Faster, supports more languages including Arabic" | ||
| OcrQuality.BEST -> "Higher accuracy for Latin text" | ||
| } | ||
|
|
||
| OcrQualityOptionRow( | ||
| label = label, | ||
| description = description, | ||
| quality = quality, | ||
| currentQuality = ocrQuality, | ||
| onSelect = onOcrQualityChanged | ||
| ) | ||
| } |
There was a problem hiding this comment.
The labels and descriptions for the OCR quality options are hardcoded here, which duplicates information that could be held in the OcrQuality enum. This can lead to inconsistencies if one is updated and the other is not.
To improve maintainability and adhere to the single source of truth principle, I recommend moving these UI-specific strings into the OcrQuality enum and using them directly from there.
First, you can update OcrQuality.kt to hold the desired UI strings:
// in app/src/main/java/com/skeler/scanely/ocr/OcrQuality.kt
enum class OcrQuality(val label: String, val description: String) {
FAST("Fast (Tesseract)", "Faster, supports more languages including Arabic"),
BEST("Best (ML Kit)", "Higher accuracy for Latin text")
}Then, you can simplify this forEach loop significantly.
OcrQuality.entries.forEach { quality ->
OcrQualityOptionRow(
label = quality.label,
description = quality.description,
quality = quality,
currentQuality = ocrQuality,
onSelect = onOcrQualityChanged
)
}
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.