TheSpaghettiDetective / OctoPrint-Obico

GNU Affero General Public License v3.0
136 stars 42 forks source link

[BUG] FFMPEG bug causes Obico not to respect RFC2396 for stream URLs causing compatible mode to run at 0.1 fps #174

Open puterboy opened 2 years ago

puterboy commented 2 years ago

In Octoprint settings, under "Webcam & Timelapse", if you set stream url to: http://myrpi:8080/?action=stream which is a valid RFC2396 URL, then ffmpeg will fail to recognize the URL and returns: http://myrpi:8080?action=stream: Server returned 400 Bad Request

This is actually an ffmpeg bug See the bug report: https://trac.ffmpeg.org/ticket/8466

Obico doesn't detect this bug and merrily launches ffmpeg with the faulty URL. This causes ffmpeg fails (silently) and then Obico defaults to 0.1fps snapshots.

Suggested fix is either:

  1. Warn users if 'malformed' URL (and document to prevent users like me from wasting hours of time and having to debug the code to figure out what is going on)
  2. Or add a manual fix to stream_url in webcam_stream.py to insert a slash if missing before '?'

Of course, until ffmpeg or Obico code is adjusted, users can avoid the problem by making sure there is a "/" before the '?'. i.e., change http://myrpi:8080?action=stream to http://myrpi:8080/?action=stream

kennethjiang commented 2 years ago

Good catch. Feel free to send a PR for it.

puterboy commented 2 years ago

I'm not really a Python programmer, but I think this should work (feel free to correct my crude "style")

--- webcam_stream.py    2022-06-26 22:23:20.631896668 -0400
+++ webcam_stream.py.new   2022-06-26 22:30:05.722292999 -0400
@@ -1,5 +1,6 @@
 import io
 import re
+from urllib.parse import urlparse
 import os
 import logging
 import subprocess
@@ -192,6 +193,9 @@
         jpg = wait_for_webcamd(webcam_settings)
         (_, img_w, img_h) = get_image_info(jpg)
         stream_url = webcam_full_url(webcam_settings.get("stream", "/webcam/?action=stream"))
+        stream_url_parse = urlparse(stream_url)
+        if stream_url_parse.query and not stream_url_parse.path :
+            stream_url = stream_url_parse._replace(path='/').geturl()
         bitrate = bitrate_for_dim(img_w, img_h)
         fps = 25
         if not self.plugin.is_pro_user():
puterboy commented 2 years ago

Alternatively, you could more broadly (and cleanly) add the patch to the definition of webcam_full_url in webcam_capture.py Something like:

def webcam_full_url(url):
    if not url or not url.strip():
        return None

    full_url = url.strip()
- if not urlparse(full_url).scheme:
+ url_parse=urlparse(full_url)
+ if not url_parse.scheme
        full_url = "http://localhost/" + re.sub(r"^\/", "", full_url)
+ elif url_parse.query and not url_parse.path :
+     full_url = url_parse._replace(path='/').geturl()    

    return full_url
puterboy commented 1 year ago

Any reason this patch (or similar) has not been included in latest updates?

kennethjiang commented 1 year ago

It'll be a lot easier for me if you can submit a PR so that we can discuss the specific code.