From a2cb38dd10e20774b5f3ecf8b8c5d0b07a2051f5 Mon Sep 17 00:00:00 2001 From: Abdellatif Anaflous <62770500+hktitof@users.noreply.github.com> Date: Sun, 4 Oct 2026 18:11:50 +0100 Subject: [PATCH] fix(capture): derive video ids from pipelines and guard empty list (#711) when NEKO_CAPTURE_VIDEO_PIPELINES is set without capture.video.ids the VideoIDs list stayed empty, so capture.Video().IDs() returned [] and the first websocket signal request crashed the whole server with "index out of range [0] with length 0" in signalRequest. now we fill VideoIDs from the configured pipeline keys (sorted by id) whenever no ids are set explicitly, and signalRequest returns a plain error instead of panicking when there is no video stream at all. added tests for the config loader, the capture manager and the signal handler so this path stays covered. --- server/internal/capture/manager_test.go | 48 +++++++ server/internal/config/capture.go | 15 +++ server/internal/config/capture_ids_test.go | 62 ++++++++++ server/internal/websocket/handler/signal.go | 3 + .../internal/websocket/handler/signal_test.go | 117 ++++++++++++++++++ 5 files changed, 245 insertions(+) create mode 100644 server/internal/capture/manager_test.go create mode 100644 server/internal/config/capture_ids_test.go create mode 100644 server/internal/websocket/handler/signal_test.go diff --git a/server/internal/capture/manager_test.go b/server/internal/capture/manager_test.go new file mode 100644 index 00000000..7bdc0be3 --- /dev/null +++ b/server/internal/capture/manager_test.go @@ -0,0 +1,48 @@ +package capture + +import ( + "reflect" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/spf13/viper" + + "github.com/m1k1o/neko/server/internal/config" +) + +// captureConfigFromEnv runs the production config flow (init flags, read env, +// populate values) and returns the resulting capture config. +func captureConfigFromEnv(t *testing.T, pipelines string) *config.Capture { + t.Helper() + + viper.Reset() + viper.SetEnvPrefix("NEKO") + viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) + viper.AutomaticEnv() + + t.Setenv("NEKO_CAPTURE_VIDEO_PIPELINES", pipelines) + + cfg := &config.Capture{} + cmd := &cobra.Command{} + if err := cfg.Init(cmd); err != nil { + t.Fatal(err) + } + cfg.Set() + return cfg +} + +// End to end check for the reported panic: NEKO_CAPTURE_VIDEO_PIPELINES alone +// must leave the capture manager with a usable default video stream. +func TestVideoIDsAvailableWithOnlyPipelinesEnv(t *testing.T) { + const pipeline = "ximagesrc display-name={display} show-pointer=false ! appsink name=appsink" + cfg := captureConfigFromEnv(t, `{"high":"`+pipeline+`","medium":"`+pipeline+`","low":"`+pipeline+`"}`) + + manager := New(nil, cfg) + + got := manager.Video().IDs() + want := []string{"high", "low", "medium"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("capture Video().IDs() = %v, want %v (empty list panicked signalRequest)", got, want) + } +} diff --git a/server/internal/config/capture.go b/server/internal/config/capture.go index 048c3f94..dfa97d04 100644 --- a/server/internal/config/capture.go +++ b/server/internal/config/capture.go @@ -3,6 +3,7 @@ package config import ( "os" "reflect" + "sort" "strings" "github.com/go-viper/mapstructure/v2" @@ -423,6 +424,20 @@ func (s *Capture) Set() { log.Warn().Msg("you are setting both single video pipeline and multiple video pipelines, ignoring single video pipeline") } + // If no video ids were configured explicitly, derive them from the configured + // pipelines so the capture manager always has at least one stream to select. + // Without this, NEKO_CAPTURE_VIDEO_PIPELINES alone left VideoIDs empty and the + // first WebRTC signal request panicked on videos[0]. + if len(s.VideoIDs) == 0 && len(s.VideoPipelines) > 0 { + ids := make([]string, 0, len(s.VideoPipelines)) + for id := range s.VideoPipelines { + ids = append(ids, id) + } + sort.Strings(ids) + s.VideoIDs = ids + log.Warn().Strs("video_ids", ids).Msg("no capture.video.ids configured, using all video pipelines sorted by id") + } + s.VideoShowPointer = viper.GetBool("capture.video.show_pointer") if viper.IsSet("capture.video.show_pointer") { for k, p := range s.VideoPipelines { diff --git a/server/internal/config/capture_ids_test.go b/server/internal/config/capture_ids_test.go new file mode 100644 index 00000000..b4726d52 --- /dev/null +++ b/server/internal/config/capture_ids_test.go @@ -0,0 +1,62 @@ +package config + +import ( + "reflect" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/spf13/viper" +) + +// loadCaptureFromEnvWithIDs mirrors loadCaptureFromEnv but also allows setting +// capture.video.ids directly, so we can check that explicit ids are preserved. +func loadCaptureFromEnvWithIDs(t *testing.T, pipelines string, ids []string) *Capture { + t.Helper() + + viper.Reset() + viper.SetEnvPrefix("NEKO") + viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) + viper.AutomaticEnv() + + t.Setenv("NEKO_CAPTURE_VIDEO_PIPELINES", pipelines) + + c := &Capture{} + cmd := &cobra.Command{} + if err := c.Init(cmd); err != nil { + t.Fatal(err) + } + + if ids != nil { + viper.Set("capture.video.ids", ids) + } + + c.Set() + return c +} + +// Regression test for the panic in signalRequest: when video pipelines are +// configured without an explicit capture.video.ids list, VideoIDs must be +// derived from the pipeline keys so the capture manager always has a default +// stream to hand out. +func TestVideoIDsDerivedFromPipelines(t *testing.T) { + const pipeline = "ximagesrc display-name={display} show-pointer=false ! appsink name=appsink" + + t.Run("multiple pipelines without ids populate sorted ids", func(t *testing.T) { + c := loadCaptureFromEnvWithIDs(t, `{"high":"`+pipeline+`","medium":"`+pipeline+`","low":"`+pipeline+`"}`, nil) + + want := []string{"high", "low", "medium"} + if !reflect.DeepEqual(c.VideoIDs, want) { + t.Fatalf("expected video ids %v derived from pipeline keys, got %v (an empty list makes signalRequest panic)", want, c.VideoIDs) + } + }) + + t.Run("explicit ids are preserved", func(t *testing.T) { + c := loadCaptureFromEnvWithIDs(t, `{"high":"`+pipeline+`","low":"`+pipeline+`"}`, []string{"low", "high"}) + + want := []string{"low", "high"} + if !reflect.DeepEqual(c.VideoIDs, want) { + t.Fatalf("expected explicit video ids %v to be preserved, got %v", want, c.VideoIDs) + } + }) +} diff --git a/server/internal/websocket/handler/signal.go b/server/internal/websocket/handler/signal.go index 9554083c..f5078f19 100644 --- a/server/internal/websocket/handler/signal.go +++ b/server/internal/websocket/handler/signal.go @@ -30,6 +30,9 @@ func (h *MessageHandlerCtx) signalRequest(session types.Session, payload *messag // use default first video, if not provided if video.Selector == nil { videos := h.capture.Video().IDs() + if len(videos) == 0 { + return errors.New("no video streams available") + } video.Selector = &types.StreamSelector{ ID: videos[0], Type: types.StreamSelectorTypeExact, diff --git a/server/internal/websocket/handler/signal_test.go b/server/internal/websocket/handler/signal_test.go new file mode 100644 index 00000000..32166372 --- /dev/null +++ b/server/internal/websocket/handler/signal_test.go @@ -0,0 +1,117 @@ +package handler + +import ( + "testing" + + "github.com/pion/webrtc/v4" + + "github.com/m1k1o/neko/server/pkg/types" + "github.com/m1k1o/neko/server/pkg/types/message" +) + +// The fakes below embed the real interfaces so we only implement the methods +// signalRequest actually calls. +type fakeSession struct { + types.Session + profile types.MemberProfile +} + +func (s *fakeSession) ID() string { return "test-session" } + +func (s *fakeSession) Profile() types.MemberProfile { return s.profile } + +func (s *fakeSession) PrivateModeEnabled() bool { return false } + +func (s *fakeSession) Send(string, any) {} + +type fakeWebRTC struct { + types.WebRTCManager + peer *fakePeer +} + +func (w *fakeWebRTC) ICEServers() []types.ICEServer { return nil } + +func (w *fakeWebRTC) CreatePeer(types.Session) (*webrtc.SessionDescription, types.WebRTCPeer, error) { + w.peer = &fakePeer{} + return &webrtc.SessionDescription{SDP: "fake-offer"}, w.peer, nil +} + +type fakePeer struct { + types.WebRTCPeer + video types.PeerVideoRequest +} + +func (p *fakePeer) SetPaused(bool) error { return nil } + +func (p *fakePeer) SetVideo(v types.PeerVideoRequest) error { + p.video = v + return nil +} + +func (p *fakePeer) Video() types.PeerVideo { return types.PeerVideo{} } + +func (p *fakePeer) SetAudio(types.PeerAudioRequest) error { return nil } + +func (p *fakePeer) Audio() types.PeerAudio { return types.PeerAudio{} } + +type fakeCapture struct { + types.CaptureManager + ids []string +} + +func (c *fakeCapture) Video() types.EncodedStreamSelector { + return &fakeSelector{ids: c.ids} +} + +type fakeSelector struct { + types.EncodedStreamSelector + ids []string +} + +func (s *fakeSelector) IDs() []string { return s.ids } + +func newSignalTestHandler(capture types.CaptureManager) (*MessageHandlerCtx, *fakeWebRTC) { + webrtcManager := &fakeWebRTC{} + h := &MessageHandlerCtx{ + capture: capture, + webrtc: webrtcManager, + } + return h, webrtcManager +} + +func watchSession() *fakeSession { + return &fakeSession{profile: types.MemberProfile{CanWatch: true}} +} + +// On master, an empty video id list made signalRequest index videos[0] and +// panic the whole server. It must return a descriptive error instead. +func TestSignalRequestNoVideoStreams(t *testing.T) { + h, _ := newSignalTestHandler(&fakeCapture{ids: nil}) + + payload := &message.SignalRequest{} + if err := h.signalRequest(watchSession(), payload); err == nil { + t.Fatal("expected an error when no video streams are configured, got nil") + } +} + +// When streams exist and the client sends no selector, the first video id is +// used as the default stream. +func TestSignalRequestDefaultsToFirstVideo(t *testing.T) { + h, webrtcManager := newSignalTestHandler(&fakeCapture{ids: []string{"high", "low", "medium"}}) + + payload := &message.SignalRequest{} + if err := h.signalRequest(watchSession(), payload); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + selector := webrtcManager.peer.video.Selector + if selector == nil { + t.Fatal("expected a default selector to be set") + } + if selector.ID != "high" { + t.Fatalf("expected default video id %q, got %q", "high", selector.ID) + } + if selector.Type != types.StreamSelectorTypeExact { + t.Fatalf("expected exact selector type, got %v", selector.Type) + } +}