ST40p RX: move frame assembly into the transport + RFC 8331 codec dedup by Sakoram · Pull Request #1648 · OpenVisualCloud/Media-Transport-Library

Sakoram · 2026-07-08T09:07:55Z

Summary

Move ST 2110-40 frame-level assembly out of the pipeline RX and into
the transport session, fixing recurring seq_lost/frame-drop
regressions on redundant streams; pipeline RX becomes a thin
zero-copy consumer.
Add shared st40_rfc8331_* codec helpers (payload sizing, header
byte-swap, encode/decode) and migrate every call site off duplicated,
hand-rolled wire-format math: RX transport, TX transport, gstreamer
plugin, RxTxApp, and integration/unit test helpers.
Along the way, fix two independent real bugs found in the old
duplicated formulas (gstreamer plugin, RxTxApp) that undercounted
payload size by 4 bytes at udw_size % 16 == 12.
Replace the TX BAD_PARITY test hook's manual per-field bit-packing
with a post-encode corrective pass, so TX can finally call the shared
encoder instead of hand-building packets; fixes an unsigned-underflow
bug this surfaced in the MTU-fit bounds check.
Docs: stats_guide.md wording cleanup, and an MTL system-setup
instructions update (check VF binding before running gtest).
Tooling: MCP server build/install tools no longer dump entire build
logs into tool output — truncated to a log path + line count + tail.

Commits (10)

Refactor: ST40p RX moves frame assembly into the transport
Add: st40_rfc8331_{payload_bytes,rtp_hdr_bswap,payload_hdr_bswap,encode_packet,decode_packet} helpers
Refactor: ST40 RX adopts st40_rfc8331_decode_packet() for ANC parsing
Refactor: TX ancillary build adopts RFC 8331 codec helpers (size/bswap only)
Refactor: gstreamer plugin and RxTxApp adopt RFC 8331 codec helpers (+ 2 bug fixes)
Refactor: integration/unit ST40 test helpers adopt RFC 8331 codec helpers
Docs: remove changelog framing from stats_guide.md
Docs: teach mtl-system-setup instructions to check VF binding before run_gtest
Refactor: ST40 TX adopts st40_rfc8331_encode_packet() (+ underflow fix)
Refactor: MCP server truncates large build/install output

Testing

Full unit suite: 319/319 pass.
Integration KahawaiTest --gtest_filter=*St40*: 36/36 pass on real VFs.

Frame-level assembly for ST 2110-40 was duplicated between the transport session and the pipeline RX, with subtly different per-port sequence and redundancy logic. The pipeline copy was the source of recurring seq_lost and frame-drop regressions on redundant streams. This change makes the transport the single owner of assembly and turns the pipeline into a thin consumer: - include/st40_api.h: add ST40_TYPE_FRAME_LEVEL surface - struct st40_rx_frame_meta, framebuff_cnt/framebuff_size, notify_frame_ready, st40_rx_put_framebuff. Back-compat shim keeps existing RTP_LEVEL callers working unchanged. - lib/src/st2110/st_rx_ancillary_session.c: implement the FRAME_LEVEL path - frame slot pool, per-frame seq bitmap (popcount-derived seq_lost across ports), pending/inflight handoff for redundant delivery, parse + UDW validation, notify_frame_ready dispatch and slot reclaim on app reject. - lib/src/st2110/pipeline/st40_pipeline_rx.{c,h}: now a single notify_frame_ready handler that pins the transport pool's UDW buffer zero-copy into the pipeline framebuff and returns -EBUSY when the pool is empty; frame status enum trimmed to FREE/READY/IN_USER to match the transport-owned assembly model. - tests/unit: drop the pipeline-only ST40p harness/tests (the logic they covered now lives in the transport) and grow the session-level ST40 harness/tests to cover frame assembly, redundancy, sequence bitmap and slot pool exhaustion. - doc: update design.md and stats_guide.md to reflect the new API and counters. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

…de_packet,decode_packet} helpers RFC 8331 wire-format math (10-bit-packed DID/SDID/DC/UDW[]/checksum, ceil-to-byte, 4-byte align) and the header byte-swap dance are duplicated across TX build, RX parse, gstreamer, RxTxApp and test call sites. Add the primitives to st_ancillary.c / st40_api.h as a pure addition, no call sites migrated yet (follow-up commits). st40_get_udw()/st40_calc_checksum() are const-correct now (ABI unchanged). encode_packet()/decode_packet() do the full parity+checksum+advance dance in one call and reject udw_size > 255 (wire field is 8 bits) with -EINVAL. st_tx_ancillary_session.c intentionally does not adopt the encoder: its TX_ANC_TEST_APPLY_PARITY hook needs direct primitive access. New tests: rfc8331_payload_bytes_test.cpp, rfc8331_codec_test.cpp. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

rx_anc_slot_parse_pkt() delegates per-ANC-subpacket wire parsing (header decode, parity, checksum, UDW extraction) to st40_rfc8331_decode_packet() instead of hand-rolling it inline. Partial-success semantics and stat_anc_pkt_parse_err bumps are preserved; meta_entry->udw_offset is set from the frame-relative slot->udw_buffer_fill after each successful decode. Two intentional behavior deltas vs the old inline parser: DID/SDID/DC parity is now validated (previously wasn't), and the checksum is now validated even for udw_size == 0 sub-packets (previously skipped, though RFC 8331 defines the checksum over DID+SDID+DC+UDW regardless of UDW length). Both are pinned by new tests: DecodeChecksumFailZeroUdw, ChecksumFailureRecordedForEmptyAncPacket. Also adds CrossPacketUdwOffsetThreadsAcrossRtpPackets, pinning that udw_offset threads correctly across two separate RTP packets within the same frame. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

tx_ancillary_session_build_packet()/build_rtp_packet() use st40_rfc8331_payload_bytes() for the size formula and st40_rfc8331_rtp_hdr_bswap()/payload_hdr_bswap() at all 9 htonl() toggle sites in this file. Pure mechanical substitution, no behavior change (314/314 unit tests pass unchanged). TX still calls st40_set_udw()/tx_ancillary_apply_parity()/ st40_calc_checksum() directly rather than st40_rfc8331_encode_packet(): tx_ancillary_apply_parity()'s TX_ANC_TEST_APPLY_PARITY test hook needs direct primitive access, which the single-call encoder can't accommodate. Documented on encode_packet()'s doc comment so the gap doesn't need git-archaeology to explain. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

Replace manual ntohl/htonl byte-swap pairs with st40_rfc8331_payload_hdr_bswap() in gst_mtl_st40_rx.c, gst_mtl_st40p_tx.c, rx_ancillary_app.c and tx_ancillary_app.c. Also fixes two independent latent formula bugs found while migrating, both undercounting the aligned payload size by 4 bytes at udw_size % 16 == 12 (verified numerically over the full 0-255 range): gst_mtl_st40_rx_shift_payload_hdr()'s bespoke WORD_10_BIT_ALIGN formula misparses the next ANC sub-packet when triggered; RxTxApp's floor-division formula sends 4 bytes of uninitialized mbuf tail on TX and over-advances the payload pointer on RX. Both now use st40_rfc8331_payload_bytes(), same bug class as 2082f10/c0f12995. New test pins the exact divergence. The gstreamer plugin cannot be compiled/tested in this environment (gstreamer-1.0 dev package not installed); verified by code review and the numeric proof instead. RxTxApp is covered by the production build. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

…pers Replace manual ntohl/htonl byte-swap pairs and duplicated payload-size formulas in st40_test.cpp, st40i_tests.cpp and st40_harness.c with st40_rfc8331_payload_hdr_bswap()/st40_rfc8331_rtp_hdr_bswap()/ st40_rfc8331_payload_bytes(). st40_test.cpp's tx_anc_build_rtp_packet() and rx_handle_rtp() used a floor-division variant of the size formula (missing the +7 before the /8 ceil-division, and an alignment step that adds 4 extra bytes when already 4-byte aligned instead of 0). Adopting the canonical helper fixes this rather than reproducing the bug. st40i_tests.cpp already had the correct ceil formula; st40_harness.c's ut40_anc_payload_bytes() now delegates to the canonical helper instead of duplicating it. Individual translation units for the two integration test files were verified to compile cleanly (zero warnings under -Werror -Wall -Wvla); a full KahawaiTest relink could not be completed in this environment due to build/tests/ object files being repeatedly evicted between shell invocations (reproduced on unrelated, unedited files too, so this is an environment artifact, not a code issue -- see /memories/repo/build-notes.md). st40_harness.c is covered by the full unit suite: 314/314 pass. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

'From the ST40_TYPE_FRAME_LEVEL transport refactor onwards' describes what changed rather than how the stat works today. stats_guide.md documents current behavior, not history — drop the changelog phrasing, keep the factual description. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

…run_gtest Observed failure mode: an agent called run_gtest relying on port auto-discovery without checking dpdk_devbind_status first, hit a bare PF bound directly to vfio-pci (0 VFs, invalid for E810/E830), and spent many extra tool calls diagnosing multicast-join/IGMP failures before finding the real cause. Add a VF-binding check before the first run_gtest call, and a dedicated "Multicast join / IGMP failure" decision-tree entry. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

tx_ancillary_session_build_packet() and build_rtp_packet() built each ANC sub-packet by hand (did/sdid/data_count/UDW/checksum) because the BAD_PARITY test hook needed to intercept every parity-encoded field. Both now call the shared st40_rfc8331_encode_packet() codec instead; TX_ANC_TEST_APPLY_PARITY is replaced by TX_ANC_TEST_CORRUPT_PARITY, a post-encode corrective pass on the finished wire buffer. Deriving room from the accumulated payload offset exposed an unsigned underflow once the running byte count reached max_pkt_len (the prior loop-entry break only estimated UDW bit cost, not per-packet fixed overhead), which wrapped room to a huge value and defeated the new bounds check. Both loops now bail out via the existing STI_FRAME_ANC_TOO_LARGE path before the subtraction can wrap, and the break heuristic uses the exact per-packet byte size instead. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

install_dependencies, dpdk_build, ice_driver_rebuild, build_mtl, and the ecosystem/eBPF build tools returned entire build logs (hundreds to thousands of lines) inline in the tool result. Add _summarize_output(), reusing the existing _save_test_log() log-file convention from run_gtest, to save the full output to build/logs/ and return only the log path, line count, and last 40 lines. run_gtest now uses the same helper instead of its own inline tail/log-path logic. Signed-off-by: Kasiewicz, Marek <marek.kasiewicz@intel.com>

moleksy · 2026-07-09T14:59:31Z

  /**
-   * Optional. the callback when lib finish the sending of one rtp packet. And only
-   * non-block method can be used in this callback as it run from lcore tasklet routine.
+   * Optional for ST40_TYPE_RTP_LEVEL. The callback when lib receive one rtp packet.


Says optional, but rx_ancillary_ops_check will not set ops->type ST40_TYPE_RTP_LEVEL without it.

moleksy · 2026-07-09T15:23:17Z

+int st40_rx_put_framebuff(st40_rx_handle handle, void* frame) {
+  struct st_rx_ancillary_session_handle_impl* s_impl = handle;
+  struct st_rx_ancillary_session_impl* s;
+
+  if (s_impl->type != MT_HANDLE_RX_ANC) {
+    err("%s, invalid type %d\n", __func__, s_impl->type);
+    return -EIO;
+  }
+
+  s = s_impl->impl;
+  if (!s->frame_slots) {
+    err("%s(%d), session is not in FRAME_LEVEL mode\n", __func__, s->idx);
+    return -EIO;
+  }
+
+  for (uint16_t i = 0; i < s->frame_slots_cnt; i++) {
+    struct st_rx_anc_frame_slot* slot = &s->frame_slots[i];
+    if (slot->udw_buf == frame) {
+      atomic_store_explicit(&slot->state, ST_RX_ANC_SLOT_FREE, memory_order_seq_cst);
+      return 0;
+    }
+  }
+
+  err("%s(%d), invalid frame %p\n", __func__, s->idx, frame);
+  return -EIO;
+}
+


Possible race condition on s_impl type check, st40_rx_free() can free the session mid-call. Every sibling API uses HANDLE_GUARD / HANDLE_RELEASE

Suggested change

int st40_rx_put_framebuff(st40_rx_handle handle, void* frame) {

struct st_rx_ancillary_session_handle_impl* s_impl = handle;

struct st_rx_ancillary_session_impl* s;

if (s_impl->type != MT_HANDLE_RX_ANC) {

err("%s, invalid type %d\n", __func__, s_impl->type);

return -EIO;

}

s = s_impl->impl;

if (!s->frame_slots) {

err("%s(%d), session is not in FRAME_LEVEL mode\n", __func__, s->idx);

return -EIO;

}

for (uint16_t i = 0; i < s->frame_slots_cnt; i++) {

struct st_rx_anc_frame_slot* slot = &s->frame_slots[i];

if (slot->udw_buf == frame) {

atomic_store_explicit(&slot->state, ST_RX_ANC_SLOT_FREE, memory_order_seq_cst);

return 0;

}

}

err("%s(%d), invalid frame %p\n", __func__, s->idx, frame);

return -EIO;

}

int st40_rx_put_framebuff(st40_rx_handle handle, void* frame) {

struct st_rx_ancillary_session_handle_impl* s_impl = handle;

struct st_rx_ancillary_session_impl* s;

int ret = -EIO;

MT_HANDLE_GUARD(s_impl, MT_HANDLE_RX_ANC, -EIO);

s = s_impl->impl;

if (!s->frame_slots) {

err("%s(%d), session is not in FRAME_LEVEL mode\n", __func__, s->idx);

goto out;

}

for (uint16_t i = 0; i < s->frame_slots_cnt; i++) {

struct st_rx_anc_frame_slot* slot = &s->frame_slots[i];

if (slot->udw_buf == frame) {

atomic_store_explicit(&slot->state, ST_RX_ANC_SLOT_FREE, memory_order_seq_cst);

ret = 0;

goto out;

}

}

err("%s(%d), invalid frame %p\n", __func__, s->idx, frame);

out:

MT_HANDLE_RELEASE(s_impl);

return ret;

}

moleksy · 2026-07-09T15:28:06Z

+      break;
+    }
+
+    meta_entry->udw_offset = udw_offset;


st40_meta.udw_offset is uint16_t, and here line 211 we have uint32_t udw_offset = slot->udw_buffer_fill;

When sum of UDW passes 65535, the offset silently wraps. Let's widen the field or document/enforce a framebuff_size ≤ 64KB limit.

This was referenced Jul 8, 2026

Refactor: ST40p RX moves frame assembly into the transport #1534

Closed

Refactor: Consolidate RFC 8331 ANC encode/decode helpers #1590

Closed

Sakoram force-pushed the St40p-rx-move-frame-assembly-into-the-transport branch 2 times, most recently from 28cc744 to 8c0e371 Compare July 8, 2026 11:26

Sakoram added 8 commits July 8, 2026 11:37

Sakoram force-pushed the St40p-rx-move-frame-assembly-into-the-transport branch from 8c0e371 to bb0bb88 Compare July 8, 2026 11:38

Sakoram marked this pull request as ready for review July 8, 2026 13:05

Sakoram requested review from DawidWesierski4, awilczyns, moleksy and soopel as code owners July 8, 2026 13:05

Sakoram changed the title ~~Refactor: ST40p RX moves frame assembly into the transport~~ ST40p RX: move frame assembly into the transport + RFC 8331 codec dedup Jul 8, 2026

Sakoram force-pushed the St40p-rx-move-frame-assembly-into-the-transport branch from 95458ad to bf67e4c Compare July 8, 2026 13:09

Sakoram requested review from moleksy and removed request for moleksy July 9, 2026 11:55

moleksy reviewed Jul 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ST40p RX: move frame assembly into the transport + RFC 8331 codec dedup#1648

ST40p RX: move frame assembly into the transport + RFC 8331 codec dedup#1648
Sakoram wants to merge 10 commits into
mainfrom
St40p-rx-move-frame-assembly-into-the-transport

Sakoram commented Jul 8, 2026 •

edited

Loading

Uh oh!

moleksy Jul 9, 2026

Uh oh!

moleksy Jul 9, 2026

Uh oh!

moleksy Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Sakoram commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Commits (10)

Testing

Uh oh!

moleksy Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

moleksy Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

moleksy Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sakoram commented Jul 8, 2026 •

edited

Loading