feat: add colour filtering to immich frame settings #40
Conversation
Signed-off-by: juanjsebgarcia <github@juangarcia.co.uk>
📝 WalkthroughWalkthroughAdds image adjustment features (brightness, contrast, RGB channels, gamma) with a custom SeekBarPreference, caching and application of ColorMatrixColorFilter across ImageView, WebView, ScreenSaverService, MainActivity, and WidgetProvider. Also adds a Makefile with build/test/install/run targets. Changes
Sequence DiagramsequenceDiagram
participant User
participant SettingsFragment
participant Helpers
participant MainActivity/ScreenSaverService
participant ImageView/WebView
User->>SettingsFragment: change SeekBar (brightness/contrast/R/G/B/gamma)
SettingsFragment->>SettingsFragment: persist preference, toggle visibility if needed
MainActivity/ScreenSaverService->>MainActivity/ScreenSaverService: onSettingsLoaded / after change
MainActivity/ScreenSaverService->>Helpers: getImageAdjustmentFilter(context)
alt cached
Helpers-->>MainActivity/ScreenSaverService: return cached ColorMatrixColorFilter
else compute
Helpers->>Helpers: read preferences, compose ColorMatrix, build filter
Helpers->>Helpers: apply gamma lookup if requested, cache filter
Helpers-->>MainActivity/ScreenSaverService: return new ColorMatrixColorFilter
end
MainActivity/ScreenSaverService->>ImageView/WebView: apply filter (Paint + layer type, fallback to software)
ImageView/WebView-->>User: render adjusted image
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @app/src/main/java/com/immichframe/immichframe/Helpers.kt:
- Around line 419-463: The early-return in applyGammaToBitmap currently returns
the original bitmap when too large, violating the "will be recycled" contract;
fix by making the large-bitmap branch create and return a new bitmap copy (e.g.,
val copy = bitmap.copy(bitmap.config ?: Bitmap.Config.ARGB_8888, true) or
Bitmap.createBitmap(bitmap)), then call bitmap.recycle() before returning the
copy so the input is always recycled, or alternatively update the function docs
to explicitly state the exception for large bitmaps if copying is unacceptable.
In @app/src/main/res/layout/preference_seekbar.xml:
- Around line 29-45: The two TextView labels (ids seekbar_min and seekbar_max)
are hardcoded to "-100"/"100" and should reflect the actual SeekBar bounds;
update the SeekBarPreference dialog setup (where the layout is inflated in
SeekBarPreference.kt) to find these views (seekbar_min and seekbar_max) and set
their text to the preference min and max values (e.g.,
min.toString()/max.toString()) after inflation so they display the correct
dynamic range; also update the XML placeholder text to something neutral like
"0" or empty if desired.
In @Makefile:
- Line 10: Help text for the Makefile target install-release is missing a space
after the hyphen; update the echo string for the install-release target (the
line that prints "install-release- Install release APK to connected device") to
include a space after the hyphen so it reads "install-release - Install release
APK to connected device" to keep help formatting consistent.
- Line 1: The .PHONY line is incomplete and should list all non-file targets to
avoid Make treating them as files; update the .PHONY declaration to include the
missing targets build-release, assemble, uninstall, lint, and run (in addition
to the existing build, clean, install, install-debug, install-release, test,
help) so that targets like build-release, assemble, uninstall, lint, and run are
always treated as phony.
🧹 Nitpick comments (6)
Makefile (1)
4-15: Consider static analysis suggestions for Makefile conventions.The static analysis tool flagged two style conventions:
- The
helptarget body exceeds the recommended length (10 lines vs. 5 max)- Missing a conventional
allphony targetThese are optional improvements. The current help output is clear and comprehensive, though you could split it into a separate script if desired. The
alltarget is a common convention that typically builds all main artifacts.app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)
594-609: Good hardware/software fallback for WebView rendering.The layer rendering approach with fallback is well-implemented. However, this exact code block is duplicated in the
reapplyImageFilters()method (lines 640-653).♻️ Consider extracting the WebView filter application to reduce duplication
You could extract the WebView filter application logic into a small helper method to avoid duplication:
private fun applyWebViewFilter() { if (!::webView.isInitialized) return val filter = Helpers.getImageAdjustmentFilter(this) if (filter != null) { val paint = Paint() paint.colorFilter = filter try { webView.setLayerType(View.LAYER_TYPE_HARDWARE, paint) } catch (e: Exception) { Log.w("MainActivity", "Hardware layer failed, falling back to software: ${e.message}") webView.setLayerType(View.LAYER_TYPE_SOFTWARE, paint) } } else { webView.setLayerType(View.LAYER_TYPE_HARDWARE, null) } }Then call
applyWebViewFilter()at lines 594-609 and inreapplyImageFilters().app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (1)
530-545: Consider extracting WebView filter application to avoid duplication.This WebView color filter logic is duplicated in
reapplyImageFilters()(lines 578-589). While functional, extracting this to a helper method would reduce duplication and make future changes easier to maintain.♻️ Suggested refactor
+ private fun applyWebViewFilter(filter: ColorMatrixColorFilter?) { + if (filter != null) { + val paint = Paint() + paint.colorFilter = filter + try { + webView.setLayerType(View.LAYER_TYPE_HARDWARE, paint) + } catch (e: Exception) { + Log.w("ScreenSaverService", "Hardware layer failed, falling back to software: ${e.message}") + webView.setLayerType(View.LAYER_TYPE_SOFTWARE, paint) + } + } else { + webView.setLayerType(View.LAYER_TYPE_HARDWARE, null) + } + }Then use it in both locations:
// In loadSettings(): applyWebViewFilter(Helpers.getImageAdjustmentFilter(this)) // In reapplyImageFilters(): applyWebViewFilter(Helpers.getImageAdjustmentFilter(this))app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt (2)
18-18: Unused import.The
DialogFragmentimport appears to be unused. The code usesSeekBarPreference.SeekBarPreferenceDialogFragmentwhich extendsPreferenceDialogFragmentCompat, notDialogFragment.♻️ Proposed fix
-import androidx.fragment.app.DialogFragment
70-86: Manual persistence is redundant.
SwitchPreferenceCompatautomatically persists its value when the change listener returnstrue. The explicitSharedPreferences.edit()call duplicates this behavior.♻️ Simplified implementation
chkImageAdjustments?.setOnPreferenceChangeListener { preference, newValue -> val value = newValue as Boolean imageBrightness?.isVisible = value imageContrast?.isVisible = value imageRedChannel?.isVisible = value imageGreenChannel?.isVisible = value imageBlueChannel?.isVisible = value imageGamma?.isVisible = value - - // Save the preference value immediately so it takes effect - PreferenceManager.getDefaultSharedPreferences(requireContext()) - .edit() - .putBoolean("imageAdjustments", value) - .apply() - true }app/src/main/java/com/immichframe/immichframe/SeekBarPreference.kt (1)
49-55: Use explicit locale for consistent number formatting.
String.formatwithout an explicit locale uses the device's default locale, which can cause inconsistent decimal separators (e.g., "1.50" vs "1,50"). For numeric display values,Locale.USorLocale.ROOTensures consistent formatting.♻️ Proposed fix
+import java.util.Locale + // In updateSummary(): - String.format("%.2f", currentValue / 100.0) + String.format(Locale.US, "%.2f", currentValue / 100.0) // In onBindDialogView() for minText: - String.format("%.1f", preference.min / 100.0) + String.format(Locale.US, "%.1f", preference.min / 100.0) // In onBindDialogView() for maxText: - String.format("%.1f", preference.max / 100.0) + String.format(Locale.US, "%.1f", preference.max / 100.0) // In updateValueText(): - String.format("%.2f", value / 100.0) + String.format(Locale.US, "%.2f", value / 100.0)Also applies to: 79-89, 120-126
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Makefileapp/src/main/java/com/immichframe/immichframe/Helpers.ktapp/src/main/java/com/immichframe/immichframe/MainActivity.ktapp/src/main/java/com/immichframe/immichframe/ScreenSaverService.ktapp/src/main/java/com/immichframe/immichframe/SeekBarPreference.ktapp/src/main/java/com/immichframe/immichframe/SettingsFragment.ktapp/src/main/java/com/immichframe/immichframe/WidgetProvider.ktapp/src/main/res/layout/preference_seekbar.xmlapp/src/main/res/values/attrs.xmlapp/src/main/res/xml/settings_view.xml
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (1)
app/src/main/java/com/immichframe/immichframe/MainActivity.kt (1)
reapplyImageFilters(633-655)
🪛 checkmake (0.2.2)
Makefile
[warning] 4-4: Target body for "help" exceeds allowed length of 5 (10).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
🪛 detekt (1.23.8)
app/src/main/java/com/immichframe/immichframe/SeekBarPreference.kt
[warning] 104-104: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
[warning] 105-105: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
[warning] 51-51: String.format("%.2f", currentValue / 100.0) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
[warning] 81-81: String.format("%.1f", preference.min / 100.0) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
[warning] 86-86: String.format("%.1f", preference.max / 100.0) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
[warning] 122-122: String.format("%.2f", value / 100.0) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🔇 Additional comments (21)
app/src/main/java/com/immichframe/immichframe/WidgetProvider.kt (1)
93-93: LGTM!The image adjustment is correctly applied to the bitmap before displaying in the widget, maintaining consistency with the adjustments applied in MainActivity and ScreenSaverService.
app/src/main/res/values/attrs.xml (1)
1-7: LGTM!The custom attribute definition is correctly structured for the SeekBarPreference component.
app/src/main/java/com/immichframe/immichframe/MainActivity.kt (4)
9-9: LGTM!The Paint import is necessary for applying color filters to the WebView via layer rendering.
240-240: LGTM!The color filter is correctly applied to the ImageView after the bitmap is set, ensuring adjustments are visible in the UI.
619-620: LGTM!Reapplying image filters after loading server settings ensures the adjustments are consistently applied to any currently visible content.
633-655: LGTM!The
reapplyImageFilters()method correctly handles both rendering modes and applies filters to prevent visual artifacts during transitions. The duplication with the WebView filter code inloadSettings()has been noted in a previous comment.app/src/main/res/xml/settings_view.xml (2)
1-2: LGTM!The
appnamespace is correctly added to support custom attributes for the SeekBarPreference component.
39-81: LGTM!The Image Adjustments preferences are well-structured with appropriate ranges and defaults. The gamma setting's different range (10-300) and helpful summary note are good design choices.
app/src/main/res/layout/preference_seekbar.xml (1)
49-56: LGTM!The Reset button is appropriately styled and positioned for the preference dialog.
app/src/main/java/com/immichframe/immichframe/ScreenSaverService.kt (2)
229-229: LGTM!Filter is correctly applied to the new image view during transition. This ensures the color adjustments are visible as soon as the image is displayed.
569-591: LGTM!The
reapplyImageFilters()method correctly handles both ImageView and WebView modes with appropriate hardware/software fallback. The implementation is consistent withMainActivity.kt.app/src/main/java/com/immichframe/immichframe/SettingsFragment.kt (2)
28-55: LGTM!The preference lookups and initial visibility setup for image adjustment controls are correctly implemented.
173-184: LGTM with minor note on deprecation.The custom dialog handling for
SeekBarPreferenceis correctly implemented. The@Suppress("DEPRECATION")is appropriate sincesetTargetFragmentis still required byPreferenceDialogFragmentCompatuntil a future AndroidX update provides an alternative.app/src/main/java/com/immichframe/immichframe/SeekBarPreference.kt (3)
91-107: LGTM!The SeekBar setup correctly calculates progress from the preference min/max range. The empty interface methods at lines 104-105 are required by
SeekBar.OnSeekBarChangeListenerand cannot be removed (static analysis false positive).
128-139: LGTM!Dialog close handling correctly validates via
callChangeListenerbefore persisting, following the Android preference pattern.
141-149: LGTM!The companion object factory method correctly creates the dialog fragment with the preference key argument.
app/src/main/java/com/immichframe/immichframe/Helpers.kt (5)
23-27: LGTM!Thread-safe caching with a dedicated lock object is appropriate for this use case where the filter may be accessed from multiple contexts (MainActivity, ScreenSaverService, WidgetProvider).
112-144: LGTM! Breaking change well-documented.The function now consistently recycles the input bitmap in all code paths, which is clearly documented. Callers should be aware they cannot reuse the input bitmap after calling this function.
238-286: LGTM!The filter caching implementation is well-structured:
- Validates and clamps preference values to prevent corruption
- Correctly handles the
includeGammaflag in the cache key- Returns early when adjustments are disabled or all values are default
- Uses synchronized blocks appropriately for thread safety
288-367: LGTM!The color matrix construction is well-organized with clear comments explaining each transformation. The gamma approximation comment (lines 337-340) appropriately documents the limitation that true gamma correction requires per-pixel transformation.
382-409: LGTM!The bitmap adjustment function correctly:
- Gets filter without gamma (to apply proper per-pixel gamma separately)
- Creates a new bitmap and applies color matrix via Canvas/Paint
- Recycles input bitmap consistently
- Applies proper LUT-based gamma correction when needed
| <TextView | ||
| android:id="@+id/seekbar_min" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:layout_weight="1" | ||
| android:text="-100" | ||
| android:textSize="12sp" | ||
| android:gravity="start" /> | ||
|
|
||
| <TextView | ||
| android:id="@+id/seekbar_max" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:layout_weight="1" | ||
| android:text="100" | ||
| android:textSize="12sp" | ||
| android:gravity="end" /> |
There was a problem hiding this comment.
Fix hardcoded min/max labels to reflect actual SeekBar bounds.
The min and max TextViews have hardcoded values of "-100" and "100", which won't match preferences that use different ranges. For example, the gamma preference uses min=10 and max=300, but these labels will incorrectly display -100 and 100.
🐛 Proposed fix to make labels dynamic
The SeekBarPreference implementation should update these labels programmatically. In the SeekBarPreference.kt dialog setup, add:
// After inflating the layout
val minLabel = view.findViewById<TextView>(R.id.seekbar_min)
val maxLabel = view.findViewById<TextView>(R.id.seekbar_max)
minLabel.text = min.toString()
maxLabel.text = max.toString()And update the layout to use placeholder text:
<TextView
android:id="@+id/seekbar_min"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_weight="1"
- android:text="-100"
+ android:text="0"
android:textSize="12sp"
android:gravity="start" />
<TextView
android:id="@+id/seekbar_max"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_weight="1"
- android:text="100"
+ android:text="0"
android:textSize="12sp"
android:gravity="end" />🤖 Prompt for AI Agents
In @app/src/main/res/layout/preference_seekbar.xml around lines 29 - 45, The two
TextView labels (ids seekbar_min and seekbar_max) are hardcoded to "-100"/"100"
and should reflect the actual SeekBar bounds; update the SeekBarPreference
dialog setup (where the layout is inflated in SeekBarPreference.kt) to find
these views (seekbar_min and seekbar_max) and set their text to the preference
min and max values (e.g., min.toString()/max.toString()) after inflation so they
display the correct dynamic range; also update the XML placeholder text to
something neutral like "0" or empty if desired.
| @@ -0,0 +1,58 @@ | |||
| .PHONY: build clean install install-debug install-release test help | |||
There was a problem hiding this comment.
Incomplete .PHONY declaration.
The .PHONY declaration is missing several targets defined later in the file: build-release, assemble, uninstall, lint, and run. This could cause make to misinterpret these targets as files if files with matching names exist in the directory.
🔧 Proposed fix
-.PHONY: build clean install install-debug install-release test help
+.PHONY: build build-release assemble clean install install-debug install-release uninstall test lint run help📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .PHONY: build clean install install-debug install-release test help | |
| .PHONY: build build-release assemble clean install install-debug install-release uninstall test lint run help |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
🤖 Prompt for AI Agents
In @Makefile at line 1, The .PHONY line is incomplete and should list all
non-file targets to avoid Make treating them as files; update the .PHONY
declaration to include the missing targets build-release, assemble, uninstall,
lint, and run (in addition to the existing build, clean, install, install-debug,
install-release, test, help) so that targets like build-release, assemble,
uninstall, lint, and run are always treated as phony.
| @echo " build-release - Build release APK" | ||
| @echo " clean - Clean build artifacts" | ||
| @echo " install - Install debug APK to connected device" | ||
| @echo " install-release- Install release APK to connected device" |
There was a problem hiding this comment.
Fix formatting inconsistency.
Missing space after the hyphen in the help text for install-release.
🔧 Proposed fix
- @echo " install-release- Install release APK to connected device"
+ @echo " install-release - Install release APK to connected device"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @echo " install-release- Install release APK to connected device" | |
| @echo " install-release - Install release APK to connected device" |
🤖 Prompt for AI Agents
In @Makefile at line 10, Help text for the Makefile target install-release is
missing a space after the hyphen; update the echo string for the install-release
target (the line that prints "install-release- Install release APK to connected
device") to include a space after the hyphen so it reads "install-release -
Install release APK to connected device" to keep help formatting consistent.
Signed-off-by: juanjsebgarcia <github@juangarcia.co.uk>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/src/main/java/com/immichframe/immichframe/Helpers.kt:
- Around line 419-463: The safety check in applyGammaToBitmap currently returns
the original bitmap when it’s too large, violating the function contract that
the input bitmap will be recycled; change the early-return path to call
bitmap.recycle() before returning and update the Log.w message to clearly state
that gamma correction was skipped due to size (e.g., include
"${width}x${height}, skipping gamma and recycling input bitmap") so callers are
informed and no leak occurs.
🧹 Nitpick comments (3)
app/src/main/java/com/immichframe/immichframe/Helpers.kt (3)
238-286: Consider improving cache key clarity and error handling.The cache key includes both
gammavalue andgammaKeyflag (lines 271-272), which works but could be clearer. WhenincludeGamma=false, the gamma value is set to 100 and gammaKey to -1, making the intent less obvious. Consider documenting this behavior or restructuring the cache key.Additionally,
SharedPreferences.getInt()can throwClassCastExceptionif preferences are corrupted or contain unexpected types. Consider adding try-catch blocks around preference reads for robustness.♻️ Optional: Add defensive error handling
fun getImageAdjustmentFilter(context: Context, includeGamma: Boolean = true): ColorMatrixColorFilter? { val prefs = PreferenceManager.getDefaultSharedPreferences(context) // Check if image adjustments are enabled - val adjustmentsEnabled = prefs.getBoolean("imageAdjustments", false) + val adjustmentsEnabled = try { + prefs.getBoolean("imageAdjustments", false) + } catch (e: ClassCastException) { + Log.w("Helpers", "Invalid imageAdjustments preference type", e) + false + } if (!adjustmentsEnabled) { synchronized(filterCacheLock) { cachedFilterSettings = null cachedFilter = null } return null } // Validate and clamp preference values to prevent corruption issues - val brightness = prefs.getInt("image_brightness", 0).coerceIn(-50, 50) - val contrast = prefs.getInt("image_contrast", 0).coerceIn(-50, 50) - val red = prefs.getInt("image_red_channel", 0).coerceIn(-50, 50) - val green = prefs.getInt("image_green_channel", 0).coerceIn(-50, 50) - val blue = prefs.getInt("image_blue_channel", 0).coerceIn(-50, 50) - val gamma = if (includeGamma) prefs.getInt("image_gamma", 100).coerceIn(10, 200) else 100 + val brightness = try { prefs.getInt("image_brightness", 0) } catch (e: ClassCastException) { 0 }.coerceIn(-50, 50) + val contrast = try { prefs.getInt("image_contrast", 0) } catch (e: ClassCastException) { 0 }.coerceIn(-50, 50) + val red = try { prefs.getInt("image_red_channel", 0) } catch (e: ClassCastException) { 0 }.coerceIn(-50, 50) + val green = try { prefs.getInt("image_green_channel", 0) } catch (e: ClassCastException) { 0 }.coerceIn(-50, 50) + val blue = try { prefs.getInt("image_blue_channel", 0) } catch (e: ClassCastException) { 0 }.coerceIn(-50, 50) + val gamma = if (includeGamma) try { prefs.getInt("image_gamma", 100) } catch (e: ClassCastException) { 100 }.coerceIn(10, 200) else 100
337-364: Document gamma approximation limitations more prominently.The gamma approximation using contrast and brightness (lines 341-364) is acknowledged as inaccurate, but this could lead to visually inconsistent results between
ImageView/WebViewrendering (which uses this approximation) and widget rendering (which uses proper per-pixel gamma viaapplyGammaToBitmap).The approximation formula appears heuristic without clear mathematical basis. Consider:
- Adding visual disclaimers in the settings UI that gamma may appear different across rendering modes
- Documenting the acceptable error range or visual impact
- Considering alternative approaches (e.g., disabling gamma for ColorFilter mode, or using fragment shaders for hardware-accelerated gamma)
382-409: Optimize for no-adjustment case to save memory.When no adjustments are needed (
filter == null && gamma == 100), the function still creates an intermediate bitmap copy (line 390), which wastes memory. Consider early-returning a copy only when necessary.Additionally, line 389 uses
ARGB_8888as the fallback config, consuming 4 bytes per pixel, while other parts of the codebase useRGB_565(2 bytes per pixel). Consider usingRGB_565consistently for memory efficiency, unless alpha channel transparency is required.♻️ Proposed optimization
fun applyImageAdjustmentsToBitmap(bitmap: Bitmap, context: Context): Bitmap { val prefs = PreferenceManager.getDefaultSharedPreferences(context) val gamma = prefs.getInt("image_gamma", 100).coerceIn(10, 200) // Apply ColorMatrix-based adjustments (brightness, contrast, RGB) // Exclude gamma from ColorFilter - we'll apply it properly via LUT val filter = getImageAdjustmentFilter(context, includeGamma = false) - val config = bitmap.config ?: Bitmap.Config.ARGB_8888 + + // Early return if no adjustments needed + if (filter == null && gamma == 100) { + val copy = bitmap.copy(bitmap.config ?: Bitmap.Config.RGB_565, false) + bitmap.recycle() + return copy + } + + val config = bitmap.config ?: Bitmap.Config.RGB_565 var result = Bitmap.createBitmap(bitmap.width, bitmap.height, config) val canvas = Canvas(result) val paint = Paint()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/immichframe/immichframe/Helpers.ktapp/src/main/res/xml/settings_view.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/res/xml/settings_view.xml
🔇 Additional comments (2)
app/src/main/java/com/immichframe/immichframe/Helpers.kt (2)
23-27: LGTM! Good caching strategy.The thread-safe cache using synchronized blocks will improve performance by avoiding redundant filter creation on every image load.
112-144: ARGB_8888 fallback is appropriate despite memory cost.The function correctly assigns the result back to the variable in all callers (verified across WidgetProvider, MainActivity, and ScreenSaverService), so the bitmap recycling pattern is properly handled throughout the codebase.
However, regarding the ARGB_8888 fallback on line 131: While RGB_565 uses less memory (2 bytes/pixel vs 4), ARGB_8888 is the safer default choice because RGB_565 doesn't support an alpha channel. If the input bitmap requires transparency, using RGB_565 would lose that data. Since bitmaps passed to this function come from various sources, preserving the original config (with ARGB_8888 as fallback) is correct defensive programming—the same pattern is used elsewhere in this file (lines 389, 456).
| private fun applyGammaToBitmap(bitmap: Bitmap, gamma: Float): Bitmap { | ||
| val width = bitmap.width | ||
| val height = bitmap.height | ||
|
|
||
| // Safety check: prevent excessive memory allocation | ||
| // Max 4000x4000 = 16M pixels = 64MB for ARGB_8888 | ||
| val maxPixels = 4000 * 4000 | ||
| if (width * height > maxPixels) { | ||
| Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, skipping gamma") | ||
| return bitmap | ||
| } | ||
|
|
||
| // Build lookup table for gamma correction | ||
| val gammaLUT = IntArray(256) { i -> | ||
| val normalized = i / 255f | ||
| val corrected = Math.pow(normalized.toDouble(), gamma.toDouble()).toFloat() | ||
| (corrected * 255f).coerceIn(0f, 255f).toInt() | ||
| } | ||
|
|
||
| val pixels = IntArray(width * height) | ||
| bitmap.getPixels(pixels, 0, width, 0, 0, width, height) | ||
|
|
||
| // Apply gamma correction to each pixel | ||
| for (i in pixels.indices) { | ||
| val pixel = pixels[i] | ||
| val a = (pixel shr 24) and 0xff | ||
| val r = (pixel shr 16) and 0xff | ||
| val g = (pixel shr 8) and 0xff | ||
| val b = pixel and 0xff | ||
|
|
||
| val rCorrected = gammaLUT[r] | ||
| val gCorrected = gammaLUT[g] | ||
| val bCorrected = gammaLUT[b] | ||
|
|
||
| pixels[i] = (a shl 24) or (rCorrected shl 16) or (gCorrected shl 8) or bCorrected | ||
| } | ||
|
|
||
| val result = Bitmap.createBitmap(width, height, bitmap.config ?: Bitmap.Config.ARGB_8888) | ||
| result.setPixels(pixels, 0, width, 0, 0, width, height) | ||
|
|
||
| // Recycle input bitmap | ||
| bitmap.recycle() | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
Critical: Fix contract violation in safety check.
Line 428 returns the input bitmap without recycling it when the size check fails, violating the function's documented contract that the input "will be recycled" (line 415). This creates a memory leak path.
🐛 Proposed fix
// Safety check: prevent excessive memory allocation
// Max 4000x4000 = 16M pixels = 64MB for ARGB_8888
val maxPixels = 4000 * 4000
if (width * height > maxPixels) {
- Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, skipping gamma")
- return bitmap
+ Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, creating copy without gamma")
+ val copy = bitmap.copy(bitmap.config ?: Bitmap.Config.RGB_565, false)
+ bitmap.recycle()
+ return copy
}Additionally, consider enhancing the warning log to inform users that gamma was skipped, as this could affect the visual output unexpectedly for large images.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun applyGammaToBitmap(bitmap: Bitmap, gamma: Float): Bitmap { | |
| val width = bitmap.width | |
| val height = bitmap.height | |
| // Safety check: prevent excessive memory allocation | |
| // Max 4000x4000 = 16M pixels = 64MB for ARGB_8888 | |
| val maxPixels = 4000 * 4000 | |
| if (width * height > maxPixels) { | |
| Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, skipping gamma") | |
| return bitmap | |
| } | |
| // Build lookup table for gamma correction | |
| val gammaLUT = IntArray(256) { i -> | |
| val normalized = i / 255f | |
| val corrected = Math.pow(normalized.toDouble(), gamma.toDouble()).toFloat() | |
| (corrected * 255f).coerceIn(0f, 255f).toInt() | |
| } | |
| val pixels = IntArray(width * height) | |
| bitmap.getPixels(pixels, 0, width, 0, 0, width, height) | |
| // Apply gamma correction to each pixel | |
| for (i in pixels.indices) { | |
| val pixel = pixels[i] | |
| val a = (pixel shr 24) and 0xff | |
| val r = (pixel shr 16) and 0xff | |
| val g = (pixel shr 8) and 0xff | |
| val b = pixel and 0xff | |
| val rCorrected = gammaLUT[r] | |
| val gCorrected = gammaLUT[g] | |
| val bCorrected = gammaLUT[b] | |
| pixels[i] = (a shl 24) or (rCorrected shl 16) or (gCorrected shl 8) or bCorrected | |
| } | |
| val result = Bitmap.createBitmap(width, height, bitmap.config ?: Bitmap.Config.ARGB_8888) | |
| result.setPixels(pixels, 0, width, 0, 0, width, height) | |
| // Recycle input bitmap | |
| bitmap.recycle() | |
| return result | |
| } | |
| private fun applyGammaToBitmap(bitmap: Bitmap, gamma: Float): Bitmap { | |
| val width = bitmap.width | |
| val height = bitmap.height | |
| // Safety check: prevent excessive memory allocation | |
| // Max 4000x4000 = 16M pixels = 64MB for ARGB_8888 | |
| val maxPixels = 4000 * 4000 | |
| if (width * height > maxPixels) { | |
| Log.w("Helpers", "Bitmap too large for gamma correction: ${width}x${height}, creating copy without gamma") | |
| val copy = bitmap.copy(bitmap.config ?: Bitmap.Config.RGB_565, false) | |
| bitmap.recycle() | |
| return copy | |
| } | |
| // Build lookup table for gamma correction | |
| val gammaLUT = IntArray(256) { i -> | |
| val normalized = i / 255f | |
| val corrected = Math.pow(normalized.toDouble(), gamma.toDouble()).toFloat() | |
| (corrected * 255f).coerceIn(0f, 255f).toInt() | |
| } | |
| val pixels = IntArray(width * height) | |
| bitmap.getPixels(pixels, 0, width, 0, 0, width, height) | |
| // Apply gamma correction to each pixel | |
| for (i in pixels.indices) { | |
| val pixel = pixels[i] | |
| val a = (pixel shr 24) and 0xff | |
| val r = (pixel shr 16) and 0xff | |
| val g = (pixel shr 8) and 0xff | |
| val b = pixel and 0xff | |
| val rCorrected = gammaLUT[r] | |
| val gCorrected = gammaLUT[g] | |
| val bCorrected = gammaLUT[b] | |
| pixels[i] = (a shl 24) or (rCorrected shl 16) or (gCorrected shl 8) or bCorrected | |
| } | |
| val result = Bitmap.createBitmap(width, height, bitmap.config ?: Bitmap.Config.ARGB_8888) | |
| result.setPixels(pixels, 0, width, 0, 0, width, height) | |
| // Recycle input bitmap | |
| bitmap.recycle() | |
| return result | |
| } |
🤖 Prompt for AI Agents
In @app/src/main/java/com/immichframe/immichframe/Helpers.kt around lines 419 -
463, The safety check in applyGammaToBitmap currently returns the original
bitmap when it’s too large, violating the function contract that the input
bitmap will be recycled; change the early-return path to call bitmap.recycle()
before returning and update the Log.w message to clearly state that gamma
correction was skipped due to size (e.g., include "${width}x${height}, skipping
gamma and recycling input bitmap") so callers are informed and no leak occurs.
1 participant
This PR adds the abilty to toggle on image adjustment mode in the immich frame settings.
This allows editing of the contrast, brightness, gamma, red, green and blue channels of the frame.
This allows compensation on some frames which from factory typically come out too blue/cold.
Work done with the help of Claude, but has been edited, reviewed and tested personally.
Works in native mode and has also been tested with WebView 106.
Summary by CodeRabbit
New Features
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.