feat: iOS video renderer improvements - #77
Conversation
…l implementation from webrtc renderre
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes replace iOS sample-buffer rendering with WebRTC shared Metal rendering, correct previous-track detachment, and update iOS/macOS texture renderers to reuse buffers, optimize NV12 delivery, handle rotation efficiently, and retain the current frame buffer. ChangesVideo rendering pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RTCVideoFrame
participant FlutterRTCVideoRenderer
participant FlutterTextureRegistry
RTCVideoFrame->>FlutterRTCVideoRenderer: provide video frame
FlutterRTCVideoRenderer->>FlutterRTCVideoRenderer: select zero-copy, NV12, or I420 output
FlutterRTCVideoRenderer->>FlutterTextureRegistry: mark texture frame available
FlutterTextureRegistry->>FlutterRTCVideoRenderer: request current pixel buffer
FlutterRTCVideoRenderer-->>FlutterTextureRegistry: return retained current buffer
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ios/stream_webrtc_flutter/Sources/stream_webrtc_flutter/FlutterRTCVideoRenderer.m`:
- Around line 70-84: Update ensureNV12BufferWithWidth:height: to accept the
source pixel format and use it when creating _nv12BufferRef, rather than always
selecting kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange. Pass renderFrame:’s
fmt through this method so both video-range and full-range NV12 frames retain
their correct color range.
In
`@macos/stream_webrtc_flutter/Sources/stream_webrtc_flutter/FlutterRTCVideoRenderer.m`:
- Around line 70-84: Update ensureNV12BufferWithWidth:height: to accept the
source fmt, create _nv12BufferRef with that format, and include fmt in the
reuse-validity check so a format change recreates the buffer. Propagate fmt from
the crop path and validate the CVPixelBufferCreate result before retaining or
using the buffer.
- Around line 245-293: Double-buffer the cropped NV12 output used by the
renderer: replace the single _nv12BufferRef handoff in cropAndScaleTo: with two
reusable NV12 pixel buffers, selecting the buffer not currently published as
_currentBuffer before each crop. Update ensureNV12BufferWithWidth:height: and
the cleanup/state handling so both buffers are allocated, reused, and released
safely, while preserving the existing zero-copy path for unpadded IOSurface
frames.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6e08697-bff9-4151-a880-c4d2358bde28
📒 Files selected for processing (4)
ios/stream_webrtc_flutter/Sources/stream_webrtc_flutter/FlutterRTCVideoPlatformView.mios/stream_webrtc_flutter/Sources/stream_webrtc_flutter/FlutterRTCVideoPlatformViewController.mios/stream_webrtc_flutter/Sources/stream_webrtc_flutter/FlutterRTCVideoRenderer.mmacos/stream_webrtc_flutter/Sources/stream_webrtc_flutter/FlutterRTCVideoRenderer.m
renefloor
left a comment
renefloor
left a comment
There was a problem hiding this comment.
- _cropTempSize is updated even when malloc fails — FlutterRTCVideoRenderer.m:~278
_cropTempBuffer = malloc((size_t)tmpSize);
_cropTempSize = (size_t)tmpSize; // set unconditionally
On allocation failure _cropTempBuffer == NULL while _cropTempSize claims the buffer is large enough. It self-heals next frame (the == NULL check re-triggers), but this frame calls cropAndScaleTo:withTempBuffer:NULL with a non-zero required size — for NV12 sources bufferSizeForCroppingAndScalingToWidth: always returns non-zero, so libyuv writes through a null pointer. Set the size only on success and skip the crop otherwise.
- The crop target should be frame.width/frame.height, not cropWidth/cropHeight — renderFrame:
RTCCVPixelBuffer carries both a crop rect and an adapted size; frame.width/frame.height are the adapted dimensions, and cropAndScaleTo: is designed to produce exactly those. As written:
- The crop branch allocates and fills at pre-scale resolution, ignoring the downscale the frame adapter asked for.
- The zero-copy branch gates on requiresCropping only — a buffer needing pure scaling (requiresScalingToWidth:height:) passes through at full resolution.
Consequence: the texture's real dimensions can disagree with the didTextureChangeVideoSize value sent to Dart, and you allocate/upload more pixels than requested. Most visible for local camera preview (adapter-scaled 1080p buffers), harmless-looking because Flutter rescales — but it silently defeats resolution adaptation. Passing frame.width/frame.height as the target fixes both branches.
2 participants
Summary by CodeRabbit