[video_player_avfoundation] enable more than 30 fps

[misos1]

In the player there was hardcoded 30 fps when setting up video composition. Now it uses timing from source track and also fallback minFrameDuration as seems frameDuration must be always set to something and it takes over in some situations as is mentioned in documentation about sourceTrackIDForFrameTiming. Also video composition setup is skipped when it is not needed when preferredTransform is identity.

Function updatePlayingState is called often right after setupEventSinkIfReadyToPlay but seems it was forgotten in onListenWithArguments and also it cannot be called in that way because setupEventSinkIfReadyToPlay may finish asynchronously when called again from line [self performSelector:_cmd onThread:NSThread.mainThread withObject:self waitUntilDone:NO] so now is updatePlayingState called right after _isInitialized = YES which is what it needs to even do something.

There was one more obstacle for playing 60 fps videos on 60 hz screen. Seems there are at least two "display links" at play when playing video, one calls function displayLinkFired and other one from flutter engine calls copyPixelBuffer but only when textureFrameAvailable was called previously. But the order in which those two are called is undefined so 16 ms after displayLinkFired may be called copyPixelBuffer and right after that displayLinkFired and so on. But copyPixelBuffer steals the newest pixel buffer from video player output and in displayLinkFired hasNewPixelBufferForItemTime will not report another pixel buffer for time close to that. Then the next frame is not called copyPixelBuffer because textureFrameAvailable was not called and in this way it skips every second frame so it plays video at 30 fps. There was also a synchronization problem with lastKnownAvailableTime. Now pixel buffers are produced and reported just on a single place in displayLinkFired and received with correct synchronization in copyPixelBuffer. Ideally there would be just a single "display link" from flutter engine if it supported also pulling frames instead of only pushing (which is good enough for a camera where the system is pushing frames to us, but from player video output is needed to pull frames). Calling textureFrameAvailable every frame could accomplish that but it looks like this line in flutter engine is calling even when copyPixelBuffer returns NULL and it may be expensive (although there is no need to call it in such case):

sk_sp<flutter::DlImage> image = [self wrapExternalPixelBuffer:_lastPixelBuffer context:context];

Seems there is some bug with the video player using this flutter engine on macos. Looks like the video is playing normally but then it starts "tearing", it looks like it is displaying frames normally but once in a while it shows some frame from the past like some previously cached frame. This is happening on the main branch but rendering on 60 fps exaggerates it (it is not caused by this PR).

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene page, which explains my responsibilities.
• [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• [x] I signed the CLA.
• [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• [x] I linked to at least one issue that this PR fixes in the description above.
• [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] I added new tests to check the change I am making, or this PR is test-exempt.
• [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[stuartmorgan-g]

@misos1 Are you still planning on addressing the remaining comments?

[misos1]

@stuartmorgan Yes, I was waiting for your input as I thought you wanted to handle when minFrameDuration is kCMTimeInvalid differently. I thought of setting frameDuration to some constant in such a case but also emit some warning log message like I saw sometimes a flutter engine does but I did not find any way how that is normally done in plugins.

[stuartmorgan-g]

Sorry, I didn't realize that was waiting for my input. I'll respond there.

[misos1]

Seems there is some bug with the video player using this flutter engine on macos. Looks like the video is playing normally but then it starts "tearing", it looks like it is displaying frames normally but once in a while it shows some frame from the past like some previously cached frame. This is happening on the main branch but rendering on 60 fps exaggerates it (it is not caused by this PR).

Seems this is caused by a bug in the flutter engine. There is this "You need to maintain a strong reference to textureOut until the GPU finishes execution of commands accessing the texture, because the system doesn’t automatically retain it.". But here is textureOut released right after CVMetalTextureCacheCreateTextureFromImage:

https://github.com/flutter/engine/blob/6f802b39ab0669eb6ba3272dff1d34e85febeb77/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm#L233-L249 https://github.com/flutter/engine/blob/6f802b39ab0669eb6ba3272dff1d34e85febeb77/shell/platform/darwin/graphics/FlutterDarwinExternalTextureMetal.mm#L177-L197

I thought that it flashes video frames from the past but I looked closely at video recorded from the screen and for example at one moment for 1/60 of a second it shows a frame from video 12 frames into the future. Maybe when the underlying textureOut is released this memory may be reused and overwritten by AVPlayer for decoding following video frames.

This does not explain so well another thing which I noticed. If copyPixelBuffer returns for some frame NULL then the engine shows a transparent image. But I will assume this is also caused by that use after free bug.

There is also this "Note that Core Video doesn’t explicitly declare any pixel format types as Metal compatible. Specify true for the kCVPixelBufferMetalCompatibilityKey option to create Metal-compatible buffers when creating or requesting Core Video pixel buffers.". Maybe the player should also specify this in pixBuffAttributes when creating AVPlayerItemVideoOutput?

[stuartmorgan-g]

Seems this is caused by a bug in the flutter engine.

Please definitely file an issue with details if you haven't already!

Edited to add: You can cross-reference the engine issue with https://github.com/flutter/flutter/issues/135999, which sounds like the macOS playback issue you are describing here if I'm understanding correctly.

[stuartmorgan-g]

There is also this "Note that Core Video doesn’t explicitly declare any pixel format types as Metal compatible. Specify true for the kCVPixelBufferMetalCompatibilityKey option to create Metal-compatible buffers when creating or requesting Core Video pixel buffers.". Maybe the player should also specify this in pixBuffAttributes when creating AVPlayerItemVideoOutput?

This would probably be a question for the #hackers-engine channel; I'm not familiar with the current end-to-end pipeline requirements for the engine texture rendering path.

[stuartmorgan-g]

In terms of moving this forward, from my perspective:

• Structurally everything here makes sense to me at this point; thanks again for the clear explanations!
• I don't consider the macOS issue blocking as it's an existing issue. If we get feedback that the experience is substantially worse we can always do a follow-up to disable just >30fps for macOS, while leaving all the other improvements (e.g., threading model fixes) in place.

So once the smaller feedback items still open are addressed in an an updated version of the PR, I can re-review, and I expect we'll be on track for getting this landed.

Does that sound right?

[misos1]

If we get feedback that the experience is substantially worse we can always do a follow-up to disable just >30fps for macOS, while leaving all the other improvements (e.g., threading model fixes) in place.

Yes, video composition can be set every time even with affinity and fps forced to 30 but it still happens. I wonder why no one reported this. And there are other things which may seemingly help little with it like retaining pixel buffer by frame updater and creating fresh copy of pixel buffer (I first though that AVPlayer at some point rewrites pixel buffers already returned by copyPixelBufferForItemTime but that is not the case). At least these worked on my specific system, but there is nothing for sure if there is UB, someone may experience it even worse with a 30 fps version.

[stuartmorgan-g]

I wonder why no one reported this.

I think they have, per my edit to this comment above. Unless that's not the same behavior you're describing?

And there are other things which may seemingly help little with it like retaining pixel buffer by frame updater and creating fresh copy of pixel buffer

If we have lifetime issues within the engine pipeline itself, that's definitely something we should fix in the engine rather than try to hack around at the video_player level.

[misos1]

I think they have, per my edit to this comment above. Unless that's not the same behavior you're describing?

Maybe partially, as I wrote, if copyPixelBuffer returns for some frame NULL then the engine shows a transparent image meaning it will flicker into background. I did not experience this on ios although it seems they share the same code so UB should be also on ios.

[misos1]

Would it be doable to add new capability to the flutter engine and depend on it in this package or it needs to also work with the engine prior to such change?

[stuartmorgan-g]

Whether we try to work around engine bugs at the plugin level is something we decide on a case-by-cases basis. If the macOS engine texture pipeline is buggy, I would not attempt to work around that at the plugin level without a very compelling reason to do so.

[misos1]

No I do not mean this, rather for the engine being able to pull pixel buffers rather than needing textureFrameAvailable. Something like a flag when the engine will call copyPixelBuffer for every frame, if it returns NULL it will do nothing, if it returns pixel buffer it will update internal buffers.

[stuartmorgan-g]

I'm not really sure what you mean by "for every frame", but if you want to propose a new engine API the place to start would be a design document. Details of exactly when video_player would adopt a new proposed API would be figured out much later in the process.

[misos1]

By "for every frame" I mean that it would be called as if _textureFrameAvailable was always true. _textureFrameAvailable is set to true by calling textureFrameAvailable and set to false soon after is copyPixelBuffer called.

[misos1]

Regarding https://github.com/flutter/packages/pull/7466#issuecomment-2377628306, it seems that macOS actually uses some different code path than ios but here it is the same, cvMetalTexture is released right after CVMetalTextureCacheCreateTextureFromImage:

https://github.com/flutter/engine/blob/6f802b39ab0669eb6ba3272dff1d34e85febeb77/shell/platform/darwin/macos/framework/Source/FlutterExternalTexture.mm#L119-L135 https://github.com/flutter/engine/blob/6f802b39ab0669eb6ba3272dff1d34e85febeb77/shell/platform/darwin/macos/framework/Source/FlutterExternalTexture.mm#L77-L97

For both it seems like a change here was some 4 years ago so I am not sure why such similar things were done twice.

And this is eventually called from embedder_external_texture_metal.mm which handles things differently, it does not use flag like _textureFrameAvailable like is used in FlutterDarwinExternalTextureMetal.mm but instead it nullifies last stored image when is called textureFrameAvailable so if copyPixelBuffer returns NULL it will not show anything and will periodically call it until it returns non-NULL and this explains what I observed.

I am not sure if this is intended or not but FlutterTexture does not tell anything about whether copyPixelBuffer can or cannot return NULL or what should happen in such a case:

/**
 * Copy the contents of the texture into a `CVPixelBuffer`.
 *
 * The type of the pixel buffer is one of the following:
 * - `kCVPixelFormatType_32BGRA`
 * - `kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange`
 * - `kCVPixelFormatType_420YpCbCr8BiPlanarFullRange`
 */
- (CVPixelBufferRef _Nullable)copyPixelBuffer;

So I will assume that returning NULL from copyPixelBuffer after previously returning non-NULL is undefined (it is practically unavoidable to return NULL at least once at start because the engine will call it until it returns non-NULL even without calling textureFrameAvailable). Then both old and current implementation are not entirely correct because they can return NULL from copyPixelBuffer because copyPixelBufferForItemTime can return NULL anytime so player needs to remember last returned pixel buffer and if it does not have any new then return this remembered buffer again until there is some new.

[cbracken]

@misos1 when you get a chance, please review @stuartmorgan's latest round of comments. Thanks for your contribution.

[misos1]

I suppose I have to merge the latest upstream for these failing checks to pass? "warning - lib/src/common/package_looping_command.dart:343:30 - The receiver can't be null"

[jmagman]

@misos1 the analyzer warnings look unrelated to your change (warnings are related to turning on the analyzer for another part of the code). Could you rebase/merge onto master, and resolve the merge conflicts?

[stuartmorgan-g]

In case this was waiting on me, the changes here LGTM. I was just waiting until the merge to pass CI has happened for final review+approval, since that merge will need a review.

[jmagman]

@misos1 the analyzer warnings look unrelated to your change (warnings are related to turning on the analyzer for another part of the code). Could you rebase/merge onto master, and resolve the merge conflicts?

Hi @misos1, friendly ping that this would still need a rebase to land (to get past the analyzer issues), and also re-update the CHANGELOG and pubspec. It's so close to landing! 🙂