Skip to content

fix(rtc): use the right plane layout for I420A, I422 and NV12 frames - #864

Open
ANDROIDHASSAN wants to merge 1 commit into
livekit:mainfrom
ANDROIDHASSAN:fix/video-plane-layout
Open

ANDROIDHASSAN wants to merge 1 commit into
livekit:mainfrom
ANDROIDHASSAN:fix/video-plane-layout

Conversation

@ANDROIDHASSAN

Copy link
Copy Markdown
Contributor

Two helpers in video_frame.py compute the layout of a packed frame, and three formats were off:

  • _get_plane_length sized the chroma of I420A and NV12 as chroma_width * chroma_width instead of chroma_width * chroma_height. _from_owned_info copies that many bytes out of FFI memory for every frame a VideoStream delivers in that format, so a landscape frame is copied past the end of its buffer and a portrait one comes back short. For 640x480: NV12 512000 bytes instead of 460800, I420A 819200 instead of 768000.
  • _get_plane_infos started the I422 V plane at u.data_ptr + u.size + u.size, one U plane too late, which is the end of the buffer. get_plane(2) reads from there, and _proto_info sends that pointer to the FFI on VideoSource.capture_frame.

The fix is the three formulas; I420, I444 and I010 already matched.

Tests in tests/rtc/test_video_frame.py:

  • test_planes_are_packed_and_match_the_frame_length: for every planar format and 640x480, 480x640 and 5x3, the plane sizes match the format, the planes follow each other with no gap, and _get_plane_length equals their total. Fails on main for I420A, I422 and NV12 at every size.
  • test_frame_from_the_ffi_copies_exactly_the_frame[I420A|NV12]: _from_owned_info on a 640x480 buffer copies exactly the frame (the buffer is allocated larger, so main over-reads safely and the test just fails).
  • test_get_plane_returns_each_i422_plane: get_plane(0..2) returns the Y, U and V bytes of an I422 frame. Fails on main.

Ran tests/rtc against the published livekit wheel with video_frame.py (and main's audio_mixer.py) swapped in: the new tests are the only difference between main and this branch (test_agc/test_apm/test_resampler fail identically on both here, from missing local fixtures). ruff format --check, ruff check (0.15.4) and mypy on the module pass.

Scope: left alone, each separate from these formulas: VideoFrame.__init__ does not check len(data) against _get_plane_length, so a short buffer can still be over-read by get_plane/capture_frame; _from_owned_info assumes one packed buffer rather than reading info.components; get_plane accepts a negative index.

_get_plane_length sized I420A and NV12 chroma as chroma_width *
chroma_width instead of chroma_width * chroma_height, so a frame received
from the FFI (VideoStream) was copied with the wrong length: past the end
of the buffer for a landscape frame, short for a portrait one.
_get_plane_infos started the I422 V plane one U plane too late, at the end
of the buffer, which get_plane(2) and capture_frame then read from.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant