-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[video_player]fix ios 16 bug where encrypted video stream is not showing #6442
[video_player]fix ios 16 bug where encrypted video stream is not showing #6442
Conversation
e6c34fd
to
da66ba8
Compare
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.
Can we only enable this for m3u8 files?
File extensions may not be reliable, and it is not realistic to dig into the m3u8 specs and inspect the metadata.
This would instead be done by introspecting the file uniform type identifier, which would probably be public.m3u-playlist
. https://developer.apple.com/documentation/uniformtypeidentifiers/uttype/3551528-m3uplaylist. However it's likely there will be more formats in the future with this issue. We could add that optimization in the future if needed.
Was there any way to add an integration test for this? Could you programmatically tell that audio was not playing? (see flutter/assets-for-api-docs#178)
This seems really fragile.
In terms of the actual Flutter behavior, yes, but based on the diff in flutter/flutter#86613 it seems like it might not actually be a huge change? As discussed there, this is something we've talked about doing in the past. Recent discussions around platform views vs textures (in the context of Android, but they apply more generally) give me pause on actually switching implementation outright, but I think a better option here would probably be:
Once we have some real-world feedback (e.g., comparing issues reported in both versions) we can decide if we want to switch entirely (and potentially spin out the old implementation as a community-supported unendorsed implementation that people could choose to use). Until recently I was against maintaining two versions, but my recent experience makes me much more inclined to a gradual, opt-in roll-out. |
@jmagman which API are you referring to? I found this NSURL API but it's only for local resources.
|
@stuartmorgan thanks for the insight. I think the "opt-in roll-out" strategy is a good idea. It's lucky that we are able to reverse engineer AVFoundation and spot this "loophole", which could be fixed by Apple in the future. When that happens, I don't think it will be possible to access those pixel buffers of protected contents anymore. |
That gets the extended attribute Finder puts on it, which works for local URLs. If you can get the mime type from the |
As discussed offline I think we will skip the file type check. I am still looking into integration tests as suggested. |
@jmagman I looked into this for a few days, but did not find anything about validating if video is playing. So I came up with this workaround to sample 30 screenshots. Having at least 3 distinct screenshots should prove that the video is playing (1 for loading, and 2 for distinct video frames). The original code fails the test and new code passes it. |
packages/video_player/video_player_avfoundation/example/ios/RunnerUITests/VideoPlayerUITests.m
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/example/ios/RunnerUITests/VideoPlayerUITests.m
Show resolved
Hide resolved
packages/video_player/video_player_avfoundation/example/lib/main.dart
Outdated
Show resolved
Hide resolved
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.
error: -[VideoPlayerUITests testEncryptedVideoStream] : ((frames.count >= 3) is true) failed - Must have at least 3 distinct frames.
This failed in CI.
Hmmm, could be network timeout. Let me try adding a delay. |
13be93f
to
5bd7351
Compare
5e6f810
to
4d4d534
Compare
packages/video_player/video_player_avfoundation/example/ios/RunnerUITests/VideoPlayerUITests.m
Outdated
Show resolved
Hide resolved
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.
This does seem brittle and an unfortunate hack but it sounds like it may be the only workaround we have until platform views are adopted, as Stuart mentioned. flutter/flutter#86613
auto label is removed for flutter/plugins, pr: 6442, due to - The status or check suite ios-platform_tests CHANNEL:master PACKAGE_SHARDING:--shardIndex 0 --shardCount 4 has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Unfortunately we can't get the attachments off of Cirrus (that I know of). You could log the png data base64EncodedStringWithOptions and then make a little app to recreate the png from that string on your own machine. |
cb5c320
to
1338027
Compare
I didn't mean to land that change, I meant to try to figure out why the test was failing in CI. If that test is flaky we really need to fix it so it doesn't keep failing... |
@hellohuanlin this one it's not solved by this PR flutter/flutter#116021 |
@SamerOrfali22 As explained in your issue, your video link fails to load even with Apple's vanilla video player. So it is not related to this plugin. You may want to file a radar with Apple here |
The issue
copyPixelBufferForItemTime
always return nil (hasNewPixelBufferForItemTime
returning false) on iOS 16 for encrypted video stream.Research
There aren't too much info online. It seems to be by design that only Apple's
AVPlayerLayer
can access those encrypted video stream on iOS 16.For example, this unresolved issue (5 years ago, but could be the same issue under different format):
Another hint is that iOS 16 introduced a new API
AVPlayerLayer::copyDisplayedPixelBuffer
with a note:Reverse engineering
I almost concluded that there's no way to solve it. But i was wondering "how did Apple solve it in
AVPlayerLayer
"? It must have accessed some private APIs. Something like this:If I were to design AVFoundation, I would probably make it a layered structure, so that the "unlocked" pixel buffers can be piped downstream. This mean that "unlocking" probably happens towards the upstream, hence all outputs (including ours!) can be granted the access. Something like this:
To verify this hypothesis, I created a dummy
AVPlayerLayer
(without adding it to screen), hoping that the hypothetical unlocking happens in the constructor. The result is that only first video frame is displayed, and all other frames are still not accessible.This tells 3 things:
Now based on all the above info, this is what I guess Apple's implementation of AVPlayerLayer is:
So in conclusion, all we need to do is to have a dummy AVPlayerLayer to concurrently play the video, which should invoke this hypothetical
renderNextFrame()
function, and eventually unlock every single frame for us.And it worked :)
Future
It is pretty obvious that Apple's intention is to only allow
AVPlayerLayer
to access those pixel buffers from iOS 16 onwards.To follow this spirit, we should probably rely on platform views with
AVPlayerLayer
attached. This would be a huge change from the current implementation though (CC: @stuartmorgan)Other notes
Can we only enable this workaround only for encrypted files
It is hard to tell if a video stream is encrypted or not. For example, the second answer in this question.
Can we only enable this for m3u8 files?
File extensions may not be reliable, and it is not realistic to dig into the m3u8 specs and inspect the metadata.
Performance impact?
Very minimal impact since the layer is not rendered on screen. Screenshots here.
About tests
The current unit test isn't the best since it validates implementation instead of behavior. A better unit test is to validate that
copyPixelBuffer
actually returns the frames. But this requires setting up the video player properly with encrypted video file and then play the video. Unfortunately I am not too familiar with how this plugin works and how apple's API works yet.List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#111457
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.