Skip to content

Wire viewer-count presence heartbeat/leave into the player - #126

Open
dhinesh-kumar-m wants to merge 1 commit into
mainfrom
feat/presence-sdk-integration
Open

Wire viewer-count presence heartbeat/leave into the player#126
dhinesh-kumar-m wants to merge 1 commit into
mainfrom
feat/presence-sdk-integration

Conversation

@dhinesh-kumar-m

Copy link
Copy Markdown

Summary

  • Adds a presence heartbeat/leave client to the Android player SDK, matching the protocol and behavior already shipped for the web embed player and live-analytics service (/presence/v1/heartbeat, /presence/v1/leave).
  • PresenceHeartbeatManager — starts/stops with playback (onIsPlayingChanged, release()), jittered first heartbeat, server-driven cadence via next_heartbeat_in, Retry-After handling on 429, capped backoff on repeated 401s, best-effort fire-and-forget leave call.
  • PresenceViewerIdStore — SharedPreferences-backed persistent viewer/device ID, generated once and reused across sessions (same ID resent on every heartbeat for device-binding checks server-side).
  • PresenceScheduler — Handler-based timer abstraction, injectable for tests.
  • TPStreamsPlayer.Listener.onPresenceTokenExpired(videoId, callback) — new optional, default no-op listener method (matches the existing onNetworkDiagnosticsStarted() pattern) fired on a 401, so integrators can fetch a fresh token and resolve the callback. Presence is still rollout-gated to 3 orgs, so this is opt-in and won't affect existing integrators who don't implement it.
  • AssetInfo/API layer: viewer_id is appended to the asset-info request when available, and the response's live_stream.presence.{token,base_url} is parsed into a new PresenceConfig and passed to the player.

Test plan

  • New unit tests: PresenceViewerIdStoreTest, PresenceHeartbeatManagerTest (MockWebServer-based, using a RecordingPresenceScheduler for deterministic timer control)
  • Extended ApiServiceTest with viewer_id URL construction and presence-parsing cases
  • Not yet build/runtime-verified in a full app since this is a library-only change; downstream verification happens via the Flutter/RN wrapper SDKs (see linked PRs) once this merges and releases

Related: testpress/iOSPlayerSDK#170, testpress/streams#2507, testpress/flutter-player-sdk#56, testpress/react-native-tpstreams#57, testpress/player.js#42

Kept entirely internal to the player rather than exposed as new public API an
app has to call: TPStreamsPlayer starts the heartbeat loop on
onIsPlayingChanged(true) and stops it (sending a best-effort leave) on
onIsPlayingChanged(false) and release() — the same signal
NetworkDiagnosticsManager and ResumePlaybackManager already key off.

PresenceHeartbeatManager mirrors the web presence-client's and iOS SDK's loop
(jittered first beat, server-driven cadence via next_heartbeat_in,
Retry-After handling) so all three behave identically to the server. JSON
request/response bodies use org.json.JSONObject rather than Gson, matching
how TPStreamsApiService/TestPressApiService already parse everything else in
this codebase. Resends the same persisted device id (PresenceViewerIdStore,
SharedPreferences-backed) on every heartbeat/leave call, matching the
device-binding requirement flagged by senior review. On a 401 it calls back
through TPStreamsPlayer.Listener.onPresenceTokenExpired(videoId, callback)
rather than retrying the same token — mirrors onAccessTokenExpired's exact
shape, but with a default no-op body (this interface already uses that
pattern for onNetworkDiagnosticsStarted): presence is still rollout-gated to
a handful of organizations, and a required override would break every
existing integrator for a feature almost none of them have enabled.

The presence token itself travels through the same asset-info fetch that
already returns playback URLs, nested under live_stream.presence in the API
response, gated behind a new viewer_id query param BaseApiService.assetInfoUrl
now accepts — without it the server mints a fresh anonymous id per request
and the token could never pass device binding at all. TPStreamsApiService
only: TestPressApiService accepts and ignores the parameter, since that
provider has no presence support to bind a device to.

Scheduling and the OkHttpClient are both injected (PresenceScheduler,
OkHttpClient — reusing AssetRepository's existing client-construction style
rather than TPStreamsPlayer's own companion client, to avoid coupling an
unrelated shared resource to this feature) so PresenceHeartbeatManager can be
unit tested against a real MockWebServer, added here as a new test
dependency (this module had none before).

NOTE: this work was initially built in testpress/android-player-sdk, which
turned out to be an archived, deprecated repo — a different package name
(com.tpstream.player, singular) from this one (com.tpstreams.player, plural).
Nothing was pushed there; this is the correct, from-scratch implementation
against the actual active SDK.
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

Claude finished @dhinesh-kumar-m's task in 2m 10s —— View job


Reviewing PR #126 — Wire viewer-count presence heartbeat/leave into the player

