Review #103
Review #103
Conversation
|
Thanks, I'll try to take a look over the holidays. |
|
@shbatm: About which holidays are you talking about? 😅 |
There was a problem hiding this comment.
Formatting updates all look good. Sorry it's taken me so long to getting around to review this, life got in the way.
I fully agree with removing OMXPlayer. The config helper was nice to have to simplify the number of questions I was having to answer just for setup--it was written as quick and easy as I could, so jquery was the shortcut, but if you want to take the time or find a better framework go for it.
If you're going to try to keep maintaining, you can update the README to reflect that--it still shows it not being maintained.
I do think long term the WebRTC support is going to be the best option for most people. For my current set-up I'm using an embedded Home Assistant dashboard for most of my Mirror, and really only using a couple simple modules; mostly using the MM backend and frontend combo for automating the mirror as a browser--that being said, I don't have much capacity for testing this anymore--I can review the code, but function tests aren't possible.
| > - I am no longer using this module on my own mirror. After several years, I found that I use the snapshots much more frequently than I streamed the actual cameras, which can be performed by much simpler modules and methods. To enable streaming, WebRTC (like [MMM-HomeAssistant-WebRTC](https://github.com/Anonym-tsk/MMM-HomeAssistant-WebRTC)) is a newer and better standard with much lower server overhead and latency for delivering RTSP Streams to the frontend than any of the options used here, in the future, this will be the method I focus on and I will not try to shoehorn another technology into this module. | ||
| > - Update 5-Oct-2022: See alternative module [MMM-RTSPtoWeb](https://github.com/shbatm/MMM-RTSPtoWeb) for a drastically simplified module relying on WebRTC and a backend server. | ||
| > | ||
| > - I am no longer using this module on my own mirror. After several years, I found that I use the snapshots much more frequently than I streamed the actual cameras, which can be performed by much simpler modules and methods. To enable streaming, WebRTC (like [MMM-HomeAssistant-WebRTC](https://github.com/Anonym-tsk/MMM-HomeAssistant-WebRTC)) is a newer and better standard with much lower server overhead and latency for delivering RTSP Streams to the frontend than any of the options used here, in the future, this will be the method I focus on and I will not try to shoehorn another technology into this module. |
There was a problem hiding this comment.
You may want to update this if you're going to be maintaining further. Also, you can remove the next line about RTSPtoWeb, I'm not going to maintain that repo further.
There was a problem hiding this comment.
You may want to update this if you're going to be maintaining further.
I'm still experimenting with RTSP streams, so the module is still relevant for me. I wouldn't want to take over the module, but I can try to keep it compatible with new versions of MM. If it's okay with you that I might create a PR from time to time? I would therefore leave your comment as it is for now.
Also, you can remove the next line about RTSPtoWeb, I'm not going to maintain that repo further.
Okay, I just removed the reference to MMM-RTSPtoWeb. Would you also like to write a note in the repository that you are no longer maintaining it?
|
Thank you for the detailed answer 🙂
I'll put that and jquery on my long to-do list 🙂
At the moment I would not want to change the status as it is in the README ("I will accept PRs and leave the repo active"). I might check the issues from time to time and make a few small changes, but I don't want to make any promises 🙂 |
I know you don't use the module anymore, but I still find it useful 🙂 I took a look at the code and have some suggestions for changes.
There are no functional changes, just optimizations like formatting, typos and dependency updates.
Notes: