fix(mux): resync-gate drops reach errors(), including after the gap resolves
ResyncGate::dropped is zeroed the moment a keyframe disarms the gate,
and the only EOF warning fires for gates STILL armed. So a mid-title gap
that resolves left no trace anywhere — and most gaps do resolve. A rip
with several concealed gaps reported 0 errors and 0 lost bytes while
whole GOPs had been discarded, which disc.rs's own test comment calls
the ONLY channel through which loss is reported.
This is the other half of e99b634. Arming the gate after an 8 MiB
backstop discard is right — a picture with dangling references must not
ship — but until the drop is counted that trades silent corruption for
silent loss.
The gate now carries dropped_total alongside dropped: per-run answers
"how expensive was this gap", cumulative answers "what did the caller
lose". errors() sums the gates.
Summed in ONE place rather than counted at the three admit call sites.
Three copies of the same increment is how the mux-flush path ends up
counting and the main path not, or the reverse — the duplication shape
this release has been removing. The gate already knows its own total;
the accessor just has to ask.
Found independently by two round-9 lenses, which is what raised it from
plausible to worth acting on.
Both halves are pinned: removing the dropped_total increment reds the
resync test, and removing the sum from errors() reds the DiscStream one.
The second test asserts the ACCESSOR rather than the gate's counter,
because a test on the counter would have passed throughout the entire
period the defect existed.
This commit is contained in:
@@ -1062,7 +1062,20 @@ impl crate::pes::Stream for DiscStream {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn errors(&self) -> u64 {
|
fn errors(&self) -> u64 {
|
||||||
|
// Read errors PLUS every frame the B1 resync gates discarded.
|
||||||
|
//
|
||||||
|
// Summed from the gates here rather than counted at each of the three
|
||||||
|
// `admit` call sites: one place cannot drift, three can, and a frame
|
||||||
|
// dropped by the demuxer flush is the same loss as one dropped on the
|
||||||
|
// main path. `ResyncGate::dropped` is zeroed at every resync, so only
|
||||||
|
// `dropped_total` sees a gap that RESOLVES — which is most of them, and
|
||||||
|
// was the whole reason concealed video loss reached no caller.
|
||||||
self.errors
|
self.errors
|
||||||
|
+ self
|
||||||
|
.resync
|
||||||
|
.iter()
|
||||||
|
.map(super::resync::ResyncGate::dropped_total)
|
||||||
|
.sum::<u64>()
|
||||||
}
|
}
|
||||||
|
|
||||||
fn lost_bytes(&self) -> u64 {
|
fn lost_bytes(&self) -> u64 {
|
||||||
@@ -2112,6 +2125,46 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Frames the B1 resync gate discards must reach `errors()`, including
|
||||||
|
/// after the gap RESOLVES.
|
||||||
|
///
|
||||||
|
/// `ResyncGate::dropped` is zeroed the moment a keyframe disarms the
|
||||||
|
/// gate, and the only EOF warning fires for gates STILL armed — so a
|
||||||
|
/// mid-title gap that resolves left no trace anywhere. That is the
|
||||||
|
/// common case: most gaps do resolve. A rip with several concealed gaps
|
||||||
|
/// reported 0 errors and 0 lost bytes while whole GOPs were discarded.
|
||||||
|
///
|
||||||
|
/// This asserts the accessor, not the gate's own counter, because
|
||||||
|
/// `errors()` is the only channel through which a caller learns
|
||||||
|
/// anything went wrong.
|
||||||
|
#[test]
|
||||||
|
fn errors_reports_frames_the_resync_gate_dropped_after_the_gap_resolves() {
|
||||||
|
let mut s = short_read_stream(true);
|
||||||
|
assert_eq!(PesStream::errors(&s), 0, "clean before anything happens");
|
||||||
|
|
||||||
|
// Drive a gate directly: a discontinuity, two dropped inter-coded
|
||||||
|
// frames, then a keyframe that resyncs and zeroes the per-run count.
|
||||||
|
// The short-read fixture's title carries no streams, so give it a
|
||||||
|
// gate to drive.
|
||||||
|
s.resync.push(crate::mux::resync::ResyncGate::new());
|
||||||
|
let gate = s.resync.last_mut().expect("just pushed");
|
||||||
|
assert!(!gate.admit(true, true, false));
|
||||||
|
assert!(!gate.admit(true, false, false));
|
||||||
|
assert!(gate.admit(true, false, true));
|
||||||
|
assert_eq!(
|
||||||
|
gate.dropped_in_run(),
|
||||||
|
0,
|
||||||
|
"the resync zeroed the per-run counter, which is the whole trap"
|
||||||
|
);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
PesStream::errors(&s),
|
||||||
|
2,
|
||||||
|
"the two discarded frames must still reach the caller after the \
|
||||||
|
gap resolved; reading the per-run counter here would report 0"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
// ── write(): DiscStream is read-only ──────────────────────────────
|
// ── write(): DiscStream is read-only ──────────────────────────────
|
||||||
|
|
||||||
/// `DiscStream` is the tree's only read-only `Stream`. `write()` returning
|
/// `DiscStream` is the tree's only read-only `Stream`. `write()` returning
|
||||||
|
|||||||
+67
-1
@@ -21,8 +21,17 @@
|
|||||||
pub(crate) struct ResyncGate {
|
pub(crate) struct ResyncGate {
|
||||||
/// True while dropping post-gap inter-coded frames until the next keyframe.
|
/// True while dropping post-gap inter-coded frames until the next keyframe.
|
||||||
armed: bool,
|
armed: bool,
|
||||||
/// Count of frames dropped while armed (for a single summary log on resync).
|
/// Count of frames dropped in the CURRENT armed run. Reset at each resync,
|
||||||
|
/// so it answers "how expensive was this gap" and nothing else.
|
||||||
dropped: u64,
|
dropped: u64,
|
||||||
|
/// Every frame this gate has ever dropped, across all runs. NOT reset on
|
||||||
|
/// resync.
|
||||||
|
///
|
||||||
|
/// `dropped` alone cannot report loss: it is zeroed the moment a keyframe
|
||||||
|
/// disarms the gate, so a mid-title gap that resolves before EOF leaves no
|
||||||
|
/// trace at all. That is the common case — most gaps do resolve — which
|
||||||
|
/// made concealed video loss invisible to every consumer.
|
||||||
|
dropped_total: u64,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ResyncGate {
|
impl ResyncGate {
|
||||||
@@ -30,6 +39,7 @@ impl ResyncGate {
|
|||||||
Self {
|
Self {
|
||||||
armed: false,
|
armed: false,
|
||||||
dropped: 0,
|
dropped: 0,
|
||||||
|
dropped_total: 0,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -58,6 +68,7 @@ impl ResyncGate {
|
|||||||
true
|
true
|
||||||
} else {
|
} else {
|
||||||
self.dropped += 1;
|
self.dropped += 1;
|
||||||
|
self.dropped_total += 1;
|
||||||
false
|
false
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
@@ -71,6 +82,12 @@ impl ResyncGate {
|
|||||||
self.dropped
|
self.dropped
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Every frame this gate has dropped, across all armed runs. Survives
|
||||||
|
/// resync, so it is the number a caller reports loss from.
|
||||||
|
pub(crate) fn dropped_total(&self) -> u64 {
|
||||||
|
self.dropped_total
|
||||||
|
}
|
||||||
|
|
||||||
/// Whether the gate is currently dropping frames (armed, awaiting keyframe).
|
/// Whether the gate is currently dropping frames (armed, awaiting keyframe).
|
||||||
pub(crate) fn is_armed(&self) -> bool {
|
pub(crate) fn is_armed(&self) -> bool {
|
||||||
self.armed
|
self.armed
|
||||||
@@ -79,6 +96,55 @@ impl ResyncGate {
|
|||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
|
|
||||||
|
/// `dropped` is per-run and `dropped_total` is cumulative, and only the
|
||||||
|
/// second can report loss.
|
||||||
|
///
|
||||||
|
/// A gap that RESOLVES — the common case — disarms the gate at the next
|
||||||
|
/// keyframe and zeroes `dropped`. Anything reading that counter afterwards
|
||||||
|
/// sees nothing happened, which is how concealed video loss reached no
|
||||||
|
/// caller: the only EOF warning fired for gates STILL armed, i.e. exactly
|
||||||
|
/// the gaps that did not resolve.
|
||||||
|
#[test]
|
||||||
|
fn a_resolved_gap_still_reports_its_dropped_frames() {
|
||||||
|
let mut g = ResyncGate::new();
|
||||||
|
|
||||||
|
// Gap, then two inter-coded frames dropped, then a keyframe resyncs.
|
||||||
|
assert!(
|
||||||
|
!g.admit(true, true, false),
|
||||||
|
"post-gap non-keyframe is dropped"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
!g.admit(true, false, false),
|
||||||
|
"still dropping until a keyframe"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
g.admit(true, false, true),
|
||||||
|
"the keyframe resyncs and is emitted"
|
||||||
|
);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
g.dropped_in_run(),
|
||||||
|
0,
|
||||||
|
"the per-run counter is zeroed by the resync — by design"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
g.dropped_total(),
|
||||||
|
2,
|
||||||
|
"but the frames are still gone, and the cumulative count must say so"
|
||||||
|
);
|
||||||
|
assert!(!g.is_armed(), "resynced");
|
||||||
|
|
||||||
|
// A second gap accumulates rather than restarting.
|
||||||
|
assert!(!g.admit(true, true, false));
|
||||||
|
assert!(g.admit(true, false, true));
|
||||||
|
assert_eq!(
|
||||||
|
g.dropped_total(),
|
||||||
|
3,
|
||||||
|
"totals accumulate across runs; a title with several concealed gaps \
|
||||||
|
must not report only the last one"
|
||||||
|
);
|
||||||
|
}
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
Reference in New Issue
Block a user