Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add go2rtc package #5889

Merged
merged 19 commits into from
Oct 18, 2023
Merged

Add go2rtc package #5889

merged 19 commits into from
Oct 18, 2023

Conversation

skrashevich
Copy link
Contributor

@skrashevich skrashevich commented Sep 17, 2023

Description

This PR adds the go2rtc package, a camera streaming application with support for various protocols. The package version is 1.7.1. The Makefile, PLIST, and digests for the package are created. The package is also added to the Synology package manager with the necessary configuration files, service setup script, and user interface files for installation and upgrade wizards. The package is configured to run as a background service with a dedicated user group.

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

This commit adds the go2rtc package, a camera streaming application with support for various protocols. The package version is 1.7.1. The Makefile, PLIST, and digests for the package are created. The package is also added to the Synology package manager with the necessary configuration files, service setup script, and user interface files for installation and upgrade wizards. The package is configured to run as a background service with a dedicated user group.
@skrashevich
Copy link
Contributor Author

skrashevich commented Sep 20, 2023

now it doesn't depends on #5890

cross/go2rtc/Makefile Outdated Show resolved Hide resolved
spk/go2rtc/Makefile Outdated Show resolved Hide resolved
skrashevich and others added 2 commits September 20, 2023 15:47
The maintainer of the go2rtc Makefile has been updated from AlexxIT to skrashevich. This change reflects the current responsibility for the maintenance of this file.
cross/go2rtc/digests Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor

hgy59 commented Sep 20, 2023

@skrashevich some more suggestions

  • the package icon must be of quadratic shape (we prefer 512x512 pixel, or at least 256x256)
  • in service-setup.sh the config file must not be overwritten in package ugrade, it must only be created at first installation. Otherwise custom config settings will get lost on upgrade (you find examples in other packages, search for == "INSTALL")
  • this package uses port 8443 for srtp that is missing in the *.sc file. And this port is already in use by Synology CalDAV package. So the config file must define a different port for srtp
  • I propose to add a config.yml file to install with placeholders for user/password from the wizard.
  • I found that it is not possible to restart go2rtc within the web UI. We should document this, or (if it is possible) disable the Save & Restart Button (or modify the button to Save only). For Restart the DSM Package Center must be used.

sample log file:

Wed Sep 20 20:09:51 CEST 2023
Starting go2rtc command /volume1/@appstore/go2rtc/bin/go2rtc -config /volume1/@appstore/go2rtc/var/go2rtc.yaml
20:09:51.706 INF go2rtc version 1.6.2 linux/arm64
20:09:51.707 INF [api] listen addr=0.0.0.0:1984
20:09:51.707 INF [rtsp] listen addr=0.0.0.0:8554
20:09:51.708 INF [webrtc] listen addr=0.0.0.0:8555/tcp
20:09:51.709 INF [srtp] listen addr=0.0.0.0:8443

…-setup.sh to include the new configuration if the package status is INSTALL
@skrashevich
Copy link
Contributor Author

skrashevich commented Sep 24, 2023

  • I found that it is not possible to restart go2rtc within the web UI. We should document this, or (if it is possible) disable the Save & Restart Button (or modify the button to Save only). For Restart the DSM Package Center must be used.

AlexxIT/go2rtc#652 (backported in 4adbe55 )

@skrashevich skrashevich requested a review from hgy59 September 24, 2023 12:55
cross/go2rtc/Makefile Outdated Show resolved Hide resolved
@skrashevich skrashevich requested a review from hgy59 September 27, 2023 20:13
@skrashevich
Copy link
Contributor Author

upd to 1.8.0

spk/go2rtc/Makefile Outdated Show resolved Hide resolved
cross/go2rtc/digests Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2023

@skrashevich please avoid git force-push when ever possible (use only if required, e.g. when rebasing to master).

@skrashevich
Copy link
Contributor Author

@hgy59 Please tell me about the possibility of merging this PR into main branch? I don't want to maintain package without real feedback

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2023

@hgy59 Please tell me about the possibility of merging this PR into main branch? I don't want to maintain package without real feedback

  1. You created a PR ✅
  2. One of the maintainers will approve it to run/enable the github build action
  3. The github build action will create packages (artifacs) that can be manually downloaded and installed

Since there is still pending work in spk/go2rtc/Makefile (go2rtc_extra_install) I did not approve step 2. yet.
If go2rtc supports additionial configuration by environment variables, you should activate this and document how to use it in the environment-variables file, otherwise just remove the code and the file.
Such a environment-variables file should only be used if there is no other solution provided by the application like a config file.

@skrashevich
Copy link
Contributor Author

What changes need to be made? Exactly; not brief ?

@skrashevich
Copy link
Contributor Author

@AlexxIT need your opinion

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2023

@skrashevich I got the current version running on aarch64 with DSM 6.2.4 (DS-218+).

