Skip to content

[AI Chapters] Add playback_start_latency prop to player_chapter_selected event - #5522

Open
sztomek wants to merge 6 commits into
feat/chapter-fingerprint-syncfrom
feat/chapter-fingerprint-delay-analytics
Open

[AI Chapters] Add playback_start_latency prop to player_chapter_selected event#5522
sztomek wants to merge 6 commits into
feat/chapter-fingerprint-syncfrom
feat/chapter-fingerprint-delay-analytics

Conversation

@sztomek

@sztomek sztomek commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Description

This PR integrates the latest version of EventHorizon that contains the most recent analytics changes to track how long did it take to find the right moment of the stream where the chapter actually begins.
It builds upon our previous PR that introduced fingerptint syncing for chapters.

conversation: p1782969913761809-slack-C0BB11SBN12

UPDATE after PR discussion

  • New fingerprint_calculation_time_ms (generated chapters only) — reports how long the episode's fingerprint mapping took to prepare (Preparing → Active), i.e. the alignment cost on our side, not the playback move. Sourced from FingerprintTimingManager.preparationDurationMs.
  • playback_start_latency_ms no longer conflates the two — it now excludes the fingerprint wait, so for generated chapters it measures the same tap→playback-start latency as producer-provided ones.

UPDATE v2

  • player_chapter_fingerprint_calculated is a new event that we report in order to be on par with iOS behavior.

Fixes PCDROID-645 https://linear.app/a8c/issue/PCDROID-645/update-player-chapter-selected-with-playback-start-latency-ms-prop

Testing Instructions

  1. Select a podcast episode with generated chapters
  2. Select Chapters tab on episode details
  3. Select a chapter mid-list
  4. Observe logs

Screenshots or Screencast

SCR-20260720-jxyw

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews
  • I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@sztomek sztomek added this to the 8.16 milestone Jul 3, 2026
@sztomek
sztomek requested a review from a team as a code owner July 3, 2026 10:44
@sztomek
sztomek requested review from geekygecko and removed request for a team July 3, 2026 10:44
@sztomek sztomek added [Type] Feature Adding a new feature. [Area] Analytics Analytics related issues [Area] Chapters Episode chapters labels Jul 3, 2026
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @sztomek's task in 4m 50s —— View job


Code Review

Reviewed against origin/feat/chapter-fingerprint-sync. Nicely structured change — the UNDISPATCHED collector started before skipToChapter (so the resume emission isn't missed), reading playback state twice around the alignment wait, and using the raw DB chapter for re-alignment are all thoughtful. Tests cover the main paths well. I have three substantive concerns and a couple of minor notes; I've left them as inline comments.

Findings

  • 🟠 Analytics under-counting on rapid tapsChaptersViewModel.playChapter. Tracking moved from the start of the method to the end (after awaiting playback). Since playChapterJob?.cancel() aborts the in-flight job, a quick second tap drops the first tap's player_chapter_selected event entirely. This changes event volume, which matters for an analytics PR. (inline comment)

  • 🟠 Unbounded alignment wait can hang playbackawaitStreamAlignedChapter has no timeout; PLAYBACK_START_TIMEOUT only guards resume detection. If the fingerprint map never yields an entry while state stays Preparing/Active (e.g. streaming audio that never matches the reference), the tap never seeks/plays until cancelled by another tap. Consider a withTimeoutOrNull fallback to the unaligned chapter. (inline comment)

  • 🟡 Readiness gate = "map non-empty", not "map covers the chapter"playbackTimeMs()/interpolate() returns non-null (extrapolated) for any non-empty map, so alignment resolves as soon as a single entry exists. On the non-eager streaming path this can commit an extrapolated seek far from the tapped chapter, and the one-shot .first() never corrects it. The unit test can't reproduce it because it mocks a null → value transition that can't occur for a non-empty map. (inline comment)

Minor / non-blocking

  • ChapterManager.awaitStreamAlignedChapter uses fully-qualified au.com.shiftyjelly.pocketcasts.models.to.Chapter in its signature because the file aliases DbChapter as Chapter. Correct, but noisy — a clearer alias (e.g. import ...to.Chapter as TransferChapter) would read better.
  • The different-episode branch where episode == null no longer returns early: it now falls through, plays nothing, waits the full 5s timeout, then still tracks the event with latencyMs = null and podcastUuid = null. Bounded, but a wasted wait and a slightly odd event; worth a quick sanity check that this case is acceptable.
  • Checklist notes the EventHorizon schema PR is still unchecked — make sure the playback_start_latency_ms/source/episode_uuid/podcast_uuid props are reflected there before/with merge.

No security concerns. ChapterDao.findChapter column names and IS usage match the existing DAO style, and the primary key (episode_uuid, chapter_index) guarantees a single row.


· feat/chapter-fingerprint-delay-analytics

@wpmobilebot

wpmobilebot commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Project dependencies changes

list
! Upgraded Dependencies
com.automattic:eventhorizon:pocket-casts-2026-07-20_08-31-45, (changed from pocket-casts-2026-07-14_22-15-37)
tree
 +--- project ':modules:features:account'
 |    \--- project ':modules:features:search'
 |         \--- project ':modules:services:analytics'
-|              +--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37
+|              +--- com.automattic:eventhorizon:pocket-casts-2026-07-20_08-31-45
 |              +--- project ':modules:services:model'
-|              |    +--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37 (*)
+|              |    +--- com.automattic:eventhorizon:pocket-casts-2026-07-20_08-31-45 (*)
 |              |    \--- project ':modules:services:utils'
 |              |         \--- project ':modules:services:payment'
-|              |              \--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37 (*)
+|              |              \--- com.automattic:eventhorizon:pocket-casts-2026-07-20_08-31-45 (*)
 |              \--- project ':modules:services:preferences'
-|                   \--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37 (*)
+|                   \--- com.automattic:eventhorizon:pocket-casts-2026-07-20_08-31-45 (*)
 \--- project ':modules:features:discover'
      \--- project ':modules:features:podcasts'
           \--- project ':modules:features:player'
                \--- project ':modules:features:transcripts'
                     \--- project ':modules:services:sharing'
-                         \--- com.automattic:eventhorizon:pocket-casts-2026-07-14_22-15-37 (*)
+                         \--- com.automattic:eventhorizon:pocket-casts-2026-07-20_08-31-45 (*)

@wpmobilebot wpmobilebot modified the milestones: 8.16, 8.17 Jul 6, 2026
@wpmobilebot

Copy link
Copy Markdown
Collaborator

Version 8.16 has now entered code-freeze, so the milestone of this PR has been updated to 8.17.

@sztomek
sztomek requested a review from geekygecko July 14, 2026 15:06
@sztomek

sztomek commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@geekygecko please take another look as i fixed all the issues

} ?: chapter
} else {
chapter
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can't seem to get this working. I saw the playback_start_latency parameter appear once, but I haven't been able to figure out exactly what's happening.

I'm also not quite sure I understand what this is measuring. Shouldn't we be timing the alignment process itself, rather than how long it takes the client app to move playback to the chapter position?

Screen.Recording.2026-07-15.at.4.29.09.pm.mp4

@leandroalonso

leandroalonso commented Jul 16, 2026

Copy link
Copy Markdown
Member

@sztomek @geekygecko I'm working on bringing this event to iOS and I have a few questions/suggestions.

If I got it right, this will always be tracked, no matter if the chapter is generated or producer-provided, right? That means we're tracking two different things:

  1. Provided chapters: the actual connection latency
  2. Generated chapters: fingerprint + connection latency

The big thing for me is that connection latency is out of our reach. This is a network problem, there isn't much we can do, so what do we really want with this data?

On the other hand, calculating fingerprint is something on our side, that we might want to improve and to verify by checking a property.

My 2 cents here would be: we should do a fingerprint_calculation_time property only for generated chapters, that tracks how long a fingerprint resolution took.

What do y'all think?

@sztomek

sztomek commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@leandroalonso thanks for your thoughts 🙏

we should do a fingerprint_calculation_time property only for generated chapters, that tracks how long a fingerprint resolution took.

sounds good to me, will update this PR + address the comments it received.

@sztomek
sztomek force-pushed the feat/chapter-fingerprint-sync branch from c0d95f5 to f7045fc Compare July 17, 2026 12:02
@sztomek

sztomek commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@leandroalonso raised EventHorizonSchemas PR with the proposed changes: https://github.com/Automattic/EventHorizonSchemas/pull/110

@sztomek
sztomek force-pushed the feat/chapter-fingerprint-delay-analytics branch from cea1a03 to 176cb86 Compare July 17, 2026 12:49
@sztomek
sztomek requested a review from geekygecko July 17, 2026 12:56
@leandroalonso

Copy link
Copy Markdown
Member

@sztomek sorry for hijacking this one again. At the moment, we have a difference between iOS and Android:

  • iOS is reactive: meaning, we only fingerprint when the user taps
  • Android is not, fingerprint is calculated anyway. It doesn't matter if the user taps or not.

This basically means I can't have this property in this event because the fingerprint calculation will still happen.

What do you think about another event for that? This way it can work for both Android and iOS.

@wpmobilebot wpmobilebot modified the milestones: 8.17, 8.18 Jul 21, 2026
@wpmobilebot

Copy link
Copy Markdown
Collaborator

Version 8.17 has now entered code-freeze, so the milestone of this PR has been updated to 8.18.

@sztomek

sztomek commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

@geekygecko may i get a review on this one, please? 🙏

@wpmobilebot wpmobilebot modified the milestones: 8.18, 8.19 Aug 3, 2026
@wpmobilebot

Copy link
Copy Markdown
Collaborator

Version 8.18 has now entered code-freeze, so the milestone of this PR has been updated to 8.19.

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

Labels

[Area] Analytics Analytics related issues [Area] Chapters Episode chapters [Type] Feature Adding a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants