Skip to content

chore: point livekit-sdk at client-sdk-unity PR#70 (windows FFI symbols) - #9068

Merged
alejandro-jimenez-dcl merged 7 commits into
devfrom
chore/livekit-ffi-symbols-pr70
Jun 30, 2026
Merged

chore: point livekit-sdk at client-sdk-unity PR#70 (windows FFI symbols)#9068
alejandro-jimenez-dcl merged 7 commits into
devfrom
chore/livekit-ffi-symbols-pr70

Conversation

@alejandro-jimenez-dcl

@alejandro-jimenez-dcl alejandro-jimenez-dcl commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Pull Request Description

What does this PR change?

Pins the com.decentraland.livekit-sdk package to commit 222d67ccc289337dadb5e8d96ee5b633dfb905b6, which corresponds to client-sdk-unity PR#70. That PR builds the native livekit_ffi.dll for Windows with symbols (strip = symbols/debug info retained) instead of the current stripped binary.

Previously the manifest tracked the default branch of client-sdk-unity with no commit hash, so the resolved FFI binary had no PDB/symbol information. When livekit_ffi.dll crashed (e.g. a Rust panic = abort surfacing as 0xC0000409), the resulting minidumps were impossible to symbolicate, leaving native LiveKit crashes effectively undebuggable.

By pinning to the PR#70 commit we get a symbolized Windows FFI build, so future native crash dumps can be resolved to real stack frames.

Files changed

  • Explorer/Packages/manifest.json — pin livekit-sdk to the commit hash.
  • Explorer/Packages/packages-lock.json — corresponding lockfile version/hash update.

Note: this is a temporary pin to a feature branch commit. Once client-sdk-unity PR#70 is merged, this should be re-pointed at the merged commit on the default branch.

Test Instructions (QA)

This change is under the hood. Nothing should look or behave differently. The goal is just to confirm that livestreams still work normally and the client stays stable.

What to test

  1. Launch the client and go to a scene that has a livestream (video stream / "live" screen or stage).
  2. Confirm the livestream plays correctly: the video appears and the audio is heard.
  3. Move around the scene, walk away from the stream and come back, and confirm playback recovers / keeps working.
  4. Try a few different scenes with livestreams if available.
  5. Throughout, confirm the client stays stable and does not crash.

Expected result

Livestreams play exactly as they did before this change (video + audio), and the client does not crash. There should be no visible difference.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered (none — dependency pin only)
  • For SDK features: Test scene is included (N/A)

Code Review Reference

Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.

@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

badge

New build in progress, come back later!

@alejandro-jimenez-dcl
alejandro-jimenez-dcl marked this pull request as ready for review June 26, 2026 13:02
@alejandro-jimenez-dcl
alejandro-jimenez-dcl requested review from a team as code owners June 26, 2026 13:02
@claude

This comment has been minimized.

@claude

This comment has been minimized.

@github-actions github-actions 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.

Auto-approved by Claude — simple fix/chore with no blocking issues. QA approval is still required.

@Ludmilafantaniella Ludmilafantaniella 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.

✅ Approved — tested on Windows and Mac.

Verified livestreams on multiple scenes: video and audio play correctly, playback recovers after moving away and returning, and the client stays stable throughout. No regressions observed. The change is transparent to the end user, as expected.

26.06.2026_15.07.46_REC.mp4

✅Smoke test performed:

  • ✔️ Backpack and wearables in world
  • ✔️ Emotes in world and in backpack
  • ✔️ Teleport with map/coordinates/Jump In
  • ✔️ Chat and multiplayer
  • ✔️ Profile card
  • ✔️ Camera
  • ✔️ Skybox

@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Code Review

  • Gather context and read changed files
  • Analyze diff
  • Post review findings

View job run

@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Claude Code is working…

I'll analyze this and get back to you.

View job run

@claude

This comment has been minimized.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

@alejandro-jimenez-dcl
alejandro-jimenez-dcl enabled auto-merge (squash) June 30, 2026 10:17
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @alejandro-jimenez-dcl's task in 1m 13s —— View job


Code Review

  • Load context (CLAUDE.md, docs/README.md, diff)
  • Root-cause check (Step 2)
  • Design & integration review (Step 3)
  • Member audit (Step 4)
  • Line-level review (Step 5)
  • Complexity & QA assessment (Steps 6–7)
  • Post verdict (Steps 8–9)

Summary

Step 2 — Root-cause check: Two distinct changes in this PR:

  1. DebugMetricsAnalyticsHandler.cs — The original #if ENABLE_PROFILER … #endif return false; structure compiled both return statements when ENABLE_PROFILER was defined, making return false unreachable (CS0162). The fix inserts #else before return false and moves #endif after it, so exactly one return compiles per configuration. Direct fix. PASS.
  2. Package pin — A floating git URL (no SHA) for com.decentraland.livekit-sdk is replaced with an immutable commit pin that rebuilds the Windows FFI binary with symbols retained, directly addressing unsymbolizable crash dumps. Direct fix. PASS.

Step 3 — Design & integration: No new systems, lifecycle owners, subscriptions, or long-lived units introduced by either change. The C# diff is a preprocessor-directive correctness fix on an existing private static property. Package changes are limited to Unity manifest and lockfile. Owner search: N/A. PASS.

Step 4 — Member audit: The only changed member is the existing private profilerEnabled property. Its consumers are:

  • Update() line 119 (inside #if ENABLE_PROFILER guard — correct)
  • TryUpdateAllMetrics() line 184 (unconditional call — correct, the property gates itself)

No new public properties. No single-use-merge issue. N/A.

Step 5 — Line-level review:

Internal consistency — all three SHA references match:

Location Value
manifest.json ref 222d67ccc289337dadb5e8d96ee5b633dfb905b6
packages-lock.json version 222d67ccc289337dadb5e8d96ee5b633dfb905b6
packages-lock.json hash 222d67ccc289337dadb5e8d96ee5b633dfb905b6

No drift. No blocking issues found across any of the three changed files.

Step 8 — Non-blocking warning:

⚠️ PR description still cites 55d3333… while the deployed binary is 222d67c…. Prior automated reviews flagged this mismatch on 2026-06-29 and 2026-06-30. The DEV approval was issued after acknowledgement, so it is not a blocker — but the PR body should be updated to 222d67c… before merge for traceability.


REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Only a preprocessor-directive fix in a debug-metrics handler and a Unity package manifest pin — no ECS systems, runtime logic, or architectural changes.
QA_REQUIRED: YES

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @alejandro-jimenez-dcl's task in 52s —— View job


Security-focused dependency review (re-run on synchronize)

  • Gather context and read changed files
  • Identify all dependency changes
  • Assess provenance, capability, shipping impact
  • Check non-dependency code change in this push
  • Post verdict

What changed in this push

Three files differ from origin/dev:

File Nature Security relevance
Explorer/Packages/manifest.json livekit-sdk pinned to commit dependency change (reviewed below)
Explorer/Packages/packages-lock.json matching lockfile version + hash dependency change (reviewed below)
Explorer/Assets/.../DebugMetricsAnalyticsHandler.cs #endif/#else reorder (CS0162 unreachable-code fix) none — preprocessor cleanup, no dependency/security impact

Dependency change

A floating git URL (default-branch HEAD) is replaced with an immutable 40-char commit pin — a provenance improvement.

- "com.decentraland.livekit-sdk": "https://github.com/decentraland/client-sdk-unity.git",
+ "com.decentraland.livekit-sdk": "https://github.com/decentraland/client-sdk-unity.git#222d67ccc289337dadb5e8d96ee5b633dfb905b6",

Internal consistency — no drift:

Location Value
manifest.json ref 222d67ccc289337dadb5e8d96ee5b633dfb905b6
packages-lock.json version 222d67ccc289337dadb5e8d96ee5b633dfb905b6
packages-lock.json hash 222d67ccc289337dadb5e8d96ee5b633dfb905b6

Transitive deps in the lockfile are unchanged (com.cysharp.unitask, com.nickkhalow.richtypes, io.livekit.unity). No other manifest entries, .asmdef files, committed binaries, or build/editor scripts changed. No workflow/prompt files changed.

Resolved since prior reviews: the PR body now cites the shipped commit 222d67c… (the earlier 55d3333… mismatch is gone). No SHA drift remains between the description, the manifest, and the lockfile.

Dependency change table

Dependency Before After Source Type Scope Risk Confidence
com.decentraland.livekit-sdk floating …client-sdk-unity.git pinned #222d67c…905b6 git, decentraland org (first-party) source pkg incl. native FFI binaries runtime, ships to desktop client LOW→MEDIUM HIGH (diff) / LOW (upstream)

Assessment

A. Provenance — improved. [PR] Floating → immutable SHA on a first-party decentraland org repo. Recommended posture. Caveat: it targets a feature-branch commit (client-sdk-unity PR#70), so dev is tied to pre-merge upstream code until that PR lands.

B. Capability — proportionate, not new. [Inference] client-sdk-unity carries native FFI plugins for LiveKit voice/video transport. Network + native capability is inherent and already shipping; this PR re-points to a build with Windows symbols retained — it does not add capability.

C. Shipping impact. Runtime dependency shipping in the end-user desktop client; rebuild improves Windows crash-dump symbolication.

Items requiring a human glance (MEDIUM)

  1. Pin references a PR-branch head, not a merged/tagged release. Immutable and safe, but pre-merge. Confirm upstream client-sdk-unity PR#70 is merged so dev doesn't ship from an abandoned branch commit, then re-point at the merged SHA. [UNKNOWN]
  2. Native FFI binaries are not reviewable from this PR — they live upstream. Confirm they were produced by trusted CI, not a local machine. [UNKNOWN]
  3. Upstream not verifiable from this sandbox — couldn't independently confirm 222d67c… is reachable or inspect its contents. [UNKNOWN]

Verdict

Net positive (floating → immutable pin, first-party dep, no new capability, no new transitive deps). The PR-body/QA SHA mismatch from earlier reviews is now resolved. Remaining concerns are operational, not hard blockers — they hinge on upstream PR#70 landing and on FFI-binary build provenance. QA already validated livestreams on Windows + Mac with no regressions.

DEPENDENCY_REVIEW: NEEDS_ATTENTION
· Branch: chore/livekit-ffi-symbols-pr70


💰 Review cost: $0.24 · claude-sonnet-4-6 · 6 turns

@github-actions

Copy link
Copy Markdown
Contributor

Warnings not reduced: 48551 => 48585 — remove at least one warning to merge.

@alejandro-jimenez-dcl
alejandro-jimenez-dcl merged commit 5d25adf into dev Jun 30, 2026
19 of 21 checks passed
@alejandro-jimenez-dcl
alejandro-jimenez-dcl deleted the chore/livekit-ffi-symbols-pr70 branch June 30, 2026 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants