[Android] Fix Capture video crashes after stopping recording on Android 12#35638
Conversation
<!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## What Restrict the agentic-labeler to apply **exactly one `area-*` label** per item, while still allowing multiple `platform/*` labels. ## Why Backfilling the 26 items affected by the `max:1` bug (fixed in dotnet#35540) revealed that the labeler occasionally applies multiple `area-*` labels for ambiguous cases: - **dotnet#35501** got both `area-layout` and `area-safearea` - **dotnet#35490** got both `area-navigation` and `area-controls-tabbedpage` The intended behavior is exactly one best-fit `area-*` per item (a label-quota distinction not expressible via `safe-outputs.add-labels.max:` — that field counts total labels, not labels per prefix). The fix has to live in the agent's instructions. ## Changes ### `.github/skills/agentic-labeler/SKILL.md` - Scope section: "Exactly one `area-*`" / "One or more `platform/*`". - Area rules section: renamed heading, changed "pick one or more" → "apply exactly one". - New **tie-breaking heuristics** for the area-* selection: - Specific control beats generic area (`area-controls-tabbedpage` over `area-navigation`) - Sub-area beats parent area (`area-safearea` over `area-layout`) - Subject-matter focus beats incidental touch - When genuinely tied, prefer the user-visible feature - Mixed-PR rule clarified: infra-primary PRs get only `area-infrastructure` (no second product area). ### `.github/workflows/agentic-labeler.md` - Added explicit reinforcement in the workflow prompt: "Apply exactly one `area-*` label … and one or more `platform/*` labels". - Fixed two stale `max: 1` comments left over from dotnet#35540 (the cap is now `max: 10`). ### `.github/workflows/agentic-labeler.lock.yml` - Regenerated via `gh aw compile`. Diff is frontmatter-hash + heredoc rotations only — no semantic change to the compiled config. ## Validation - Reviewed all 21 existing eval scenarios in `tests/eval.yaml` — none assert multiple `area-*` labels, so no test updates needed. - The `max: 10` cap in `safe-outputs` is preserved as a blast-radius safeguard (one area + several platforms still fit comfortably). ## Follow-ups (not in this PR) If accuracy of the "one area" rule drops below ~95% in eval runs, consider adding a deterministic post-step that strips extra `area-*` labels per a known precedence list (Option B from the design discussion). Co-authored-by: bot <bot@test> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Description
Extends the `maui-copilot` DevDiv pipeline (pipeline 27723) with a
3-stage architecture that runs real UI tests on platform-pool agents and
reports results directly in the AI summary PR comment.
### Pipeline Workflow
```
┌─────────────────────────────────────────────────────────┐
│ Stage 1: ReviewPR │
│ │
│ STEP 1: Branch Setup (checkout + cherry-pick PR) │
│ STEP 2: Detect UI Test Categories │
│ STEP 3: Run Detected UI Tests (in-process, fast) │
│ STEP 4: Regression Cross-Reference │
│ STEP 5: Gate — verify tests fail/pass before/after fix │
│ STEP 6: Code Review — deep analysis via Copilot agent │
│ │
│ Outputs → CopilotLogs artifact + detectedCategories │
└──────────────────────┬──────────────────────────────────┘
│
┌──────────────────────▼──────────────────────────────────┐
│ Stage 2: RunDeepUITests (platform-pool agent) │
│ │
│ iOS: AcesShared Tahoe + iOS 26.4 │
│ Android: ubuntu-22.04 + KVM + AVD │
│ │
│ Runs BuildAndRunHostApp.ps1 per detected category │
│ Outputs → drop-deep-uitests artifact (TRX + diffs) │
└──────────────────────┬──────────────────────────────────┘
│
┌──────────────────────▼──────────────────────────────────┐
│ Stage 3: PostResults │
│ │
│ 1. Download CopilotLogs (review content files) │
│ 2. Download drop-deep-uitests (TRX results) │
│ 3. Merge deep results into uitests/content.md │
│ 4. Post full AI Summary comment on PR │
│ 5. Apply labels (s/agent-reviewed, etc.) │
│ │
│ One comment with everything — no patching needed │
└─────────────────────────────────────────────────────────┘
```
### What's New
**Deep UI Test Execution (Stage 2)**
- Runs detected UI test categories on proper platform-pool agents (not
in-process on Linux)
- **iOS**: AcesShared Tahoe agents with iOS 26.4 simulator, iPhone 11
Pro (matching `ios-26` baselines from PR dotnet#35061)
- **Android**: ubuntu-22.04 with KVM, AVD boot with `-partition-size
2048`, `ignoreHiddenApiPolicyError` capability
- TRX results + snapshot-diff PNGs published as `drop-deep-uitests`
artifact
**Unified Comment Posting (Stage 3)**
- Comment posting and label application deferred to Stage 3 (after deep
tests complete)
- Single AI summary comment includes ALL results: code review + deep
test results
- Nested collapsible `<details>` for failed tests with full error +
stack trace
- Dynamic section title: `🧪 UI Tests — CollectionView, TabbedPage`
- Artifact download link for snapshot-diff PNGs
**Android Emulator Improvements**
- AVD boot step with proper partition size, ADB key pre-authorization,
boot wait
- `DEVICE_UDID` pass-through prevents double emulator boot
- Disk cleanup on hosted ubuntu agents (frees ~22GB)
- KVM enablement + `appium:ignoreHiddenApiPolicyError` for API 30
**iOS Simulator Improvements**
- Tahoe pool demand ensures macOS 26.x agents
- Explicit iOS 26.4 download via latest Xcode
- Auto-creates iPhone 11 Pro for baseline resolution match
### Validation
Tested across 30+ pipeline iterations on 6 PRs:
| PR | iOS | Android |
|---|---|---|
| 35358 (ViewBaseTests) | **112/112 ALL PASS** ✅ | **118/119 PASS** ✅ |
| 35359 (TabbedPage) | 44/50 (1 real failure) | 74/75 (1 real failure) |
| 35356 (CollectionView) | **415/417 PASS** ✅ | 593/619 (26 real
failures) |
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…35589) > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! Backport of dotnet#35460 to `main`. /cc @PureWeen Co-authored-by: HarishKumarSF4517 <harish.kumar@syncfusion.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35638Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35638" |
|
Hey there @@HarishwaranVijayakumar! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/review -b feature/refactor-copilot-yml |
|
/review -b feature/refactor-copilot-yml |
🤖 AI Summary
📊 Review Session —
|
| Category | Tests | Snapshot diffs |
|---|---|---|
Essentials |
— | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🔍 Regression Cross-Reference
🔍 Regression Cross-Reference
🟢 No regression risks detected. No labeled bug-fix PRs in the last 6 months touched the modified files.
🔍 Pre-Flight — Context & Validation
Issue: #28891 - Capture video crashes after stopping recording on Android 12
PR: #35638 - [Android] Fix Capture video crashes after stopping recording on Android 12
Platforms Affected: Android 12 / Android 12L (API 31-32)
Files Changed: 1 implementation, 0 test
Key Findings
- Issue Capture video crashes after stopping recording on Android 12 #28891 is a verified Android MediaPicker crash:
CaptureVideoAsyncreceives a pending MediaStore URI from the camera app on Android 12, andFileSystemUtils.EnsurePhysicalPath(intent.Data)crashes whenContentResolver.OpenInputStreamhits MediaProvider ownership enforcement. - PR [Android] Fix Capture video crashes after stopping recording on Android 12 #35638 changes only
src/Essentials/src/MediaPicker/MediaPicker.android.cs, adding an Android 12-onlyFileProvider+MediaStore.ExtraOutputcapture path for videos that mirrors the existing photo-capture path. - No tests were added; gate was already skipped because no tests were detected. Android device/manual validation is the relevant regression surface.
ghCLI was unavailable due missing authentication, but GitHub MCP metadata was available and used for PR details, issue details, comments, files, and diff.
Code Review Summary
Verdict: NEEDS_DISCUSSION
Confidence: medium
Errors: 0 | Warnings: 1 | Suggestions: 2
Key code review findings:
⚠️ src/Essentials/src/MediaPicker/MediaPicker.android.cs:378returns the temporary path without checking whether the camera actually wrote the file; if a camera returns success but ignoresEXTRA_OUTPUT, callers receive aFileResultfor a missing file.- 💡
src/Essentials/src/MediaPicker/MediaPicker.android.cs:365hardcodes.mp4; low-risk, but OEM cameras can theoretically write a different container. - 💡 The Android 12 video path duplicates the existing
CapturePhotoAsyncFileProvider/ExtraOutputlogic and could drift if only one path is hardened later.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35638 | For Android API 31-32, pre-create an app cache file and pass its FileProvider URI via MediaStore.ExtraOutput so MAUI never opens the camera-owned pending MediaStore URI. |
src/Essentials/src/MediaPicker/MediaPicker.android.cs |
Original PR; code review says approach is sound but notes missing file-existence hardening. |
🔬 Code Review — Deep Analysis
Code Review — PR #35638
Independent Assessment
What this changes: Adds a version-gated code path in CaptureVideoAsync for Android API 31–32 (Android 12 / 12L). Instead of relying on the camera app's returned intent.Data URI — which is a pending MediaStore URI on Android 12 — the fix pre-creates a temp file in the app's cache directory and passes it to the camera via MediaStore.ExtraOutput. The camera writes directly to that file, bypassing the MediaStore ownership enforcement. The approach is structurally identical to the existing CapturePhotoAsync method.
Inferred motivation: A crash (IllegalStateException) was occurring on Android 12 when MAUI tried to open the video URI returned by the camera. On Android 12, camera apps create videos in MediaStore with IS_PENDING=1 and release ownership only after they finalize the entry. MAUI's UID doesn't match the camera app's UID, so ContentResolver.OpenInputStream(uri) triggers requireOwnershipForItem() enforcement, which did not exist on Android 11 and was relaxed on Android 13.
Is the approach sound? Yes. The FileProvider + ExtraOutput approach is the correct documented workaround. The version range IsAndroidVersionAtLeast(31) && !IsAndroidVersionAtLeast(33) precisely targets Android 12 and 12L. The existing code path is preserved for all other API levels.
Reconciliation with PR Narrative
Author claims: Crash on Android 12 caused by requireOwnershipForItem() enforcement; fix uses FileProvider + ExtraOutput approach mirroring CapturePhotoAsync; references Xamarin.Essentials as prior art.
Agreement: The root cause analysis and fix approach match the code. The version range is accurate. The code is a direct copy of the CapturePhotoAsync pattern applied to video.
No disagreement: The PR is honest about scope — it targets only Android 12 rather than applying the EXTRA_OUTPUT approach globally (which would also be valid but is a broader change). The conservative scope is appropriate.
Findings
⚠️ Warning — No file-existence check before returning path (line 378)
FileSystemUtils.GetTemporaryFile creates directory scaffolding but does not create the actual video file. The camera app is expected to write to tmpFile via EXTRA_OUTPUT. If it does not (e.g., a camera app on Android 12 that ignores EXTRA_OUTPUT for video), IntermediateActivity may still return RESULT_OK, causing return tmpFile.AbsolutePath to return a path to a non-existent file. CaptureAsync checks captureResult is not null but will receive a non-null string, so it proceeds to new FileResult(captureResult). Downstream OpenReadAsync() calls will throw FileNotFoundException.
Suggested fix:
return tmpFile.Exists() ? tmpFile.AbsolutePath : null;Note: CapturePhotoAsync has the same design, so this is not a regression introduced by this PR. For photos EXTRA_OUTPUT support is universal; for video it is broadly supported on API 31+ but not guaranteed by all OEM cameras.
💡 Suggestion — Hardcoded .mp4 extension (line 365)
FileExtensions.Mp4 (.mp4) is a safe default for Android 12 video capture — the vast majority of Android 12 devices record MP4. However, some OEM camera apps record to .3gp or use non-standard containers. Since FileResult infers ContentType from the file extension, a mismatch would produce an incorrect MIME type. This is low-risk but worth documenting as a known limitation (same caveat applies to CapturePhotoAsync hardcoding .jpg).
💡 Suggestion — Code duplication with CapturePhotoAsync
The new Android 12 video block (lines 363–379) is nearly identical to CapturePhotoAsync (lines 269–291). If the pattern is ever changed in one place (e.g., to add file-existence checks), it will need to be updated in both. A shared private helper (CaptureToTempFileAsync) accepting the captureIntent and file extension would eliminate the duplication and reduce drift risk.
Devil's Advocate
Challenging the Warning: Is the file-existence concern real on Android 12? Android 12 camera apps are generally modern and EXTRA_OUTPUT is well-supported on that API level. The referenced Xamarin.Essentials used this approach on all API levels. The likelihood of a camera app returning RESULT_OK without writing the file is low. However, the fix costs nothing (one tmpFile.Exists() check) and silently swallows a failure mode that would otherwise manifest as a cryptic downstream error.
Challenging LGTM: The fix does not add any tests. It is validated only by manual testing on Android. The gate flagged "no tests detected." A device test for Android 12 video capture would strengthen confidence in future regressions.
Am I sure about the version range? Yes. API 31 = Android 12, API 32 = Android 12L, API 33 = Android 13. The expression IsAndroidVersionAtLeast(31) && !IsAndroidVersionAtLeast(33) is correct and precise.
CI Status
✅ All 36 CI checks pass including maui-pr, Build Analysis, and all integration test jobs.
Verdict: NEEDS_DISCUSSION
Confidence: medium
Summary: The fix correctly addresses a real Android 12 crash using a well-established pattern (matching the existing CapturePhotoAsync). CI is green. The main open question is whether tmpFile.Exists() should be checked before returning the path — the current behavior silently returns a path to a non-existent file if the camera app doesn't honor EXTRA_OUTPUT. This exact design is shared with CapturePhotoAsync, so it is not a regression. The team should explicitly decide whether to harden both capture methods or accept the current behavior as-is. If the team is comfortable with the existing CapturePhotoAsync pattern, this PR can be approved as LGTM.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Shared CaptureToAppCacheAsync; use FileProvider + EXTRA_OUTPUT for all video captures below Android 13; return null if no file is written. |
❌ Failed expert review; Android compile passed | 1 file | Broadened behavior risks regressing Android <= 30 camera apps that ignore EXTRA_OUTPUT. |
| 2 | try-fix | Create an app-owned pending MediaStore.Video row, pass its URI via EXTRA_OUTPUT, publish it after capture, then resolve/cache it. |
❌ Failed expert review; Android compile passed | 1 file | Avoids camera-owned pending URI but changes privacy/lifecycle by publishing a gallery-visible MediaStore item with no cleanup path. |
| 3 | try-fix | Keep the returned camera URI and retry EnsurePhysicalPath on Android 12 IllegalStateException. |
❌ Failed expert review; Android compile passed | 1 file | Races undocumented IS_PENDING clearance timing and contains a retry-loop logic bug. |
| PR | PR #35638 | Android API 31-32 only FileProvider + MediaStore.ExtraOutput path for video capture. |
1 file | Original PR; scoped to the known Android 12 pending-MediaStore ownership bug. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Keep the API 31-32 guard; helper extraction/hardening may be useful, but broadening below API 31 is unsafe without device coverage. |
| maui-expert-reviewer | 1 | Yes | MediaStore ownership avoids the crash but introduces gallery/public-row lifecycle problems; prefer app-cache output for transient captures. |
| maui-expert-reviewer | 1 | No | Retrying the returned URI is nondeterministic because the Android 12 failure is ownership enforcement, not a bounded transient I/O delay. |
| maui-expert-reviewer | 2 | No | The remaining solution space is exhausted: post-capture reads hit the same ownership check, app-owned MediaStore output changes visibility/lifecycle, and broadening EXTRA_OUTPUT below API 31 risks older OEM regressions. |
Exhausted: Yes
Selected Fix: PR #35638 — all meaningfully different alternatives compiled but failed expert review; the PR's targeted FileProvider/cache approach remains the safest candidate. Consider adding a tmpFile.Exists()/length hardening check and regression coverage.
📋 Report — Final Recommendation
Comparative Report — PR #35638
Candidates compared
| Rank | Candidate | Result | Regression-test status | Assessment |
|---|---|---|---|---|
| 1 | pr |
Winner | Best-scoped fix. Uses app-cache FileProvider + MediaStore.ExtraOutput only on Android API 31-32, matching the verified Android 12 pending MediaStore ownership bug. Preserves existing behavior elsewhere. |
|
| 1 | pr-plus-reviewer |
Equivalent to winner | The MAUI expert reviewer returned no actionable inline findings, so this candidate is identical to pr. |
|
| 3 | try-fix-1 |
Failed expert review | Android compile passed, expert review failed | Useful helper/existence-check idea, but broadens EXTRA_OUTPUT video capture to all pre-Android-13 devices and risks regressing older OEM camera apps where the existing returned-URI path worked. |
| 4 | try-fix-2 |
Failed expert review | Android compile passed, expert review failed | Avoids the camera-owned pending URI with an app-owned MediaStore row, but changes lifecycle/privacy by publishing a gallery-visible video and provides no durable cleanup path through FileResult. |
| 5 | try-fix-3 |
Failed expert review | Android compile passed, expert review failed | Retry strategy races undocumented camera timing for clearing IS_PENDING and contains a retry-loop logic bug. It does not deterministically address the ownership-enforcement root cause. |
Winning candidate
Winner: pr
The submitted PR fix and pr-plus-reviewer are functionally identical because the expert reviewer returned no actionable findings. I select pr as the single winning candidate because it is the raw PR fix as submitted, it directly targets the verified Android 12/12L crash, and all try-fix alternatives failed expert review despite compiling.
Notes
No tests were detected in the PR, so adding Android coverage or documenting manual Android 12 validation remains recommended. Per ranking rules, all candidates that failed expert review/regression assessment are ranked below the PR candidates.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
Root Cause
Description of Change
Android 12 video capture compatibility:
CaptureVideoAsyncinMediaPicker.android.csto useFileProviderand setMediaStore.ExtraOutputfor video capture on Android 12 (API 31-32), ensuring the app can access the video file without encounteringIllegalStateExceptiondue to stricter ownership enforcement.Reference:
Issues Fixed
Fixes #28891
Tested the behaviour in the following platforms
Output
Before_.28891.mov
After_.28891.mov