Improve video filter pipeline Pipeline by stabilizing bitmap lifecycle#1600
Improve video filter pipeline Pipeline by stabilizing bitmap lifecycle#1600aleksandar-apostolov merged 8 commits intodevelopfrom
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
rahul-lohra
added
pr:bug
SDK Size Comparison 📏
|
rahul-lohra
changed the title
|
Why is this PR towards |
Fixed—now pointing to develop. @aleksandar-apostolov |
WalkthroughThe changes refactor video frame processing to improve resource initialization and lifecycle management. FilterVideoProcessor now uses initializeGLResources() instead of initialize() and defers bitmap recycling to after frame emission. YuvFrame's bitmapFromVideoFrame method is rewritten to use direct I420 wrapping and rotation with more contained buffer handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/FilterVideoProcessor.kt`:
- Around line 109-126: The I420Buffer created via I420Buffer.wrap (referred to
as libYuvI420) inside YuvFrame.bitmapFromVideoFrame is never closed, causing a
leak; modify YuvFrame.bitmapFromVideoFrame to call libYuvI420.release() before
returning in the successful (try) path and ensure that in the catch block you
release any allocated i420Buffer (and libYuvI420 if allocated) to avoid leaks;
locate the I420Buffer.wrap usage and add explicit .release() calls for
libYuvI420 and for i420Buffer in the respective try and catch branches so both
ref-counted buffers are always cleaned up.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/YuvFrame.kt`:
- Around line 66-71: The code can leak rotatedI420 and abgrBuffer if an
exception occurs during allocation/convert/asBitmap; wrap their lifetimes in
structured cleanup (use try/finally or Kotlin .use) so rotatedI420.close()
always runs after rotateI420(...) and abgrBuffer.close() always runs after
AbgrBuffer.allocate(...)/convertTo(...)/asBitmap(), and ensure
i420Buffer.release() is invoked in the outer finally; locate the bitmap
conversion function (e.g., bitmapFromVideoFrame / VideoFrame.buffer.toI420
usage) and add nested try/finally (or use) blocks around rotatedI420 and
abgrBuffer and a final finally that calls i420Buffer.release(), also catch/log
the throwable and return null as before.
- Around line 42-75: The try block can throw before i420Buffer.release() is
called, leaking native memory; update the conversion to ensure i420Buffer is
always released (and also close rotatedI420 and abgrBuffer if they were
allocated) by moving cleanup into a finally block or using Kotlin's
use/try-with-resources pattern: after wrapping with I420Buffer.wrap and
allocating rotatedI420/abgrBuffer (functions/classes: I420Buffer.wrap,
rotateI420, rotatedI420, AbgrBuffer.allocate, abgrBuffer, asBitmap), ensure
i420Buffer.release() is invoked in finally (and call rotatedI420.close() and
abgrBuffer.close() only if they were created) so all native resources are freed
on both success and exception paths.
🧹 Nitpick comments (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/YuvFrame.kt (1)
28-31: TODO comment indicates architectural concern.The TODO suggests this should be an
internal classrather thanobject. SinceYuvFrameis now stateless (no stored buffers), this change would be straightforward and would improve testability and avoid any potential threading concerns with a global singleton.Would you like me to help refactor this from
objectto aninternal class?stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/FilterVideoProcessor.kt (1)
23-30: Remove unused imports.The imports
androidx.core.graphics.getandkotlin.getValueare not used in this file.♻️ Remove unused imports
-import androidx.core.graphics.get import org.webrtc.SurfaceTextureHelper import org.webrtc.TextureBufferImpl import org.webrtc.VideoFrame import org.webrtc.VideoProcessor import org.webrtc.VideoSink import org.webrtc.YuvConverter -import kotlin.getValue
| private fun initializeGLResources(width: Int, height: Int, textureHelper: SurfaceTextureHelper) { | ||
| yuvBuffer?.release() | ||
|
|
||
| if (this.inputWidth != width || this.inputHeight != height) { | ||
| this.inputWidth = width | ||
| this.inputHeight = height | ||
| inputFrameBitmap?.recycle() | ||
| inputBuffer?.release() | ||
|
|
||
| val type = VideoFrame.TextureBuffer.Type.RGB | ||
|
|
||
| val matrix = Matrix() | ||
| // This is vertical flip - we need to investigate why the image is flipped vertically and | ||
| // why we need to correct it here. | ||
| matrix.preScale(1.0f, -1.0f) | ||
| val surfaceTextureHelper: SurfaceTextureHelper = textureHelper | ||
|
|
||
| this.inputBuffer = TextureBufferImpl( | ||
| inputWidth, inputHeight, type, textures[0], matrix, surfaceTextureHelper.handler, | ||
| inputWidth, inputHeight, type, textures[0], matrix, textureHelper.handler, | ||
| yuvConverter, null as Runnable?, | ||
| ) | ||
| this.inputFrameBitmap = | ||
| Bitmap.createBitmap(this.inputWidth, this.inputHeight, Bitmap.Config.ARGB_8888) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the YuvFrame class
fd -t f "YuvFrame" --extension kt --extension javaRepository: GetStream/stream-video-android
Length of output: 170
🏁 Script executed:
# Also search for references to bitmapFromVideoFrame
rg "bitmapFromVideoFrame" -t kt -t javaRepository: GetStream/stream-video-android
Length of output: 100
🏁 Script executed:
# Find the FilterVideoProcessor.kt file to see the context
fd -t f "FilterVideoProcessor" --extension ktRepository: GetStream/stream-video-android
Length of output: 182
🌐 Web query:
libyuv I420Buffer.wrap Android WebRTC explicit close cleanup requirement
💡 Result:
Short answer: yes — you must release I420 buffers when they're ref‑counted. JavaI420Buffer.wrap just wraps direct ByteBuffers (no copy) and accepts an optional releaseCallback; the Java I420 buffer is ref‑counted and callers should call release() (or provide a releaseCallback) when finished to avoid native memory leaks. [1][2][3]
Sources:
- JavaI420Buffer.wrap implementation (wraps direct ByteBuffers; accepts releaseCallback). [1]
- Example usage showing i420Buffer.release() in WebRTC Android code. [2]
- API docs noting JavaI420Buffer.wrap signature. [3]
🏁 Script executed:
cat -n stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/YuvFrame.ktRepository: GetStream/stream-video-android
Length of output: 4450
🏁 Script executed:
sed -n '109,126p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/FilterVideoProcessor.ktRepository: GetStream/stream-video-android
Length of output: 738
🏁 Script executed:
sed -n '100,130p' stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/FilterVideoProcessor.ktRepository: GetStream/stream-video-android
Length of output: 948
Close libYuvI420 to prevent resource leak in YuvFrame.bitmapFromVideoFrame.
The method rename to initializeGLResources is appropriate and the GL resource cleanup logic is correct. However, the libYuvI420 buffer created via I420Buffer.wrap() at line 44 is never closed. This is a resource leak—I420Buffer is ref-counted and requires explicit cleanup. Additionally, in the exception path (catch block), both libYuvI420 and i420Buffer are leaked. Close libYuvI420 in the try block before returning, and ensure i420Buffer is released in the catch block.
🤖 Prompt for AI Agents
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/FilterVideoProcessor.kt`
around lines 109 - 126, The I420Buffer created via I420Buffer.wrap (referred to
as libYuvI420) inside YuvFrame.bitmapFromVideoFrame is never closed, causing a
leak; modify YuvFrame.bitmapFromVideoFrame to call libYuvI420.release() before
returning in the successful (try) path and ensure that in the catch block you
release any allocated i420Buffer (and libYuvI420 if allocated) to avoid leaks;
locate the I420Buffer.wrap usage and add explicit .release() calls for
libYuvI420 and for i420Buffer in the respective try and catch branches so both
ref-counted buffers are always cleaned up.
| try { | ||
| // Wrap WebRTC I420 planes (borrowed, read-only) | ||
| val libYuvI420 = I420Buffer.wrap( | ||
| planeY = PlanePrimitive(RowStride(i420Buffer.strideY), i420Buffer.dataY), | ||
| planeU = PlanePrimitive(RowStride(i420Buffer.strideU), i420Buffer.dataU), | ||
| planeV = PlanePrimitive(RowStride(i420Buffer.strideV), i420Buffer.dataV), | ||
| width = i420Buffer.width, | ||
| height = i420Buffer.height, | ||
| ) | ||
|
|
||
| libYuvI420Buffer = I420Buffer.wrap( | ||
| planeY = PlanePrimitive(RowStride(webRtcI420Buffer.strideY), webRtcI420Buffer.dataY), | ||
| planeU = PlanePrimitive(RowStride(webRtcI420Buffer.strideU), webRtcI420Buffer.dataU), | ||
| planeV = PlanePrimitive(RowStride(webRtcI420Buffer.strideV), webRtcI420Buffer.dataV), | ||
| width = width, | ||
| height = height, | ||
| ) | ||
| } | ||
| // Rotate into a new owned buffer | ||
| val rotatedI420 = rotateI420( | ||
| source = libYuvI420, | ||
| rotationDegrees = videoFrame.rotation, | ||
| ) | ||
|
|
||
| private fun rotateLibYuvI420Buffer(rotationDegrees: Int) { | ||
| val width = webRtcI420Buffer.width | ||
| val height = webRtcI420Buffer.height | ||
| // Convert to ABGR (owned buffer) | ||
| val abgrBuffer = AbgrBuffer.allocate( | ||
| rotatedI420.width, | ||
| rotatedI420.height, | ||
| ) | ||
|
|
||
| when (rotationDegrees) { | ||
| 90, -270 -> changeOrientation(width, height, RotateMode.ROTATE_90) // upside down, 90 | ||
| 180, -180 -> keepOrientation(width, height, RotateMode.ROTATE_180) // right, 180 | ||
| 270, -90 -> changeOrientation(width, height, RotateMode.ROTATE_270) // upright, 270 | ||
| else -> keepOrientation(width, height, RotateMode.ROTATE_0) // left, 0, default | ||
| } | ||
| } | ||
| rotatedI420.convertTo(abgrBuffer) | ||
|
|
||
| private fun changeOrientation(width: Int, height: Int, rotateMode: RotateMode) { | ||
| libYuvRotatedI420Buffer?.close() | ||
| libYuvRotatedI420Buffer = I420Buffer.allocate(height, width) // swapped width and height | ||
| libYuvI420Buffer.rotate(libYuvRotatedI420Buffer!!, rotateMode) | ||
| // Materialize Bitmap | ||
| val bmp = abgrBuffer.asBitmap() | ||
| rotatedI420.close() | ||
| abgrBuffer.close() | ||
| i420Buffer.release() | ||
| return bmp | ||
| } catch (t: Throwable) { | ||
| logger.e(t) { "Failed to convert VideoFrame to Bitmap" } | ||
| return null | ||
| } |
There was a problem hiding this comment.
Resource leak in error path: i420Buffer is not released on failure.
When the try block throws an exception, i420Buffer.release() (line 70) is never called. This can leak native memory. The i420Buffer should be released in a finally block or restructured to ensure cleanup.
🐛 Proposed fix using try-finally
fun bitmapFromVideoFrame(videoFrame: VideoFrame?): Bitmap? {
if (videoFrame == null) return null
val i420Buffer = videoFrame.buffer.toI420() ?: return null
try {
// Wrap WebRTC I420 planes (borrowed, read-only)
val libYuvI420 = I420Buffer.wrap(
planeY = PlanePrimitive(RowStride(i420Buffer.strideY), i420Buffer.dataY),
planeU = PlanePrimitive(RowStride(i420Buffer.strideU), i420Buffer.dataU),
planeV = PlanePrimitive(RowStride(i420Buffer.strideV), i420Buffer.dataV),
width = i420Buffer.width,
height = i420Buffer.height,
)
// Rotate into a new owned buffer
val rotatedI420 = rotateI420(
source = libYuvI420,
rotationDegrees = videoFrame.rotation,
)
// Convert to ABGR (owned buffer)
val abgrBuffer = AbgrBuffer.allocate(
rotatedI420.width,
rotatedI420.height,
)
rotatedI420.convertTo(abgrBuffer)
// Materialize Bitmap
val bmp = abgrBuffer.asBitmap()
rotatedI420.close()
abgrBuffer.close()
- i420Buffer.release()
return bmp
} catch (t: Throwable) {
logger.e(t) { "Failed to convert VideoFrame to Bitmap" }
return null
+ } finally {
+ i420Buffer.release()
}
}🤖 Prompt for AI Agents
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/YuvFrame.kt`
around lines 42 - 75, The try block can throw before i420Buffer.release() is
called, leaking native memory; update the conversion to ensure i420Buffer is
always released (and also close rotatedI420 and abgrBuffer if they were
allocated) by moving cleanup into a finally block or using Kotlin's
use/try-with-resources pattern: after wrapping with I420Buffer.wrap and
allocating rotatedI420/abgrBuffer (functions/classes: I420Buffer.wrap,
rotateI420, rotatedI420, AbgrBuffer.allocate, abgrBuffer, asBitmap), ensure
i420Buffer.release() is invoked in finally (and call rotatedI420.close() and
abgrBuffer.close() only if they were created) so all native resources are freed
on both success and exception paths.
| // Materialize Bitmap | ||
| val bmp = abgrBuffer.asBitmap() | ||
| rotatedI420.close() | ||
| abgrBuffer.close() | ||
| i420Buffer.release() | ||
| return bmp |
There was a problem hiding this comment.
Intermediate buffer leak on exception between allocations.
If an exception occurs after rotatedI420 is created but before it's closed (e.g., during AbgrBuffer.allocate or convertTo), the rotatedI420 buffer will leak. Similarly, if asBitmap() throws, abgrBuffer leaks. Consider using use or structured cleanup.
♻️ Suggested safer cleanup pattern
fun bitmapFromVideoFrame(videoFrame: VideoFrame?): Bitmap? {
if (videoFrame == null) return null
val i420Buffer = videoFrame.buffer.toI420() ?: return null
return try {
val libYuvI420 = I420Buffer.wrap(
planeY = PlanePrimitive(RowStride(i420Buffer.strideY), i420Buffer.dataY),
planeU = PlanePrimitive(RowStride(i420Buffer.strideU), i420Buffer.dataU),
planeV = PlanePrimitive(RowStride(i420Buffer.strideV), i420Buffer.dataV),
width = i420Buffer.width,
height = i420Buffer.height,
)
val rotatedI420 = rotateI420(source = libYuvI420, rotationDegrees = videoFrame.rotation)
try {
val abgrBuffer = AbgrBuffer.allocate(rotatedI420.width, rotatedI420.height)
try {
rotatedI420.convertTo(abgrBuffer)
abgrBuffer.asBitmap()
} finally {
abgrBuffer.close()
}
} finally {
rotatedI420.close()
}
} catch (t: Throwable) {
logger.e(t) { "Failed to convert VideoFrame to Bitmap" }
null
} finally {
i420Buffer.release()
}
}🤖 Prompt for AI Agents
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/video/YuvFrame.kt`
around lines 66 - 71, The code can leak rotatedI420 and abgrBuffer if an
exception occurs during allocation/convert/asBitmap; wrap their lifetimes in
structured cleanup (use try/finally or Kotlin .use) so rotatedI420.close()
always runs after rotateI420(...) and abgrBuffer.close() always runs after
AbgrBuffer.allocate(...)/convertTo(...)/asBitmap(), and ensure
i420Buffer.release() is invoked in the outer finally; locate the bitmap
conversion function (e.g., bitmapFromVideoFrame / VideoFrame.buffer.toI420
usage) and add nested try/finally (or use) blocks around rotatedI420 and
abgrBuffer and a final finally that calls i420Buffer.release(), also catch/log
the throwable and return null as before.
|
2 participants
Goal
Prevent beginning & intermittent black frames when video filters are enabled by ensuring bitmap resources are not recycled or recreated during adaptive resolution changes. This change makes the video filter pipeline resilient to dynamic frame size updates caused by adaptive streaming, eliminating visible flicker during filter activation and resolution switches.
Implementation
Problem
When video filters are enabled,
FilterVideoProcessorconverts incoming frames to a Bitmap for bitmap-based filters.Previously, whenever the frame width or height changed (which can happen frequently due to adaptive streaming), the pipeline:
During this transition, we lost the original bitmap and as result we rendered a black bitmap
Solution
🎨 UI Changes
None
Testing
Apply a filter and there should no black frame
Summary by CodeRabbit
Bug Fixes
Performance Improvements
✏️ Tip: You can customize this high-level summary in your review settings.