Fix seek A/V desync, seek latency and segment-boundary media loss - #2093
Fix seek A/V desync, seek latency and segment-boundary media loss#2093larena1 wants to merge 6 commits into
Conversation
Review Details
|
|
Decision: NOT APPROVED Issues:
The findings need attention even though Kodiai could not produce safe automatic patches for them. Review Details
|
|
I looked into the dangling-reference concern on the 1. The container is a 2. The only genuinely concurrent thread — the download worker ( 3. Every structural modifier runs on the read/demux thread itself, or during teardown:
Because the thread executing the loop is itself blocked in it, none of these can pop/reset concurrently with the wait. 4. This mirrors the pre-existing I'm happy to add a short comment on the loop documenting that invariant so this is easier to verify in future reviews. Let me know if you'd prefer that, or if I've missed a path where |
…ment tail read() advanced segment_read_pos_ and absolute_position_ by the number of bytes available, but only copied them out and reported them when they happened to satisfy the full requested amount. Whenever a read straddled the end of a segment buffer - which happens at every segment boundary - the remaining bytes were skipped over without ever being delivered, and the caller was told the read failed. For the TS demuxer that error turns into AVCONTEXT_IO_ERROR, and TSReader::ReadPacket recovers from it by calling Reset(), whose Tell() runs read(0, 0) and therefore ensureSegment(), advancing to the next segment. The parser re-anchors there and everything it had not yet emitted from the current segment is lost - a hole of several seconds in the delivered video, at segment boundaries during plain playback as well as after a seek. Copy and report what is available. ReadPartial is a partial read by contract and AP4_ByteStream::Read loops for the remainder, so a request spanning a segment boundary is now satisfied across both segments instead of failing. Also guard the unsigned subtraction that computes the available count: a read position past the downloaded end would wrap to a huge value and read beyond the buffer. Equality is legitimate - nothing is available yet and the wait below may still deliver data - so only a position strictly beyond it is rejected. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
…with ensureSegment() treats "read position has reached BufferSize()" as "segment fully read". BufferSize() is however the amount downloaded so far, not the size of the segment. When the reader catches up with an ongoing download - at startup, and after every seek, where the download has no head start - the read position reaches that end long before the segment is complete, and the segment is popped with everything that had not yet arrived. Nothing fails visibly at that point: ResetSegment() sets segment_read_pos_ to 0 and leaves absolute_position_ alone for TS, so the absolute position maps cleanly onto the following segment and the demuxer simply reads on there. The result is a hole of several seconds in the delivered media with no read error, no failed seek and no IO error anywhere - which starves Kodi's video player into a stillframe and a decoder reset while audio keeps playing. The existing lock_guard on mutexWorker was aimed at this case but only waits for the mutex, not for data; read() has the correct wait but runs after this decision has already been made. Wait on the same condition variable read() uses until the download delivers more data or leaves the QUEUED/DOWNLOADING state, then re-check before popping. The wait is polled and bounded rather than indefinite: the download thread changes the buffer state and notifies cvRW without holding mutexRW, so a notification issued between the predicate check and the wait is lost and the waiter would never wake, and downloads can also be paused, in which case no notification is coming at all. This was observed on a stream with small, quickly downloaded segments, where the segment reached DOWNLOADED one millisecond after the reader caught up and the demuxer thread blocked - which also made the player impossible to stop. Re-evaluating on a timeout covers both cases, and on expiry the code falls through to the previous behaviour, so this can degrade but never hang. Verified with instrumentation on the affected stream: the condition fires reliably at every download chunk boundary (segment_read_pos_ 1540096, 3080192, ... = multiples of 8192 TS packets), and with the wait in place seven consecutive seeks produced no gap in the packets delivered to Kodi, where every previous build produced one within seconds of the seek and at playback start. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
…stead of wrapping Positions passed to seek() are absolute over the whole stream while only the current segment is buffered. A position before that segment underflowed the unsigned subtraction that maps it into the segment buffer, producing a huge offset that the following clamp turned into "end of the current segment" - and the method then returned false having already moved the read position. TSReader::ReadAV in turn discarded the result of Seek() and read regardless. So the parser was handed valid-looking data from an entirely different point in the timeline while being told the read succeeded, then continued in the *next* segment, silently dropping everything in between. Reject the out-of-range position up front so the failure is explicit and the read position stays untouched, and propagate it in ReadAV so the demuxer sees an IO error instead of wrong data. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
Two problems on MPEG-TS seeks: 1. Latency: the seek scanned forward for a keyframe at/after the target. The MPEG-TS parser reads ahead, so on long-GOP content the next recognised keyframe was at the following segment boundary - the scan downloaded the whole current segment (seconds of latency) and overshot the requested time. 2. A/V desync: because of the same read-ahead the reported PTS did not match the frame actually delivered, so CSession::SeekTime aligned the audio streams to the wrong PTS and audio started ahead of the picture. Stop at the first recovery point (keyframe) instead, which is the one that starts the segment AdaptiveStream::seek_time already selected as containing the requested time. This content carries a single keyframe per segment, at its start (verified on a sample segment: one random_access_indicator, at the first video packet, followed by a 250 frame / 10 second GOP), so the previous scan could not land inside the current segment at all. The scan is still bounded by timeInTs, so a stream that never flags a recovery point degrades to the previous behaviour instead of scanning to EOS. Do not reposition to the recovery position afterwards. The packet we want is already read and CTSSampleReader::TimeSeek hands this m_pkt to Kodi as the first sample, while the packets that follow it are queued in the elementary stream buffers and are drained in order. Seeking the AVContext back to GetRecoveryPos() instead loses them: the parser read-ahead puts that position *behind* the packet just delivered, and reading resumed a full GOP later - the video stream got a hole of seconds right after every seek while audio kept feeding, which starves Kodi's video player into a stillframe and a decoder reset. The reported PTS now matches what is delivered (A/V stays aligned), and no extra segment is downloaded. For scrubber (accurate) seeks Kodi drops the decoded frames up to the requested time, landing exactly; skip seeks land on the segment-start keyframe. Audio-only TS keeps the forward scan (no keyframes to snap to). Assisted-by: Claude Opus 5 <noreply@anthropic.com>
CSegContainer::FindByPTSOrNext treated m_endPts as inclusive, but it is the exclusive end of a segment - for a contiguous timeline it equals the start PTS of the next segment. A pts landing exactly on a boundary therefore matched the segment that *ends* there and the search returned the previous segment. This is hit on every seek: CSession::SeekTime replaces the requested time with the PTS of the video sample actually found and aligns the audio streams to it. Since the video seek lands on a segment-start keyframe, that PTS is exactly a segment boundary, so the audio stream selected the preceding segment and started up to a full segment ahead of the picture. Use the half-open range [startPTS_, m_endPts). A pts at the very end of the last segment still resolves to that segment, so seeking to the stream end is unchanged. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
After a seek the video lands on its segment-start sync sample, but each audio representation was still positioned through its own manifest timing plus the frozen per-stream PTS diff (CSession::SeekTime -> ISampleReader::TimeSeek). Deep into a recording the audio and video timelines drift apart by a fixed offset, so the audio reader emitted a PTS ~1.8s away from the video sample. Kodi's VideoPlayer synchronises on the emitted PTS, not on the addon's internal elapsed time, so the picture started seconds before the sound. This is hit on every skip seek and, identically, on resume-from-position (Kodi issues a seek to the resume point right at startup). Observed on fMP4 (DASH/Smooth) and TS. Audio and video are delivered from the same source PTS clock. Once the video sample actually delivered is known, align the audio reader straight to that reader PTS instead of routing it through the manifest offset: - ISampleReader::TimeSeekReaderPts(pts): seek to an absolute reader PTS (the domain PTS() returns), i.e. without the TimeSeek() m_ptsDiff compensation. Implemented once on the interface by removing that compensation before delegating to TimeSeek(), which every reader already applies. - CSession::SeekTime remembers the video reader PTS and co-times the audio streams that own a segment buffer to it. SeekAdStream has already reset the audio segment to its start, so the seek lands on the target whether or not the reader was running yet - this also covers the resume seek that happens before the first DemuxRead starts the readers. Segment selection is unchanged (it already uses the common elapsed time); only the in-segment reader landing is corrected. fMP4 audio muxed into the video stream (no own segment buffer, hasAdStream == false) keeps its existing behaviour. A LOGINFO/LOGWARNING line reports the residual A/V delta after alignment, so a bad landing (target outside the selected audio segment) is visible in the log rather than only audible. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
e18d2f2 to
4f6ed61
Compare
|
today i dont have time to review nor to test nothing,
|
|
These two logs are what the series started from: a seek on HLS that takes ~5 seconds, and audio that starts seconds before the picture on both HLS and DASH. Both were recorded before the patches. HLS — seek latency and the video hole afterwardsWhat the timestamps say:
Covered by:
DASH — audio lands a full segment ahead of the pictureHere the seek itself is quick — 419 ms, fMP4 has a sync sample table and needs no scan — and the desync is visible as plain arithmetic:
Covered by:
Why they surfaced in this orderThe HLS seek latency masked the rest: once seeking dropped below a second, the segment that
on top of the seek fixes. With all of this in, seeking and general playback are much better. I have tested DASH VoD, live and live timeshift as well as HLS and did not run into regressions — I would have fixed those too. Still, I would appreciate some thorough testing from your side when you find the time. Thanks! |
|
just a suggestion - might be easier for users to test and for reviewers if this was broken up into multiple PRs :) |
|
Fair point, and I did consider it. The reason they're together is that they're not independent fixes — they were all found while chasing the same symptom, and they sit on one causal chain. Split up, none of them is really testable on its own: a build with only one of them still stalls, because the remaining ones produce the same visible symptom. That would mostly generate "doesn't work" reports that don't actually tell you anything. The granularity is still there though — every fix is its own self-contained commit with its own explanation, so it reviews (and reverts, if needed) commit by commit just like separate PRs would. |
4f6ed61 to
a16fa1a
Compare
3 participants
Description
Fixes several defects around seeking and segment-boundary handling. The
seek-landing part is MPEG-TS specific, but the segment-buffering, segment
selection and audio co-timing fixes live in the shared layers and apply to
fMP4 (DASH/Smooth) as well.
Shared layers (affect fMP4/DASH/Smooth and TS):
seek, instead of routing them through the manifest offset
(
ISampleReader::TimeSeekReaderPts).[startPTS_, m_endPts), so a PTS ona boundary resolves to the segment that starts there instead of the previous
one.
spanning a segment boundary is satisfied across both segments.
download to deliver more data (or leave the QUEUED/DOWNLOADING state) before
popping it.
previous behaviour) so it can degrade but never hang the demuxer thread.
MPEG-TS seek landing (TS-specific):
the reported PTS matches what is delivered and no extra segment is downloaded.
Audio-only TS keeps the forward scan (no keyframes to snap to).
forward-only byte stream.
Motivation and context
Seeking produced audible/visible defects:
but each audio representation was positioned through its own manifest timing
plus a frozen per-stream PTS diff. Deep into a recording the timelines drift
apart by a fixed offset, so audio emitted a PTS ~1.8 s from the video and the
picture started seconds before the sound. Hit on every skip seek and on
resume-from-position. Observed on fMP4 (DASH/Smooth) and TS.
CSegContainer::FindByPTSOrNexttreatedm_endPtsas inclusive, so a PTS landing exactly on a segment boundary matchedthe segment that ends there and selected the previous segment; the audio
started up to a full segment ahead. Format-agnostic, hit on every seek.
AdaptiveStreamlayer dropped a segment the reader had only caught up with(treating "downloaded so far" as "fully read"), and discarded the unread tail
of a segment when a read straddled its end. Both punch multi-second holes into
the delivered media with no read error, no failed seek and no IO error –
starving the player into a stillframe/decoder reset while audio keeps playing.
keyframe; the parser read-ahead pushed the next recognised keyframe to the
following segment boundary, downloading the whole current segment, overshooting
the target, and reporting a PTS that did not match the delivered frame.
How has this been tested?
Built for Android and exercised on-device daily for over a week across a variety
of streams, with no regressions observed. On the primary HLS/TS test stream (one
keyframe per ~10 s segment, 188-byte aligned, separate AAC audio): scrub and skip
seeks land on the correct frame, audio and video stay in sync, and there is no
segment-boundary media loss during plain playback or after a seek. The A/V
co-timing path was additionally verified against fMP4 (DASH/Smooth), where the
same desync was reproducible before the change.
Screenshots (if appropriate):
N/A
Types of change
Checklist: