-
-
Notifications
You must be signed in to change notification settings - Fork 263
fix(android): Pixel/Tensor image gen MNN CPU path and clear labeling #457
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { Platform } from 'react-native'; | ||
| import DeviceInfo from 'react-native-device-info'; | ||
| import { hardwareService } from '../../../src/services/hardware'; | ||
|
|
||
| const originalOS = Platform.OS; | ||
|
|
||
| jest.mock('react-native-device-info', () => ({ | ||
| getModel: jest.fn(() => 'Pixel 10 Pro XL'), | ||
| getHardware: jest.fn(async () => 'tensor'), | ||
| getTotalMemory: jest.fn(async () => 12 * 1024 * 1024 * 1024), | ||
| getUsedMemory: jest.fn(async () => 4 * 1024 * 1024 * 1024), | ||
| getSystemName: jest.fn(() => 'Android'), | ||
| getSystemVersion: jest.fn(() => '17'), | ||
| isEmulator: jest.fn(async () => false), | ||
| getDeviceId: jest.fn(() => 'pixel10'), | ||
| })); | ||
|
|
||
| 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(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| const [imageSearchQuery, setImageSearchQuery] = useState(''); | ||
| const [imageFiltersVisible, setImageFiltersVisible] = useState(false); | ||
| const [imageRec, setImageRec] = useState<ImageModelRecommendation | null>(null); | ||
| const [imageNpuAvailable, setImageNpuAvailable] = useState(false); | ||
| const [userChangedBackendFilter, setUserChangedBackendFilter] = useState(false); | ||
| const [showRecommendedOnly, setShowRecommendedOnly] = useState(true); | ||
| const [showRecHint, setShowRecHint] = useState(true); | ||
|
|
@@ -66,6 +67,7 @@ | |
| }))); | ||
| } else { | ||
| const socInfo = await hardwareService.getSoCInfo(); | ||
| setImageNpuAvailable(socInfo.hasNPU); | ||
| setAvailableHFModels(await fetchAvailableModels(forceRefresh, { skipQnn: !socInfo.hasNPU })); | ||
| } | ||
| } catch (error: any) { | ||
|
|
@@ -130,6 +132,11 @@ | |
| hardwareService.getImageModelRecommendation().then(rec => { | ||
| if (cancelled) return; | ||
| setImageRec(rec); | ||
| hardwareService.getSoCInfo().then(soc => { | ||
| if (cancelled) return; | ||
| setImageNpuAvailable(soc.hasNPU); | ||
| setBackendFilter(prev => (!soc.hasNPU && prev === 'qnn') ? 'mnn' : prev); | ||
|
Check failure on line 138 in src/screens/ModelsScreen/useImageModels.ts
|
||
| }); | ||
|
Comment on lines
+135
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Unhandled socinfo rejection 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
|
||
| if (!userChangedBackendFilter && Platform.OS !== 'ios') { | ||
| let filter: 'qnn' | 'mnn' | 'all'; | ||
| if (rec.recommendedBackend === 'qnn') filter = 'qnn'; | ||
|
|
@@ -208,6 +215,7 @@ | |
| imageFiltersVisible, setImageFiltersVisible, | ||
| imageRec, showRecommendedOnly, setShowRecommendedOnly, | ||
| showRecHint, setShowRecHint, | ||
| imageNpuAvailable, | ||
| downloadedImageModels, | ||
| hasActiveImageFilters, filteredHFModels, imageRecommendation, | ||
| loadHFModels, loadDownloadedImageModels, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -397,14 +397,23 @@ | |
| 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+', | ||
|
Comment on lines
397
to
+416
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Em dashes in bannertext 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
|
||
| compatibleBackends: ['mnn'], | ||
| }; | ||
| } | ||
|
|
@@ -436,6 +445,9 @@ | |
| async getOpenCLCapability(): Promise<{ supported: boolean; reason?: string }> { | ||
| if (this.cachedOpenCLCapability) return this.cachedOpenCLCapability; | ||
| if (Platform.OS !== 'android') return { supported: false, reason: 'not_android' }; | ||
| if (this.requiresCpuImageBackend()) { | ||
| return (this.cachedOpenCLCapability = { supported: false, reason: 'pixel_10_cpu_only' }); | ||
|
Check warning on line 449 in src/services/hardware.ts
|
||
| } | ||
| try { | ||
| const hardware = (await DeviceInfo.getHardware()).toLowerCase(); | ||
| // Support Qualcomm Adreno (qcom) and ARM Mali GPUs. | ||
|
|
@@ -445,5 +457,14 @@ | |
| return (this.cachedOpenCLCapability = { supported: true }); | ||
| } catch { return (this.cachedOpenCLCapability = { supported: false, reason: 'detection_failed' }); } | ||
| } | ||
|
|
||
| /** | ||
| * Pixel 10 OpenCL/GPU image backends are broken on-device (same class of bug as | ||
| * LiteRT GPU on Pixel 10). Force the MNN CPU path for image load + generation. | ||
| */ | ||
| requiresCpuImageBackend(): boolean { | ||
| if (Platform.OS !== 'android') return false; | ||
| return DeviceInfo.getModel().toLowerCase().includes('pixel 10'); | ||
| } | ||
| } | ||
| export const hardwareService = new HardwareService(); | ||
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.
📐 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
hardwareServicemethods (requiresCpuImageBackend,getOpenCLCapability,getImageModelRecommendation) with a mockedreact-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) andactiveModelService(forceCpuImage/cpuOnly) — neither is exercised here. A file namedpixel10ImageGenCpuPath.test.tsunder__tests__/integration/implies it should verify the full path (HardwareService detection → ImageGenerationService/activeModelService consuming it), not just re-test HardwareService in isolation.Consider either:
ImageGenerationService/activeModelServicewith the Pixel 10 mock and assert OpenCL is disabled /cpuOnly: trueis set during model load, orAs 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
Source: Coding guidelines