Skip to content

Commit d993eef

Browse files
committed
Review the time-domain program: centralize duplication, align refusals
Closing pass over the six landed slices, written by several hands: - gate.go: the sync/playout gate twins shared ~100 copy-pasted lines — message-time extraction, stage naming, timebase refusals, and branch validation now live in one set of gate helpers; both files became thin wrappers over their policies. - pipeline/readiness.go: the readiness countdown (arm/note/claim CAS logic and the ready-event text) was duplicated verbatim between the buffered and direct runners; one shared struct owns it now, each runner keeping only its loop-shape-specific walk. - One bindGateDeps seam replaces the bindPlayoutClock/bindQoSReport pair at all three stage-materialization sites, and one markTimelinePaced helper replaces three hand-rolled assertions. - Refusal drift fixed: rate now shares the timeline-control refusal gate with Pause/Resume — the unpaced refusal names the same three fixes, and rate on a closed task refuses with control.ErrNotRunning where it previously silently succeeded. - Six copy-pasted sleep-trace assertion loops across the slice tests collapsed into one assertClockSleeps helper; no pin weakened. Net -3 lines with ~200 lines of duplication centralized; zero exports added; markdown delta zero. Hot-path audit confirmed every slice held its budget (playout/sync admit windows, one-comparison QoS on-time path, one atomic load for readiness, fully cold flush). Gate: root build/vet/tests/staticcheck/gofmt, race subset, seek suites at count=10, nested modules and all example module tests, doc pins.
1 parent 48cb4de commit d993eef

20 files changed

Lines changed: 246 additions & 249 deletions

branch_compose_build.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -582,8 +582,7 @@ func (b *builder) newBranchComposeStepStageNamed(ctx context.Context, name strin
582582
// Playout gates pace on the task timeline the runtime clone carries
583583
// as its clock; binding here keeps the gate internal to the lowering.
584584
// The QoS reporter rides the same clone so late admits reach Watch.
585-
bindPlayoutClock(transform.stage, b.taskClock())
586-
bindQoSReport(transform.stage, b.runtime.qosReportFunc())
585+
bindGateDeps(transform.stage, b.taskClock(), b.runtime.qosReportFunc())
587586
if name != "" && name != transform.stage.Name() {
588587
return namedStage{name: name, stage: transform.stage}, stream, nil
589588
}

gate.go

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package goav
2+
3+
import (
4+
"strings"
5+
"time"
6+
7+
"github.com/thesyncim/goav/av"
8+
"github.com/thesyncim/goav/errcode"
9+
"github.com/thesyncim/goav/pipeline"
10+
)
11+
12+
// This file holds what the two timed-gate families (sync, playout) share:
13+
// stage naming, the refusal for media without a usable PTS timebase, and the
14+
// pre-mutation validation that a stream carries timebase facts. The families
15+
// keep their own literal fix lists so refusals stay exact.
16+
17+
// gateMessageTime extracts the stream and PTS a timed gate schedules on; ok
18+
// is false for media without a valid PTS timebase (and for events).
19+
func gateMessageTime(msg *pipeline.Message) (av.StreamID, time.Duration, bool) {
20+
switch msg.Kind {
21+
case pipeline.MessagePacket:
22+
if msg.Packet == nil || !msg.Packet.PTS.Base.Valid() {
23+
return "", 0, false
24+
}
25+
pts, ok := msg.Packet.PTS.ToDuration()
26+
return msg.Packet.StreamID, pts, ok
27+
case pipeline.MessageFrame:
28+
if msg.Frame == nil || !msg.Frame.PTS.Base.Valid() {
29+
return "", 0, false
30+
}
31+
pts, ok := msg.Frame.PTS.ToDuration()
32+
return msg.Frame.StreamID, pts, ok
33+
default:
34+
return "", 0, false
35+
}
36+
}
37+
38+
// gateStageName builds the diagnostic node name for a timed gate:
39+
// prefix-<sanitized policy name>, with the prefix doubling as the default name.
40+
func gateStageName(prefix string, name string) string {
41+
name = strings.TrimSpace(name)
42+
if name == "" {
43+
name = prefix
44+
}
45+
replacer := strings.NewReplacer(" ", "-", "/", "-", "\\", "-", "\t", "-", "\n", "-")
46+
return prefix + "-" + replacer.Replace(name)
47+
}
48+
49+
// gateTimebaseError refuses one media message that reached a timed gate
50+
// without a valid PTS timebase; the caller supplies its gate-specific fixes.
51+
func gateTimebaseError(operation string, node string, msg *pipeline.Message, fixes []string) error {
52+
kind := ""
53+
switch {
54+
case msg == nil:
55+
case msg.Packet != nil:
56+
kind = "packet"
57+
case msg.Frame != nil:
58+
kind = "frame"
59+
}
60+
return &BuildError{
61+
Phase: phaseBuild,
62+
Family: errcode.FamilyForCode(runtimeBranchInvalidCode),
63+
Code: runtimeBranchInvalidCode,
64+
Operation: operation,
65+
Node: node,
66+
Reason: "media message has no valid PTS timebase",
67+
fields: errDetails(errDetail("message", firstNonEmpty(kind, "unknown"))),
68+
fixes: buildErrorFixes(fixes),
69+
cause: errUnsupportedBuild,
70+
}
71+
}
72+
73+
// validateGatePolicyForBranch refuses, before graph mutation, a branch whose
74+
// stream declares no timebase facts a timed gate could schedule on; a valid
75+
// TimeBase or a Codec.ClockRate (RTP-style) satisfies it.
76+
func validateGatePolicyForBranch(operation string, branchName string, stream av.Stream, reason string, fixes []string) error {
77+
if stream.TimeBase.Valid() {
78+
return nil
79+
}
80+
if stream.Codec.ClockRate != 0 {
81+
return nil
82+
}
83+
return &BuildError{
84+
Phase: phaseBuild,
85+
Family: errcode.FamilyForCode(runtimeBranchInvalidCode),
86+
Code: runtimeBranchInvalidCode,
87+
Operation: operation,
88+
Node: firstNonEmpty(branchName, string(stream.ID), "branch"),
89+
Reason: reason,
90+
fixes: buildErrorFixes(fixes),
91+
cause: errUnsupportedBuild,
92+
}
93+
}

join_plan.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1462,8 +1462,7 @@ func (p *joinPlan) joinNodeBufferPolicy(policy pipeline.BufferPolicy, work workP
14621462

14631463
// insertJoinArmStage appends a per-arm stage after upstream and returns its ref.
14641464
func insertJoinArmStage(graph pipeline.Graph, rt *runtime, stage pipeline.Stage, upstream string) (string, error) {
1465-
bindPlayoutClock(stage, rt.clock)
1466-
bindQoSReport(stage, rt.qosReportFunc())
1465+
bindGateDeps(stage, rt.clock, rt.qosReportFunc())
14671466
ref, err := graph.AddStage(stage, rt.buffer)
14681467
if err != nil {
14691468
return "", err

pipeline/buffered.go

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -142,11 +142,8 @@ type bufferedRunner struct {
142142
// topo is the immutable producer-side routing snapshot; emit reads it without
143143
// g.mu. nil means closed.
144144
topo atomic.Pointer[bufferedTopo]
145-
// readyRemaining counts the sinks that had not yet received media when Run
146-
// started; when it reaches zero the graph publishes av.EventTaskReady.
147-
// readyFired guards the publish so readiness reports at most once.
148-
readyRemaining atomic.Int64
149-
readyFired atomic.Bool
145+
// ready is the once-per-run av.EventTaskReady countdown (readiness.go).
146+
ready readiness
150147
}
151148

152149
// rebuildTopoLocked publishes a fresh producer-side routing snapshot. Caller must
@@ -1120,9 +1117,7 @@ func (g *bufferedRunner) deliver(ctx context.Context, node *bufferedNode, msg *M
11201117
}
11211118

11221119
// armReadinessLocked snapshots the readiness set at Run start: the active
1123-
// sinks that have not yet seen media. Caller must hold g.mu. With no sinks
1124-
// to wait for there is nothing to report; with sinks all fed already the
1125-
// graph is ready as it starts.
1120+
// sinks that have not yet seen media. Caller must hold g.mu.
11261121
func (g *bufferedRunner) armReadinessLocked() {
11271122
var sinks, unfed int64
11281123
for i := range g.nodes {
@@ -1135,38 +1130,32 @@ func (g *bufferedRunner) armReadinessLocked() {
11351130
unfed++
11361131
}
11371132
}
1138-
g.readyRemaining.Store(unfed)
1139-
if sinks > 0 && unfed == 0 {
1133+
if g.ready.arm(sinks, unfed) {
11401134
g.publishReady()
11411135
}
11421136
}
11431137

1144-
// noteSinkMedia counts a sink out of the readiness set exactly once (first
1145-
// media delivery, or removal before any media). The CAS keeps the countdown
1146-
// single-shot per sink; the last count publishes readiness.
1138+
// noteSinkMedia counts a sink out of the readiness set (first media delivery,
1139+
// or removal before any media); the last count publishes readiness.
11471140
func (g *bufferedRunner) noteSinkMedia(sawMedia *atomic.Bool) {
1148-
if !sawMedia.CompareAndSwap(false, true) {
1149-
return
1150-
}
1151-
if g.readyRemaining.Add(-1) == 0 {
1141+
if g.ready.note(sawMedia) {
11521142
g.publishReady()
11531143
}
11541144
}
11551145

11561146
// publishReady reports av.EventTaskReady on the graph's observer stream, at
11571147
// most once. Cold path: it runs once per graph, never per message.
11581148
func (g *bufferedRunner) publishReady() {
1159-
if !g.readyFired.CompareAndSwap(false, true) {
1149+
if !g.ready.claim() {
11601150
return
11611151
}
1162-
event := av.Event{Type: av.EventTaskReady, Reason: "every sink received its first media message"}
11631152
g.eventsMu.Lock()
11641153
defer g.eventsMu.Unlock()
11651154
if g.eventsClosed {
11661155
return
11671156
}
11681157
select {
1169-
case g.events <- event:
1158+
case g.events <- taskReadyEvent():
11701159
default:
11711160
observeDrop(nil, DropObserver, &g.cold)
11721161
}

pipeline/direct.go

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -151,11 +151,8 @@ type directRunner struct {
151151
eventsClosed bool
152152
cold coldStats
153153
closed bool
154-
// readyRemaining counts the sinks that had not yet received media when Run
155-
// started; when it reaches zero the graph publishes av.EventTaskReady.
156-
// readyFired guards the publish so readiness reports at most once.
157-
readyRemaining atomic.Int64
158-
readyFired atomic.Bool
154+
// ready is the once-per-run av.EventTaskReady countdown (readiness.go).
155+
ready readiness
159156
}
160157

161158
// rebuildTopoLocked publishes a fresh immutable routing snapshot from the current
@@ -768,9 +765,7 @@ func (g *directRunner) deliverTopo(ctx context.Context, dst *directTopoNode, msg
768765
}
769766

770767
// armReadinessRLocked snapshots the readiness set at Run start: the active
771-
// sinks that have not yet seen media. Caller must hold g.mu (read). With no
772-
// sinks to wait for there is nothing to report; with sinks all fed already
773-
// the graph is ready as it starts.
768+
// sinks that have not yet seen media. Caller must hold g.mu (read).
774769
func (g *directRunner) armReadinessRLocked() {
775770
var sinks, unfed int64
776771
for i := range g.nodes {
@@ -783,38 +778,32 @@ func (g *directRunner) armReadinessRLocked() {
783778
unfed++
784779
}
785780
}
786-
g.readyRemaining.Store(unfed)
787-
if sinks > 0 && unfed == 0 {
781+
if g.ready.arm(sinks, unfed) {
788782
g.publishReady()
789783
}
790784
}
791785

792-
// noteSinkMedia counts a sink out of the readiness set exactly once (first
793-
// media delivery, or removal before any media). The CAS keeps the countdown
794-
// single-shot per sink; the last count publishes readiness.
786+
// noteSinkMedia counts a sink out of the readiness set (first media delivery,
787+
// or removal before any media); the last count publishes readiness.
795788
func (g *directRunner) noteSinkMedia(sawMedia *atomic.Bool) {
796-
if !sawMedia.CompareAndSwap(false, true) {
797-
return
798-
}
799-
if g.readyRemaining.Add(-1) == 0 {
789+
if g.ready.note(sawMedia) {
800790
g.publishReady()
801791
}
802792
}
803793

804794
// publishReady reports av.EventTaskReady on the graph's observer stream, at
805795
// most once. Cold path: it runs once per graph, never per message.
806796
func (g *directRunner) publishReady() {
807-
if !g.readyFired.CompareAndSwap(false, true) {
797+
if !g.ready.claim() {
808798
return
809799
}
810-
event := av.Event{Type: av.EventTaskReady, Reason: "every sink received its first media message"}
811800
g.eventsMu.Lock()
812801
defer g.eventsMu.Unlock()
813802
if g.eventsClosed {
814803
return
815804
}
816805
select {
817-
case g.events <- event:
806+
case g.events <- taskReadyEvent():
818807
default:
819808
observeDrop(nil, DropObserver, &g.cold)
820809
}

pipeline/readiness.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package pipeline
2+
3+
import (
4+
"sync/atomic"
5+
6+
"github.com/thesyncim/goav/av"
7+
)
8+
9+
// readiness is the once-per-run countdown behind av.EventTaskReady, shared by
10+
// both runners: Run arms it with the sinks that have not yet seen media, the
11+
// sink delivery path counts each one out on its first packet or frame (Remove
12+
// counts out a sink that leaves unfed), and whoever takes the count to zero
13+
// publishes the event — at most once, guarded by fired.
14+
type readiness struct {
15+
remaining atomic.Int64
16+
fired atomic.Bool
17+
}
18+
19+
// arm snapshots the countdown at Run start and reports whether the graph is
20+
// ready as it starts: sinks exist and every one already saw media. With no
21+
// sinks there is nothing to wait for, so a sink-less graph never reports.
22+
func (r *readiness) arm(sinks, unfed int64) bool {
23+
r.remaining.Store(unfed)
24+
return sinks > 0 && unfed == 0
25+
}
26+
27+
// note counts a sink out exactly once (the CAS keeps the countdown
28+
// single-shot per sink) and reports whether it was the last — the caller
29+
// publishes readiness.
30+
func (r *readiness) note(sawMedia *atomic.Bool) bool {
31+
if !sawMedia.CompareAndSwap(false, true) {
32+
return false
33+
}
34+
return r.remaining.Add(-1) == 0
35+
}
36+
37+
// claim reports whether the caller wins the single readiness publish.
38+
func (r *readiness) claim() bool {
39+
return r.fired.CompareAndSwap(false, true)
40+
}
41+
42+
// taskReadyEvent is the graph-level av.EventTaskReady both runners publish.
43+
func taskReadyEvent() av.Event {
44+
return av.Event{Type: av.EventTaskReady, Reason: "every sink received its first media message"}
45+
}

0 commit comments

Comments
 (0)