Todo list:

  • Gather context (diff, repo structure, CLAUDE.md — none present in this repo)
  • Review PresenceHeartbeatManager (lifecycle, threading, backoff logic)
  • Review PresenceViewerIdStore (SharedPreferences usage)
  • Review PresenceScheduler (Handler abstraction)
  • Review TPStreamsPlayer.kt wiring (listener lifecycle, playback hooks)
  • Review API/model layer changes (AssetInfo, AssetRepository, ApiService)
  • Review new tests
  • Post final review summary with verdict

Summary

Reviewed the full diff (origin/main...HEAD, 15 files, +806/-12): the new presence package (PresenceHeartbeatManager, PresenceScheduler, PresenceViewerIdStore, PresenceConfig), the TPStreamsPlayer.Listener.onPresenceTokenExpired wiring, the viewer_id/presence plumbing through AssetRepository/BaseApiService/TPStreamsApiService/TestPressApiService/AssetInfo, and the accompanying unit tests.

Overall this is a clean, well-isolated addition. It's kept internal to the SDK (internal class), the new listener method is a default no-op so it can't break existing integrators, and the PresenceConfig/AssetInfo parsing fails safe (a malformed presence block just yields null, never a half-populated config or a parse exception). Test coverage of the heartbeat state machine (backoff, 429/Retry-After, idempotent start/stop, leave-on-stop) is thorough and uses a real MockWebServer, which is good practice for this kind of protocol code.

Risks found:

  1. onPresenceTokenExpired is invoked off the main thread (PresenceHeartbeatManager.kt:142TPStreamsPlayer.kt:1015-1017). The 401 path runs from inside the OkHttp Callback.onResponse, which executes on OkHttp's dispatcher thread — not the app/main thread. Every other listener callback in this SDK (onError, onNetworkError, onAccessTokenExpired via getNewToken()) is dispatched from the main thread/playerScope (Dispatchers.Main), so integrators can reasonably assume all TPStreamsPlayer.Listener callbacks arrive on the main thread and touch UI or call back into the player directly. An implementation of onPresenceTokenExpired that does that (e.g. updates a view, or synchronously calls back into TPStreamsPlayer) risks a CalledFromWrongThreadException or a race, and this is easy for integrators to miss since the interface doesn't document the thread it runs on. Suggest dispatching the callback (and the confirmation call from applyRefreshedToken) via playerScope.launch(Dispatchers.Main) the same way onAccessTokenExpired is handled, and documenting the threading guarantee on the interface.

  2. Presence loop churns on every buffering blip, not just real pause/resume (TPStreamsPlayer.kt:426-435). startPresenceHeartbeat()/stopPresenceHeartbeat() are wired to onIsPlayingChanged, which ExoPlayer also fires false→true across buffering, seeking, and ad-like stalls — not just explicit user pause. Each transition sends a leave beacon and, on resume, restarts with a fresh jittered first heartbeat (PresenceHeartbeatManager.kt:53) rather than resuming the existing cadence. On a flaky connection or during quality/track switches this could mean noticeably more leave/heartbeat traffic than intended, and if the server treats leave as ending the viewer session immediately, brief rebuffers could undercount concurrent viewers. Worth confirming this matches the web/iOS SDKs' behavior intentionally (the code comments assert protocol parity with those, but not this specific lifecycle trigger) — if not, hooking into playback-state (STATE_BUFFERING vs. real pause) rather than isPlaying alone would avoid the churn.

Both are non-blocking for a rollout gated to 3 orgs with an opt-in, default-no-op listener, but worth addressing before wider rollout since they affect data accuracy (point 2) and integration safety (point 1).

No issues found with: SharedPreferences-backed PresenceViewerIdStore (correctly scoped to applicationContext, simple get-or-create), the Handler-based PresenceScheduler abstraction, @Synchronized/@Volatile usage inside PresenceHeartbeatManager (start/stop/scheduleNextBeat are correctly guarded, and post-stop network responses are safely no-oped via the active check), the fail-safe parsePresence parsing, or the viewer_id URL-building in TPStreamsApiService/TestPressApiService (correctly ignored by the latter, matching its lack of presence support).

Regression assessment: No changes to core playback, DRM, offline download, or track-selection paths. The blast radius is limited to a new opt-in listener method and additive fields on AssetInfo/API URLs; existing integrators who don't implement onPresenceTokenExpired see no behavior change. Low regression risk to existing functionality.

Final Verdict

⚠️ APPROVED WITH SUGGESTIONS

No blocking correctness or lifecycle issues. The two points above are worth addressing (especially the threading contract on onPresenceTokenExpired, since it's a public API surface that's hard to change once integrators build against it) but don't need to hold up this PR given the rollout is gated and opt-in.

@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedcom.squareup.okhttp3/​mockwebserver@​4.12.03810090100100

View full report

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