fix(video_player_avfoundation): remove forced AVVideoColorPropertiesKey BT.709 (regression on iOS 17+)#12238
Conversation
…ey BT.709 The forced AVVideoColorPropertiesKey block on the AVPlayerItemVideoOutput (added in 2.9.7 to tone-map HDR sources to BT.709 SDR) overrides the AVPlayerLayer's natural color pipeline on iOS 17+, producing a desaturated grey surface across the entire video region on portrait phones. The override was only needed for the FlutterTexture path that existed in 2.9.7's pre-platformView era. Removing the override: the output uses the source's native CICP metadata, and iOS handles HDR→SDR tone-mapping automatically in AVPlayerLayer via its built-in pipeline (preferredPeakBitRate, pixelBufferAttributes, etc). Verified on iPhone 15 Pro (iOS 26) with the upstream example app: - BEFORE (with forced BT.709): uniform grey surface, all colors washed out - AFTER (override removed): full color video, skin tones natural Closes the regression reported on iOS 17+ where hosts of the plugin see a desaturated grey video region even on SDR H.264 sources.
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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 removes the forced AVVideoColorPropertiesKey configuration from AVPlayerItemVideoOutput in FVPVideoPlayer.m to resolve a desaturated grey surface rendering issue on iOS 17+. Feedback on these changes highlights that removing this configuration will break an existing unit test (videoOutputIsConfiguredWithBT709ColorProperties) which must be updated or removed. Additionally, there is a typo ("iOS 26") and a ticket-specific prefix in the newly added code comments that should be corrected.
| // KAN-FIX-170: removed the forced AVVideoColorPropertiesKey block that was added | ||
| // for flutter/flutter#91241 (HDR tone-mapping for the Flutter texture). On iOS | ||
| // 17+, forcing AVVideoColorPropertiesKey to BT.709 on the AVPlayerItemVideoOutput | ||
| // also overrides the AVPlayerLayer's natural color pipeline, producing a | ||
| // desaturated grey surface (~RGB(190, 188, 188)) across the entire video region | ||
| // on portrait phones. Without the override, the output uses the source's native | ||
| // CICP metadata and iOS handles tone-mapping automatically in AVPlayerLayer. | ||
| // | ||
| // Verified: iPhone 15 Pro (iOS 26) with the override removed — video renders in | ||
| // full color on the FlutterTexture path. Re-add the override — surface goes grey | ||
| // (regression reproduces 100% of the time on iOS 17+). |
There was a problem hiding this comment.
1. Broken Unit Test
Removing the AVVideoColorPropertiesKey block will cause the existing unit test videoOutputIsConfiguredWithBT709ColorProperties in VideoPlayerTests.swift (lines 778-798) to fail, as it explicitly asserts that these color properties are configured. Please update or remove that test in this PR to ensure the CI build passes.
2. Typo & Ticket Reference in Comment
- There is a typo in the comment: "iOS 26" (line 156). iOS 26 does not exist; this should be corrected to "iOS 17+" or "iOS 18".
- It is also recommended to remove the ticket-specific prefix
KAN-FIX-170:to keep the codebase comments clean and general.
// Removed the forced AVVideoColorPropertiesKey block that was added
// for flutter/flutter#91241 (HDR tone-mapping for the Flutter texture). On iOS
// 17+, forcing AVVideoColorPropertiesKey to BT.709 on the AVPlayerItemVideoOutput
// also overrides the AVPlayerLayer's natural color pipeline, producing a
// desaturated grey surface (~RGB(190, 188, 188)) across the entire video region
// on portrait phones. Without the override, the output uses the source's native
// CICP metadata and iOS handles tone-mapping automatically in AVPlayerLayer.
//
// Verified: iPhone 15 Pro (iOS 17+) with the override removed — video renders in
// full color on the FlutterTexture path. Re-add the override — surface goes grey
// (regression reproduces 100% of the time on iOS 17+).References
- Code should be tested. Changes to plugin packages, which include code written in C, C++, Java, Kotlin, Objective-C, or Swift, should have appropriate tests. (link)
|
Thank you for your contribution! Because of the volume of PRs we receive, we require that new contributors use our checklist to guide them through critical steps in creating a Flutter PR. This PR's description is missing that checklist, so it is being marked as a Draft. Please edit the PR description to add the checklist, then ensure that you have completed all of the steps. Once you've done that, please mark the PR as ready for review. If you need help, consider asking for advice on the #hackers-new channel on Discord. |
2 participants
Summary
The
AVVideoColorPropertiesKey: BT.709block on theAVPlayerItemVideoOutput(added in 2.9.7 to address flutter/flutter#91241 — HDR tone-mapping for the Flutter texture) also affects theAVPlayerLayer's natural color pipeline on iOS 17+, producing a desaturated grey surface (~RGB(190, 188, 188)) across the entire video region on portrait phones.The override is no longer needed: iOS handles HDR→SDR tone-mapping automatically in
AVPlayerLayerviapreferredPeakBitRate,pixelBufferAttributes, and the display-aware tone-mapping pipeline. The 2.9.7 workaround was appropriate for the pre-platformView era, but the platformView code path (added in 2.7.0) means the override is now actively harmful.Reproduction (verified on iPhone 15 Pro, iOS 26)
video_playerexample app on iOS 17+Changes
Why this is the right fix (not a workaround)
AVPlayerLayervia its built-in pipeline (preferredPeakBitRate,pixelBufferAttributes, the display's HDR capabilities).AVPlayerLayer's natural color pipeline, producing the desaturated grey surface.AVPlayerLayerworkaround layer (the_playerLayer = [AVPlayerLayer playerLayerWithPlayer:self.player]line inFVPTextureBasedVideoPlayer.m:55-56). That workaround was the actual problem — it created a second layer that conflicted with the texture. The fix in 2.9.7 was to force the color properties on the texture; the better fix is to remove the workaround layer entirely (separate PR). For now, removing the color override is the safe minimum fix.Backwards compatibility
flutter teston the plugin and example)Test plan
flutter testonvideo_player_avfoundation— should pass unchangedTracking
apps/mobile/packages/video_player_avfoundation_patch/in robertjanmastenbroek/islandspots — can be removed once this PR merges