fix(android): Pixel/Tensor image gen MNN CPU path and clear labeling#457
Open
Ayush7614 wants to merge 1 commit into
Open
fix(android): Pixel/Tensor image gen MNN CPU path and clear labeling#457Ayush7614 wants to merge 1 commit into
Ayush7614 wants to merge 1 commit into
Conversation
…ling Pixel 10 OpenCL image generation is broken on-device (same class of issue as LiteRT GPU). Force cpuOnly load and disable OpenCL during generation on Pixel 10. Rename MNN catalog/filter labels from "GPU" to "MNN" so Tensor/Pixel users are not told to pick a "CPU model" while the UI only offered GPU and NPU filters. Hide the NPU filter when the device has no QNN catalog entries. Fixes off-grid-ai#432
📝 WalkthroughWalkthroughAdds a ChangesPixel 10 CPU-only image generation and MNN rebranding
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ImageGenerationService
participant HardwareService
participant ActiveModelService
User->>ImageGenerationService: generateImage()
ImageGenerationService->>HardwareService: requiresCpuImageBackend()
HardwareService-->>ImageGenerationService: true (Pixel 10 detected)
ImageGenerationService->>ImageGenerationService: useOpenCL = false
ImageGenerationService->>ActiveModelService: doLoadImageModelLocked()
ActiveModelService->>HardwareService: requiresCpuImageBackend()
HardwareService-->>ActiveModelService: true
ActiveModelService->>ActiveModelService: forceCpuImage -> cpuOnly = true
ActiveModelService-->>ImageGenerationService: model loaded (CPU/MNN)
ImageGenerationService-->>User: image generated via CPU backend
Related issues: Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by Qodofix(android): Force MNN CPU path on Pixel 10 and rename MNN labels from GPU
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
1. Em dashes in bannerText
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
397
to
+416
| rec = { | ||
| recommendedBackend: 'mnn', | ||
| bannerText: | ||
| 'GPU models recommended \u2014 your Snapdragon doesn\u2019t support NPU acceleration', | ||
| 'MNN models recommended \u2014 your Snapdragon does not support NPU acceleration', | ||
| compatibleBackends: ['mnn'], | ||
| }; | ||
| } else if (socInfo.vendor === 'tensor') { | ||
| rec = { | ||
| recommendedBackend: 'mnn', | ||
| bannerText: | ||
| this.requiresCpuImageBackend() | ||
| ? 'MNN models recommended for Pixel \u2014 image generation runs on CPU on Pixel 10 until GPU support lands' | ||
| : 'MNN models recommended for Pixel and Tensor devices', | ||
| compatibleBackends: ['mnn'], | ||
| }; | ||
| } else { | ||
| rec = { | ||
| recommendedBackend: 'mnn', | ||
| bannerText: | ||
| 'GPU models recommended \u2014 NPU requires Snapdragon 888+', | ||
| 'MNN models recommended \u2014 NPU acceleration requires Snapdragon 888+', |
There was a problem hiding this comment.
1. Em dashes in bannertext 📘 Rule violation ✧ Quality
User-facing bannerText strings include Unicode em dashes (—), which are disallowed in user-visible text content. This can lead to noncompliant UI copy and inconsistent typography across platforms.
Agent Prompt
## Issue description
User-facing `bannerText` strings in `HardwareService.getImageModelRecommendation()` contain Unicode em dashes (`—`, U+2014). The compliance rule requires using ASCII hyphens (`-`) instead in user-visible text.
## Issue Context
These `bannerText` fields are displayed to users as recommendation banners, so they are in-scope for the text-content restriction.
## Fix Focus Areas
- src/services/hardware.ts[397-416]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+135
to
+139
| hardwareService.getSoCInfo().then(soc => { | ||
| if (cancelled) return; | ||
| setImageNpuAvailable(soc.hasNPU); | ||
| setBackendFilter(prev => (!soc.hasNPU && prev === 'qnn') ? 'mnn' : prev); | ||
| }); |
There was a problem hiding this comment.
2. Unhandled socinfo rejection 🐞 Bug ☼ Reliability
useImageModels() starts a nested hardwareService.getSoCInfo() promise chain without error handling, so a failure in SoC detection can produce an unhandled promise rejection and skip updating imageNpuAvailable/backendFilter. This can leave the image models UI in an inconsistent state after a DeviceInfo/native failure.
Agent Prompt
### Issue description
`useImageModels()` triggers `hardwareService.getSoCInfo()` in a nested `.then(...)` chain without a `.catch(...)` or surrounding `try/catch`. If `getSoCInfo()` rejects (e.g., `DeviceInfo.getHardware()` fails), this becomes an unhandled promise rejection and the state updates inside the `.then` never run.
### Issue Context
`hardwareService.getSoCInfo()` contains awaited native calls that can reject and it does not internally catch/convert those failures.
### Fix Focus Areas
- src/screens/ModelsScreen/useImageModels.ts[130-139]
- src/services/hardware.ts[308-334]
### Suggested fix
- In the mount effect, convert the chain to `async/await` with `try/catch`, or attach `.catch(...)` to the `getSoCInfo()` call.
- On error, set a safe fallback (e.g., `setImageNpuAvailable(false)` and ensure `backendFilter` is not left as `'qnn'`).
- Optionally, consider making `getSoCInfo()` resilient by catching native errors and returning `{ vendor: 'unknown', hasNPU: false }` rather than rejecting.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/ModelsScreen/ImageFilterBar.tsx (1)
9-30: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBackend label mapping duplicated across
getBackendLabelandBACKEND_OPTIONS.
getBackendLabelre-hardcodes themnn/qnnlabel strings that already live inBACKEND_OPTIONS(used a few lines below at line 128). This PR had to touch both places to rename "GPU" → "MNN" — a sign the mapping isn't defined once. As per coding guidelines, "whether the mapping/rule is defined once and reused; avoid speculative abstraction and duplication."♻️ Proposed consolidation
function getBackendLabel(filter: BackendFilter): string { - if (filter === 'mnn') return 'MNN'; - if (filter === 'qnn') return 'NPU'; - if (filter === 'coreml') return 'Core ML'; - return 'Backend'; + if (filter === 'coreml') return 'Core ML'; + return BACKEND_OPTIONS.find(o => o.key === filter)?.label ?? 'Backend'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screens/ModelsScreen/ImageFilterBar.tsx` around lines 9 - 30, The backend label text is duplicated between getBackendLabel and BACKEND_OPTIONS in ImageFilterBar, so update the component to derive labels from a single source of truth. Move the backend-to-label mapping into one shared constant or helper used by both getBackendLabel and the backend chip options, and keep the existing symbols getBackendLabel and BACKEND_OPTIONS wired to that shared mapping so future label changes only happen once.Source: Coding guidelines
🧹 Nitpick comments (1)
src/services/huggingFaceModelBrowser.ts (1)
118-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "GPU" reference in sort comment.
The comment
// Sort: GPU first, then NPUstill uses the old "GPU" terminology being replaced by "MNN" throughout this file/PR. Consider updating for consistency.✏️ Suggested fix
- // Sort: GPU first, then NPU; alphabetically within each group + // Sort: MNN first, then NPU; alphabetically within each group🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/huggingFaceModelBrowser.ts` at line 118, Update the stale sort comment in the huggingFaceModelBrowser sorting logic so it matches the new terminology used in this file. In the section around the model sorting comparator, replace the old “GPU first, then NPU” wording with the current “MNN first, then NPU” phrasing, keeping the rest of the comment aligned with the sort behavior in the surrounding code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/integration/models/pixel10ImageGenCpuPath.test.ts`:
- Around line 18-46: The current integration test only re-checks hardwareService
behavior and duplicates unit coverage instead of validating the Pixel 10
CPU-only flow across modules. Update pixel10ImageGenCpuPath.test.ts to exercise
the real cross-layer path by involving ImageGenerationService and/or
activeModelService (with the Pixel 10 mock) and assert that OpenCL is disabled
and cpuOnly/forceCpuImage is applied during model selection/load; if that
end-to-end path cannot be covered here, remove the test file since it does not
add integration coverage.
In `@src/services/imageGenerationService.ts`:
- Around line 458-461: The OpenCL setting is still presented as user-controlled
even when the backend forces CPU-only mode, so update the settings UI/state
handling to reflect the effective value. In the image generation flow around
requiresCpuImageBackend() and getOpenCLCapability(), detect the
pixel_10_cpu_only reason and disable or clearly annotate the OpenCL toggle so it
appears off/locked when hardwareService forces CPU image backend, keeping the
displayed setting aligned with imageUseOpenCL and useOpenCL.
---
Outside diff comments:
In `@src/screens/ModelsScreen/ImageFilterBar.tsx`:
- Around line 9-30: The backend label text is duplicated between getBackendLabel
and BACKEND_OPTIONS in ImageFilterBar, so update the component to derive labels
from a single source of truth. Move the backend-to-label mapping into one shared
constant or helper used by both getBackendLabel and the backend chip options,
and keep the existing symbols getBackendLabel and BACKEND_OPTIONS wired to that
shared mapping so future label changes only happen once.
---
Nitpick comments:
In `@src/services/huggingFaceModelBrowser.ts`:
- Line 118: Update the stale sort comment in the huggingFaceModelBrowser sorting
logic so it matches the new terminology used in this file. In the section around
the model sorting comparator, replace the old “GPU first, then NPU” wording with
the current “MNN first, then NPU” phrasing, keeping the rest of the comment
aligned with the sort behavior in the surrounding code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed698648-acb0-4a96-a582-49eefcfc7043
📒 Files selected for processing (14)
__tests__/integration/models/pixel10ImageGenCpuPath.test.ts__tests__/unit/services/hardware.test.ts__tests__/unit/services/huggingFaceModelBrowser.test.tssrc/screens/DownloadManagerScreen/useDownloadManager.tssrc/screens/ModelsScreen/ImageFilterBar.tsxsrc/screens/ModelsScreen/ImageModelsTab.tsxsrc/screens/ModelsScreen/constants.tssrc/screens/ModelsScreen/imageDownloadQnn.tssrc/screens/ModelsScreen/useImageModels.tssrc/screens/ModelsScreen/useModelsScreen.tssrc/services/activeModelService/index.tssrc/services/hardware.tssrc/services/huggingFaceModelBrowser.tssrc/services/imageGenerationService.ts
Comment on lines
+18
to
+46
| describe('Pixel 10 image generation CPU path integration', () => { | ||
| beforeEach(() => { | ||
| Platform.OS = 'android'; | ||
| (hardwareService as any).cachedDeviceInfo = null; | ||
| (hardwareService as any).cachedSoCInfo = null; | ||
| (hardwareService as any).cachedImageRecommendation = null; | ||
| (hardwareService as any).cachedOpenCLCapability = null; | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| Platform.OS = originalOS; | ||
| }); | ||
|
|
||
| it('detects Pixel 10 and disables OpenCL image acceleration', async () => { | ||
| expect(hardwareService.requiresCpuImageBackend()).toBe(true); | ||
| const openCl = await hardwareService.getOpenCLCapability(); | ||
| expect(openCl.supported).toBe(false); | ||
| expect(openCl.reason).toBe('pixel_10_cpu_only'); | ||
| }); | ||
|
|
||
| it('recommends only MNN models with a Pixel 10-specific banner', async () => { | ||
| const rec = await hardwareService.getImageModelRecommendation(); | ||
| expect(rec.recommendedBackend).toBe('mnn'); | ||
| expect(rec.compatibleBackends).toEqual(['mnn']); | ||
| expect(rec.bannerText).toContain('Pixel 10'); | ||
| expect(DeviceInfo.getModel).toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Integration test only exercises a single module — duplicates unit coverage instead of validating cross-layer behavior.
Both tests here call only hardwareService methods (requiresCpuImageBackend, getOpenCLCapability, getImageModelRecommendation) with a mocked react-native-device-info. The second test (Lines 39-45) is essentially identical to the "recommends MNN with Pixel 10 CPU notice" case already added in __tests__/unit/services/hardware.test.ts (lines 1035-1050) — same setup, same assertions.
Per the PR stack, the actual CPU-only enforcement happens in ImageGenerationService (disabling OpenCL) and activeModelService (forceCpuImage/cpuOnly) — neither is exercised here. A file named pixel10ImageGenCpuPath.test.ts under __tests__/integration/ implies it should verify the full path (HardwareService detection → ImageGenerationService/activeModelService consuming it), not just re-test HardwareService in isolation.
Consider either:
- Extending this test to actually invoke
ImageGenerationService/activeModelServicewith the Pixel 10 mock and assert OpenCL is disabled /cpuOnly: trueis set during model load, or - Removing this file if it doesn't add coverage beyond the unit test.
As per coding guidelines, "Integration tests in __tests__/integration/ should verify how multiple modules work together end-to-end, using mocked native modules but real logic across layers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/integration/models/pixel10ImageGenCpuPath.test.ts` around lines 18
- 46, The current integration test only re-checks hardwareService behavior and
duplicates unit coverage instead of validating the Pixel 10 CPU-only flow across
modules. Update pixel10ImageGenCpuPath.test.ts to exercise the real cross-layer
path by involving ImageGenerationService and/or activeModelService (with the
Pixel 10 mock) and assert that OpenCL is disabled and cpuOnly/forceCpuImage is
applied during model selection/load; if that end-to-end path cannot be covered
here, remove the test file since it does not add integration coverage.
Source: Coding guidelines
Comment on lines
+458
to
+461
| const useOpenCL = hardwareService.requiresCpuImageBackend() | ||
| ? false | ||
| : (settings.imageUseOpenCL ?? true); | ||
| return this._runGenerationAndSave({ params, enhancedPrompt, activeImageModel, steps, guidanceScale, imageWidth, imageHeight, useOpenCL }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "imageUseOpenCL" -C3 --type=ts --type=tsx 2>/dev/nullRepository: off-grid-ai/off-grid-ai-mobile
Length of output: 168
🏁 Script executed:
#!/bin/bash
rg -n "imageUseOpenCL|pixel_10_cpu_only|getOpenCLCapability|requiresCpuImageBackend|OpenCL" --type=ts --type=tsx --type=js --type=jsxRepository: off-grid-ai/off-grid-ai-mobile
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -e
printf 'FILES\n'
git ls-files | rg 'src/.*(imageGenerationService|settings|opencl|hardwareService)|docs/|Settings' || true
printf '\nMATCHES\n'
rg -n "imageUseOpenCL|pixel_10_cpu_only|getOpenCLCapability|requiresCpuImageBackend|OpenCL" src docs --hidden || trueRepository: off-grid-ai/off-grid-ai-mobile
Length of output: 18221
🏁 Script executed:
#!/bin/bash
set -e
git ls-files | sed -n '1,240p'Repository: off-grid-ai/off-grid-ai-mobile
Length of output: 11644
🏁 Script executed:
#!/bin/bash
set -e
printf 'MODEL SETTINGS\n'
sed -n '45,80p' src/screens/ModelSettingsScreen/ImageGenerationSection.tsx
printf '\nGENERATION SETTINGS\n'
sed -n '78,110p' src/components/GenerationSettingsModal/ImageQualitySliders.tsx
printf '\nHARDWARE CAPABILITY\n'
sed -n '440,475p' src/services/hardware.tsRepository: off-grid-ai/off-grid-ai-mobile
Length of output: 3969
Surface the Pixel 10 CPU-only override in Settings — the OpenCL toggle still looks user-controlled even though requiresCpuImageBackend() forces it off. Disable or annotate it when getOpenCLCapability().reason === 'pixel_10_cpu_only' so the UI matches the effective setting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/imageGenerationService.ts` around lines 458 - 461, The OpenCL
setting is still presented as user-controlled even when the backend forces
CPU-only mode, so update the settings UI/state handling to reflect the effective
value. In the image generation flow around requiresCpuImageBackend() and
getOpenCLCapability(), detect the pixel_10_cpu_only reason and disable or
clearly annotate the OpenCL toggle so it appears off/locked when hardwareService
forces CPU image backend, keeping the displayed setting aligned with
imageUseOpenCL and useOpenCL.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type of Change
Related Issues
Fixes #432
Test plan
Summary by CodeRabbit
New Features
Bug Fixes