Skip to content
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

fix: Fix Metal NativeBuffer flickering issue #2372

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

mrousavy
Copy link
Contributor

William and I experienced a flickering issue that appeared after a while when playing back a video using the NativeBuffer APIs.
After 5 hours of debugging and countless of print statements and renderer code rewrites, I figured out that the CVMetalTextureCacheRef apparently felt free to overwrite old MTLTexture buffers after a while because we didn't hold a strong reference to the texture holder!

In our case CVMetalTextureCacheCreateTextureFromImage gave us a CVMetalTextureRef, which we only used for the lifetime of the getSkiaTextureForCVPixelBufferPlane method, when in reality we should've held a reference on this for longer.

Effectively this CFRelease was the reason our metal texture cache pool felt free to overwrite old buffers:

  // 1. Get Metal Texture from Sample Buffer
  CVMetalTextureRef textureHolder;
  CVReturn result = CVMetalTextureCacheCreateTextureFromImage(
      kCFAllocatorDefault, textureCache, pixelBuffer, nil, pixelFormat, width,
      height, planeIndex, &textureHolder);

  // 2. Unwrap the underlying MTLTexture
  id<MTLTexture> mtlTexture = CVMetalTextureGetTexture(textureHolder);
  if (mtlTexture == nil) [[unlikely]] {
    throw std::runtime_error(
        "Failed to get MTLTexture from CVMetalTextureRef!");
  }

  // 3. Wrap MTLTexture in Skia's GrBackendTexture
  GrMtlTextureInfo textureInfo;
  textureInfo.fTexture.retain((__bridge void *)mtlTexture);
  GrBackendTexture texture =
      GrBackendTexture((int)mtlTexture.width, (int)mtlTexture.height,
                       skgpu::Mipmapped::kNo, textureInfo);
- CFRelease(textureHolder);
  return texture;

With this PR this is now fixed, and CFRelease(textureHolder) is now only called when the SkImage is destroyed.

cc @wcandillon

@mrousavy
Copy link
Contributor Author

So I just did some thorough testing and this definitely fixes the issue in your MKBHD video example for me.

In VisionCamera, I saw two issues;

  1. This one (fixed in this PR)
  2. One where the Skia context was re-created multiple times for each thread

And for issue 2., the screen started to flicker black a lot. I think this is caused by Metal having an issue with using multiple contexts per dispatch_queue (which we are doing because we use thread_local, but on iOS, the DispatchQueue can have multiple C++ threads) - the dispatch_queue PR #2368 fixes that issue for me.

So with both of those changes in my app, the Camera feed runs smoothly without a single flicker - I had it run 5 minutes.

@wcandillon wcandillon self-requested a review April 17, 2024 03:03
@wcandillon
Copy link
Contributor

very nice catch 🙏

@wcandillon wcandillon merged commit 2f6ff85 into Shopify:main Apr 17, 2024
11 checks passed
Copy link
Contributor

🎉 This PR is included in version 1.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants