Skip to content

Commit 19017c8

Browse files
pksgitclaude
andauthored
Build each output once instead of rebuilding on every notify::caps (#466)
* Build each output once instead of rebuilding on every notify::caps notify::caps is a GObject property notification, so it can fire more than once per pad. The decoder negotiates at pad-added, when the output does not exist yet and its peer caps query is unconstrained, and again once the output is linked and activation triggers a reconfigure upstream. On macOS with GStreamer 1.28 those two answers differ, so the video track-setup path runs a second time and fails: the new bin carries the same hardcoded name as the first, and GStreamer requires element names to be unique within a bin. Record the output built for each stream kind and consult it on later notifications instead of rebuilding. Caps the established output cannot take are surfaced as ENDPOINT_ERROR; anything else, including a mid-stream resolution change, is logged and tolerated so a live session is not dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Log both sets of caps on renegotiation and release the test bin Review feedback on #466: - The rejection and renegotiation log lines now carry "established caps" alongside "new caps", so the two can be compared without cross-referencing an earlier line. - newTestVideoSinkPad registers a t.Cleanup that sets the bin to NULL, so its elements are released instead of leaking for the duration of the run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop the accept-caps guard and log every renegotiation QueryAcceptCaps is an advisory query, answered by a different vfunc than the one that applies caps, and basetransform's default implementation checks only the head element's own pad templates. It is therefore more permissive than real negotiation, so a false answer only covered caps the bus would reject anyway -- while setting ENDPOINT_ERROR, which stamps EndedAt, without stopping the pipeline. Caps the pipeline genuinely cannot take now fail negotiation downstream and surface on the bus, where messageWatch both reports the error and quits the loop. The output is still built once per stream kind; a later notification only logs both sets of caps and returns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cover the renegotiation guard with unit tests onParamsReady returns from the guard before it touches p.sink or p.Params, so a zero-value Pipeline is a valid fixture: if the guard holds nothing dereferences them, and if it regresses the test panics. No mock or injected interface is needed. A ghost pad's caps come from its own sticky events, so the pad has to negotiate in the data path for the "caps" property to be set. The helper builds videotestsrc ! capsfilter ! bin(fakesink) with the ghost pad on the bin, which is the shape Input surfaces in production. Verified in the CI image (livekit/gstreamer:1.26.7-dev): golangci-lint reports no issues and go test ./pkg/... passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Store the caps event directly and use camelCase log keys Per review. A pad's caps property is its sticky CAPS event, so it can be stored with gst_pad_store_sticky_event rather than produced as a by-product of real negotiation. That drops the videotestsrc pipeline, the state changes and the cleanup, and the pad is now a src pad, matching what Input surfaces. An inactive pad is flushing and rejects the store, hence SetActive. Log keys in the renegotiation warning are camelCase, consistent with resourceID, ingressID and streamKey elsewhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent bc0e3bf commit 19017c8

2 files changed

Lines changed: 108 additions & 1 deletion

File tree

pkg/media/pipeline.go

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package media
1717
import (
1818
"context"
1919
"strings"
20+
"sync"
2021
"sync/atomic"
2122
"time"
2223

@@ -56,6 +57,11 @@ type Pipeline struct {
5657
pipelineErr chan error
5758

5859
eos *eosDispatcher
60+
61+
trackLock sync.Mutex
62+
63+
// The caps each stream kind's output was built from.
64+
established map[types.StreamKind]string
5965
}
6066

6167
func New(ctx context.Context, params *params.Params, g *stats.LocalMediaStatsGatherer) (*Pipeline, error) {
@@ -88,6 +94,7 @@ func New(ctx context.Context, params *params.Params, g *stats.LocalMediaStatsGat
8894
input: input,
8995
pipelineErr: make(chan error, 1),
9096
eos: newEOSDispatcher(),
97+
established: make(map[types.StreamKind]string),
9198
}
9299

93100
input.SetOnEOS(p.eos.Fire)
@@ -137,6 +144,24 @@ func (p *Pipeline) onParamsReady(kind types.StreamKind, gPad *gst.GhostPad) {
137144
return
138145
}
139146

147+
newCaps := caps.(*gst.Caps)
148+
149+
// The audio and video pads notify on separate GStreamer streaming threads,
150+
// so this map is shared mutable state and every access takes the lock.
151+
p.trackLock.Lock()
152+
builtCaps, built := p.established[kind]
153+
p.trackLock.Unlock()
154+
155+
if built {
156+
// Rebuilding is not an option -- it adds a second output of the same name
157+
// and cannot restructure a published track -- so the session continues on
158+
// the output it has. Caps the pipeline genuinely cannot take fail
159+
// negotiation downstream and surface on the bus.
160+
logger.Warnw("caps renegotiated after the output was built, continuing on the existing output", nil,
161+
"kind", kind, "establishedCaps", builtCaps, "newCaps", newCaps.String())
162+
return
163+
}
164+
140165
defer func() {
141166
if err != nil {
142167
p.SetStatus(livekit.IngressState_ENDPOINT_ERROR, err)
@@ -149,7 +174,7 @@ func (p *Pipeline) onParamsReady(kind types.StreamKind, gPad *gst.GhostPad) {
149174
p.SendStateUpdate(context.Background())
150175
}()
151176

152-
bin, err := p.sink.AddTrack(kind, caps.(*gst.Caps))
177+
bin, err := p.sink.AddTrack(kind, newCaps)
153178
if err != nil {
154179
return
155180
}
@@ -159,6 +184,10 @@ func (p *Pipeline) onParamsReady(kind types.StreamKind, gPad *gst.GhostPad) {
159184
return
160185
}
161186

187+
p.trackLock.Lock()
188+
p.established[kind] = newCaps.String()
189+
p.trackLock.Unlock()
190+
162191
gPad.AddProbe(gst.PadProbeTypeBlockDownstream, func(pad *gst.Pad, _ *gst.PadProbeInfo) gst.PadProbeReturn {
163192
// link
164193
if linkReturn := pad.Link(bin.GetStaticPad("sink")); linkReturn != gst.PadLinkOK {

pkg/media/pipeline_test.go

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
// Copyright 2023 LiveKit, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package media
16+
17+
import (
18+
"testing"
19+
20+
"github.com/go-gst/go-gst/gst"
21+
"github.com/stretchr/testify/require"
22+
23+
"github.com/livekit/ingress/pkg/types"
24+
)
25+
26+
const testSystemMemoryCaps = "video/x-raw,format=NV12,width=1280,height=720,framerate=30/1"
27+
28+
// newCapsHoldingGhostPad returns a src ghost pad carrying capsStr, the shape
29+
// Input surfaces. A pad's caps property is its sticky CAPS event, and an
30+
// inactive pad is flushing, so the pad is activated before the event is stored.
31+
func newCapsHoldingGhostPad(t *testing.T, capsStr string) *gst.GhostPad {
32+
t.Helper()
33+
34+
ghost := gst.NewGhostPadNoTarget("video", gst.PadDirectionSource)
35+
require.True(t, ghost.SetActive(true))
36+
require.Equal(t, gst.FlowOK,
37+
ghost.StoreStickyEvent(gst.NewCapsEvent(gst.NewCapsFromString(capsStr))))
38+
require.NotNil(t, ghost.GetCurrentCaps())
39+
40+
return ghost
41+
}
42+
43+
// The renegotiation this fix exists for. On GStreamer 1.28 the video pad is
44+
// advertised as memory:GLMemory and then renegotiated to system memory, so
45+
// onParamsReady runs twice for one pad. The second run must not build another
46+
// output: the bin name is hardcoded, so gst_bin_add would reject it.
47+
//
48+
// sink is deliberately left nil. Building an output dereferences it, so a
49+
// regression here fails loudly instead of silently rebuilding.
50+
func TestSecondCapsNotificationDoesNotRebuild(t *testing.T) {
51+
gst.Init(nil)
52+
53+
const builtCaps = "video/x-raw(memory:GLMemory),format=NV12,width=1280,height=720,texture-target=rectangle"
54+
55+
p := &Pipeline{
56+
established: map[types.StreamKind]string{types.Video: builtCaps},
57+
}
58+
59+
p.onParamsReady(types.Video, newCapsHoldingGhostPad(t, testSystemMemoryCaps))
60+
61+
require.Equal(t, builtCaps, p.established[types.Video],
62+
"the established caps must survive a renegotiation")
63+
require.Len(t, p.established, 1)
64+
}
65+
66+
// A notification carrying no caps is not a renegotiation and must not record
67+
// anything, or the real caps that follow would be treated as the second one.
68+
func TestCapsNotificationWithoutCapsIsIgnored(t *testing.T) {
69+
gst.Init(nil)
70+
71+
p := &Pipeline{
72+
established: make(map[types.StreamKind]string),
73+
}
74+
75+
p.onParamsReady(types.Video, gst.NewGhostPadNoTarget("video", gst.PadDirectionSource))
76+
77+
require.Empty(t, p.established)
78+
}

0 commit comments

Comments
 (0)