feat: use monitor names for go2rtc stream aliases by AJ0070 · Pull Request #4864 · ZoneMinder/zoneminder

AJ0070 · 2026-06-01T15:31:38Z

Summary

Use monitor names for go2rtc stream aliases to make downstream integrations friendlier, while preserving the existing monitor-ID aliases for backward compatibility.

This also fixes the local test/build path so the project configures with BUILD_TEST_SUITE=ON even when Catch2 is not preinstalled, and stabilizes the comms tests so they do not rely on fixed ports or fixed Unix socket paths.

Changes

Register go2rtc streams under both:
- the existing monitor ID alias
- a new monitor name alias
Update live go2rtc playback to request streams by monitor name when available.
Update thumbnail overlay go2rtc playback to use the same shared stream-name logic.
Pass monitor names through console view data attributes needed by the frontend.
Add a shared JS helper for go2rtc stream name generation.
Add JS tests covering monitor-name behavior and fallback to monitor ID.
Add a Catch2 FetchContent fallback in CMake when find_package(Catch2) is not satisfied.
Make comms tests allocate ephemeral TCP/UDP ports and unique Unix socket paths instead of assuming fixed endpoints.

Compatibility

Existing ID-based go2rtc stream names are still registered.
Existing consumers using numeric stream names should continue to work unchanged.
New UI requests prefer monitor-name streams for friendlier Home Assistant and other external integrations.

Notes

The socket-based comms tests required running ctest outside the sandbox, because the sandbox blocks the socket operations those tests exercise.
The build still emits an existing warning in src/zms.cpp about strncpy truncation; this change does not modify that code.

fixes #4622

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR introduces monitor-name-aware go2rtc stream naming across the UI and backend, while improving test reliability by avoiding hardcoded ports and making Catch2 availability more automatic for test builds.

Changes:

Add a shared ZMGo2RTCStreamNames utility and use it to build go2rtc src values from monitor name (fallback to ID).
Extend live thumbnail overlays and MonitorStream to pass monitor name through to go2rtc stream selection.
Stabilize socket-based unit tests by using ephemeral ports / unique Unix socket paths; fetch Catch2 when not installed.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
web/skins/classic/js/skin.js	Passes monitor name into go2rtc thumbnail overlay and builds stream `src` via shared helper.
web/skins/classic/includes/functions.php	Loads the new go2rtc stream name helper in the classic skin.
web/js/go2rtc-stream-names.js	Adds a shared helper for deriving go2rtc stream names from monitor name/ID.
web/js/MonitorStream.js	Switches go2rtc stream `src` construction to use monitor name-aware helper.
web/ajax/console.php	Adds `data-monitor-name` / legacy `go2rtc_name` attributes for UI consumption.
tests/zm_comms.cpp	Replaces hardcoded ports and static Unix socket path with generated ones.
tests/js/go2rtc-stream-names.test.js	Adds basic unit tests for the new stream naming helper.
src/zm_monitor_go2rtc.cpp	Registers go2rtc streams under both monitor ID and (optionally) monitor name.
CMakeLists.txt	Falls back to fetching Catch2 via FetchContent when not found locally.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    const streamName = ZMGo2RTCStreamNames.getGo2RTCStreamName(mid, monitorName, '_CameraDirectPrimary');
+    url.search = new URLSearchParams({src: streamName}).toString();


+int reserveEphemeralPort(int socket_type) {
+  const int sd = ::socket(AF_INET, socket_type, 0);
+  REQUIRE(sd >= 0);
+
+  sockaddr_in addr = {};
+  addr.sin_family = AF_INET;
+  addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+  addr.sin_port = 0;
+
+  const int reuse_addr = 1;
+  REQUIRE(::setsockopt(sd, SOL_SOCKET, SO_REUSEADDR, &reuse_addr, sizeof(reuse_addr)) == 0);
+  REQUIRE(::bind(sd, reinterpret_cast<const sockaddr *>(&addr), sizeof(addr)) == 0);
+
+  socklen_t addr_len = sizeof(addr);
+  REQUIRE(::getsockname(sd, reinterpret_cast<sockaddr *>(&addr), &addr_len) == 0);
+
+  const int port = ntohs(addr.sin_port);
+  REQUIRE(port > 0);
+  REQUIRE(::close(sd) == 0);
+  return port;
+}



 #include "zm_comms.h"
 #include <array>
+#include <cstdio>


AJ0070 · 2026-06-03T07:36:24Z

@connortechnology

connortechnology · 2026-06-14T23:38:47Z

Please address the copilot comments

AJ0070 · 2026-06-15T03:21:15Z

@connortechnology I have addressed copilot reviews

SteveGilvarry · 2026-06-19T12:45:49Z

FYI first thing I thought when I saw this was didn't we just merge a UTF-8 for monitor names PR? Not to take from the work, but I am not sure the benefits are there to enable this. @connortechnology?

Core concern — using the monitor name as a stream identifier

A stream identifier should be stable, unique, and URL-safe. A monitor name is none of those:

Non-ASCII names break it. UriEncode() (zm_utils.cpp:385) formats bytes with snprintf("%%%02X", c) where c is a signed char, so a UTF-8 byte like 0xD0 becomes -48 → %FF (mangled + truncated). The C++ side registers the
alias with that broken encoding, while the JS side (URLSearchParams) encodes correct UTF-8 — so the registered name and the requested name don't match, and the stream can't be found. This hits exactly the names ZM just
committed to supporting (Feat: Allow national Unicode characters in monitor name #4785, the utf8mb4 switch "to support 4-byte unicode"). Even setting the bug aside, a "friendly" downstream id of %D0%B2%D1%85… is less usable than the numeric id, which inverts the goal.
Names aren't unique. Monitors.Name is varchar(64) with no unique constraint (PK is Id). Two monitors named Front Door register the same alias, then one clobbers the other, and since the frontend now prefers the name, both
monitors' live views can resolve to one stream. There's also a cross-collision: a monitor named 5 and a different monitor with Id=5 both produce alias 5.
Names are mutable. Renaming a monitor silently breaks any downstream config keyed on the old name; the numeric id is stable for the life of the monitor.
Registration is best-effort but consumption prefers name. Only the ID PUT is fatal; the name PUTs are non-fatal/logged. But the frontend unconditionally requests by name when one exists so if a name registration fails,
playback breaks even though the ID stream is fine.

On the motivation: the win is friendlier handles for manually-written go2rtc/Home-Assistant config. For automated integrations (the common ZM↔HA path) the name adds nothing, they already pull {Id, Name} from /api/monitors.
So the upside is narrow and lands only where the mutability/uniqueness costs bite hardest.

What I'd suggest

Split the escape_json_string fix and the comms-test changes into their own PR(s).
For the friendliness goal itself: if it's pursued, I'd keep the frontend and any canonical reference on the numeric id, and at most register name aliases as an optional, additive convenience for external use, never have
the UI depend on a non-unique/mutable/possibly-mangled name. And UriEncode needs to handle non-ASCII correctly regardless.

IgorA100 · 2026-06-19T13:52:30Z

was didn't we just merge a UTF-8 for monitor names PR?

Yes. #4785 & a04cd7a

I also think that using the monitor name as a stream identifier is a bad idea.

Copilot AI review requested due to automatic review settings June 1, 2026 15:31

Copilot AI reviewed Jun 1, 2026

View reviewed changes

AJ0070 added 2 commits June 15, 2026 08:44

feat: use monitor names for go2rtc stream aliases

5ee8d92

address review

bd6c6dd

AJ0070 force-pushed the fix/4622 branch from dac905d to bd6c6dd Compare June 15, 2026 03:15

addressed copilot reviews

7366b0e

connortechnology requested a review from SteveGilvarry June 18, 2026 01:12

Merge branch 'master' into fix/4622

4b2d8e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: use monitor names for go2rtc stream aliases#4864

feat: use monitor names for go2rtc stream aliases#4864
AJ0070 wants to merge 4 commits into
ZoneMinder:masterfrom
AJ0070:fix/4622

AJ0070 commented Jun 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AJ0070 commented Jun 3, 2026

Uh oh!

connortechnology commented Jun 14, 2026

Uh oh!

AJ0070 commented Jun 15, 2026

Uh oh!

SteveGilvarry commented Jun 19, 2026

Uh oh!

IgorA100 commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		const streamName = ZMGo2RTCStreamNames.getGo2RTCStreamName(mid, monitorName, '_CameraDirectPrimary');
		url.search = new URLSearchParams({src: streamName}).toString();

Uh oh!

Uh oh!

Conversation

AJ0070 commented Jun 1, 2026

Summary

Changes

Compatibility

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AJ0070 commented Jun 3, 2026

Uh oh!

connortechnology commented Jun 14, 2026

Uh oh!

AJ0070 commented Jun 15, 2026

Uh oh!

SteveGilvarry commented Jun 19, 2026

Uh oh!

IgorA100 commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants