TheSpaghettiDetective / OctoPrint-Obico

GNU Affero General Public License v3.0
138 stars 41 forks source link

[BUG] Automatically downgrades to ffmpeg_from_mjpeg if you don't have webcamd installed or if you don't have sudo access to stop service #225

Closed puterboy closed 4 months ago

puterboy commented 9 months ago

The current code in webcam_stream.py assumes that:

  1. The user has webcamd installed as a service
  2. The use has 'sudo' privileges to shutdown the service

Otherwise, the following 'if' statement will cause the streaming to revert to 'ffmpeg_from_mjpeg' even if you have a valid stream already running.

if sarge.run('sudo service webcamd stop').returncodes[0] != 0:
                self.ffmpeg_from_mjpeg()
                return

I for example use Janus to generate my webcam stream directly from the native RPi camera mp4 stream. Others may want to use their own streaming program. Also, I successfully run both Obico and Octoprint without sudo permissions.

The patch for this is simple:

                 self.ffmpeg_from_mjpeg()
                 return

-            if sarge.run('sudo service webcamd stop').returncodes[0] != 0:
+            if sarge.run('systemctl is-active webcamd').returncodes[0] == 0 and sarge.run('sudo service webcamd stop').returncodes[0] != 0:
                 self.ffmpeg_from_mjpeg()
                 return

Note that there are other places in webcam_stream.py' where the use ofwebcamd` is assumed if the service needs to be started. These should probably be addressed to for other use cases. But it doesn't affect the use case where you have a valid stream already.

I created a simple pull request.

kennethjiang commented 9 months ago

The current code in webcam_stream.py assumes that:

1. The user has `webcamd` installed as a service

2. The use has 'sudo' privileges to shutdown the service

Since the code is if sarge.run('sudo service webcamd stop').returncodes[0] != 0:, it doesn't assume any of these. If falls back to ffmpeg_from_mjpeg if any of these conditions are not met.

puterboy commented 9 months ago

But my point is that it needn't or shouldn't fallback to ffmpeg_from_mjpeg -- at least in my case.

SO I don't want or need to 'fallback' to ffmpeg_from_mjpeg rather I have Janus directly ingest the h264 stream from the RPI4 camera & HW encoder. Then the existing webcam_stream.py code later transcodes that native h264 to mpeg to stream back to octoprint on port 8080. This is amazingly CPU efficient on my RPi4 (the RPi5 is a whole other story that I am trying to tackle). The flow is simply:

(1) [HW-encoded h264]  |-> [Obico Janus streaming]
                       |-> [ffmpeg h264->mpeg transcoder] -> [Octoprint mpeg streaming]

'Falling back' would entail the following extra costly software mpeg-to-h264 transcoding.

(2) [HW-encoded h264] -> [3rd party h264->mjpeg transcoder] |-> [Obico mjpeg->h264 encoder] -> [Obico Janus streaming]
                                                            |-> [Octoprint mpeg streaming]

To summarize not having webcamd or not having sudo access forces flow # 2 when # 1 is applicable and much superior.

kennethjiang commented 9 months ago

Not sure if I'm completely following. feel free to open a PR for this. Please keep in mind that our general direction about streaming is to make it simpler, not more complicated. So if the resulted PR will make it more complicated, you will need a very strong reason (like this will benefit 25+% of the users) to get it merged.

puterboy commented 9 months ago

I opened one already :) Let me know if you don't see it. The diagrams I shared I think should be helpful in understanding the flow.

BTW, my PR just adds an additional to test to the line of code to check first if wbcamd is active because if it's inactive no point in trying to try to shut it down (and then fail) -- indeed, quite the opposite - the fact that webcamd is not active probably means that you have a good native h264 stream.

puterboy commented 4 months ago

This is fixed in new webcam_stream.py that no longer uses sarge or messes with webcamd service.