my findings so far:

  • go2rtc does not use variables from environment, so you can remove the environment-variables file and its installation code (leftover from minio package)
  • I highly recommend to install a go2rtc.yaml file by the package and not to create it in service_postinst. This will then need to replace some place holders by wizard variables (like service-setup.sh in spk/monit package)
  • We can omit the upgrade wizard. Currently it only asks for the web user and password again (but does not change anything in the configuration). Since the go2rtc.yaml file can be editet in the web ui, there is no need for such an upgrade wizard.
  • the one thing that is annoying are the color codes (esc sequences) in the log output. Probably it is possible to configure the log format for text only output, but I didn't find any documentation of this configuration parameter.
  • the CHANGELOG is not shown to the user that installs the package. So you really can remove any package related info from it...

If you don't mind, I can update your PR with my probosal.

@skrashevich
Copy link
Contributor Author

I think it will be better if you update PR with your's proposal

- install go2rtc.yaml file
- remove environment-variables file
- remove obsolete variables
  - DSM_UI_DIR: no app icon is created in DSM UI (and app is the default DSM_UI_DIR anyway)
  - SPK_COMMANDS: there is no cli tool installed (go2rtc is used in SERVICE_COMMAND only)
- use sed to inject wizard variables into config file at package installation
- remove upgrade wizard
@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2023

@skrashevich probably there is an error in go2rtc that should be reported upstream:

On the Streams page an empty "Version" is shown, but go2rtc -version displays the correct version Current version: 1.8.0.

go2rtc_streams_view

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2023

@skrashevich is there any reason to configure strp as listen: 0.0.0.0:18443 instead of listen: :18443?

If it must be configured as 0.0.0.0:18443 and :18443 does not work as expected, it would be worth to document this in the yaml file. Otherwise we should omit the 0.0.0.0.

- avoid color codes in log output
- make config file more expressive by adding all listening ports (even the default ones)
- update WebRTC protocol description
- remove obsolete PATCH_LEVEL and include in cross/go2rtc/Makefile
@skrashevich
Copy link
Contributor Author

@skrashevich probably there is an error in go2rtc that should be reported upstream:

On the Streams page an empty "Version" is shown, but go2rtc -version displays the correct version Current version: 1.8.0.

it's known bug in 1.8.0 release, will be fixed soon

@AlexxIT
Copy link
Contributor

AlexxIT commented Oct 15, 2023

@hgy59 I don't know why someone needs ipv6 in their home. Because SRTP used only for HomeKit right now. HomeKit is local thing.

With 0.0.0.0 - it will be ipv4, without - ipv4+ipv6.

@skrashevich
Copy link
Contributor Author

@skrashevich is there any reason to configure strp as listen: 0.0.0.0:18443 instead of listen: :18443?

in this PR in config default value is ":18443". Where did you see 0.0.0.0:18443?

@skrashevich
Copy link
Contributor Author

  • go2rtc does not use variables from environment, so you can remove the environment-variables file and its installation code (leftover from minio package)

go2rtc support templates for using environment variables in any part of config:
https://github.com/AlexxIT/go2rtc/wiki/Configuration#environment-variables

@skrashevich
Copy link
Contributor Author

  • We can omit the upgrade wizard. Currently it only asks for the web user and password again (but does not change anything in the configuration). Since the go2rtc.yaml file can be editet in the web ui, there is no need for such an upgrade wizard.

so, I need just to delete upgrade_uifile.sh ?

@hgy59
Copy link
Contributor

hgy59 commented Oct 15, 2023

so, I need just to delete upgrade_uifile.sh ?

already done in 6cfd3d4

@hgy59
Copy link
Contributor

hgy59 commented Oct 15, 2023

go2rtc support templates for using environment variables in any part of config:
https://github.com/AlexxIT/go2rtc/wiki/Configuration#environment-variables

Thanks for this info.
But there is no need to use environment variables, you can use the config file.
AFAICR, in minio options must be set through environment variables as there is no configuration file.

@hgy59
Copy link
Contributor

hgy59 commented Oct 15, 2023

in this PR in config default value is ":18443". Where did you see 0.0.0.0:18443?

This was your code before I added go2rtc.yaml to the package:

   if [ "${SYNOPKG_PKG_STATUS}" == "INSTALL" ]; then
      echo -e "api:\n  username: ${wizard_root_user}\n  password: ${wizard_root_password}\nsrtp:\n  listen: 0.0.0.0:18443" > ${GO2RTC_CFG_FILE}
   fi

@skrashevich
Copy link
Contributor Author

skrashevich commented Oct 16, 2023

well,
What is required by me now?

@hgy59
Copy link
Contributor

hgy59 commented Oct 16, 2023

Documented the ports used by go2rtc on https://github.com/SynoCommunity/spksrc/wiki/SynoCommunity-Used-Ports

cross/go2rtc/Makefile Outdated Show resolved Hide resolved
spk/go2rtc/Makefile Outdated Show resolved Hide resolved
@hgy59 hgy59 merged commit 71d20d0 into SynoCommunity:master Oct 18, 2023
17 checks passed
@hgy59 hgy59 added the status/published Published and activated (may take up to 48h until visible in DSM package manager) label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants