-
Notifications
You must be signed in to change notification settings - Fork 68
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
Feature/645 Test devices authentication #673
Conversation
…nly supports RTSP)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request!
Two requirements:
- Revert the old MJPEG logic back to how it was.
- Explain why show
lastTimeChecked
, or drop it.
If you could make screenshots of the changed web layouts, that would be fantastic.
www/lib/lib.php
Outdated
|
||
//-> '-stimeout' is measured in microseconds | ||
$ffprobe_output = shell_exec( | ||
"LD_LIBRARY_PATH=/usr/lib/bluecherry/ /usr/lib/bluecherry/ffprobe -stimeout 5000000 -hide_banner -show_format -show_streams -print_format json \"".$path."\""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why LD_LIBRARY_PATH=/usr/lib/bluecherry/
? I think it works without.
www/lib/lib.php
Outdated
|
||
//-> '-stimeout' is measured in microseconds | ||
$ffprobe_output = shell_exec( | ||
"LD_LIBRARY_PATH=/usr/lib/bluecherry/ /usr/lib/bluecherry/ffprobe -stimeout 5000000 -hide_banner -show_format -show_streams -print_format json \"".$path."\""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note to our future selves regarding -timeout
option.
I thought we want -timeout
option, which comes from RTSP demuxer, per the docs:
https://ffmpeg.org//ffmpeg-protocols.html#rtsp
But using timeout
instead of stimeout
makes it try to listen for incoming connections in our bundled ffmpeg version (4.2.1).
This option used to be called stimeout
up to a certain release, and our current bundled ffmpeg version is still using that, but in latest ffmpeg releases it is called timeout
and we'll have to change this bit later.
So this bit is ok, just a note to self for when we'll be updating the bundled ffmpeg.
www/lib/lib.php
Outdated
|
||
$rtsp_data = json_decode($ffprobe_output, true); | ||
|
||
$this->info['connection_status']['lastTimeChecked'] = date("d/m/Y h:i a"); //TODO: Make date/time formatting regional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why show time?
I think this time is essentially "now" anyways, so I see no need to show it at all.
Or is lastTimeChecked somehow persisted long-term? I don't think it is.
www/lib/lib.php
Outdated
$path = 'rtsp://'.((empty($this->info['rtsp_username'])) ? '' : $this->info['rtsp_username'].':'.$this->info['rtsp_password'].'@').$this->info['ipAddr'].':'.$this->info['port'].$this->info['rtsp']; | ||
break; | ||
case 'IP-MJPEG': | ||
$path= 'http://'.((empty($this->info['rtsp_username'])) ? '' : $this->info['rtsp_username'].':'.$this->info['rtsp_password'].'@').((empty($this->info['ipAddrMjpeg'])) ? $this->info['ipAddr'] : $this->info['ipAddrMjpeg']).':'.$this->info['portMjpeg'].$this->info['mjpeg_path']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this forms an HTTP URL to run ffprobe on.
This doesn't seem to work with the current bundled ffmpeg:
root@jahleel-dev:~# /usr/lib/bluecherry/ffprobe -hide_banner -show_format -show_streams -print_format json http://autkin.net/ua-war.jpg
{
http://autkin.net/ua-war.jpg: Invalid data found when processing input
}
Although that does seem to work with newer ffmpeg.
So perhaps let's pinch our nose and leave the old code intact for the IP-MJPEG case.
One more thing. Shouldn't be a deal breaker, but. |
Co-authored-by: Andriy Utkin <[email protected]>
…ing, Switched to TCP)
www/lib/lib.php
Outdated
|
||
switch($this->info['protocol']) { | ||
case 'IP-RTSP': | ||
$path = '-rtsp_transport tcp "rtsp://'.((empty($this->info['rtsp_username'])) ? '' : $this->info['rtsp_username'].':'.$this->info['rtsp_password'].'@').$this->info['ipAddr'].':'.$this->info['port'].$this->info['rtsp'].'"'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hand-coded command line argument quoting is unsafe.
escapeshellarg($url)
is needed here.
Below suggestion block is not for immediate application, but to illustrate the point:
$path = '-rtsp_transport tcp "rtsp://'.((empty($this->info['rtsp_username'])) ? '' : $this->info['rtsp_username'].':'.$this->info['rtsp_password'].'@').$this->info['ipAddr'].':'.$this->info['port'].$this->info['rtsp'].'"'; | |
$url = 'rtsp://'.((empty($this->info['rtsp_username'])) ? '' : $this->info['rtsp_username'].':'.$this->info['rtsp_password'].'@').$this->info['ipAddr'].':'.$this->info['port'].$this->info['rtsp']; | |
$rtsp_options = '-rtsp_transport tcp'; // TODO according to device config, see https://github.com/bluecherrydvr/bluecherry-apps/blob/master/lib/lavf_device.cpp#L90 | |
$cmdline = "/usr/lib/bluecherry/ffprobe -stimeout 5000000 -hide_banner -show_format -show_streams -print_format json ' . $rtsp_options . ' ' . escapeshellarg($url); |
Hand-coded URL composing seems to work as intended here. (Would preferred to see some "URL builder" API usage here, but couldn't actually find a good one in PHP in a brief search.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do
- proper escaping for RTSP URL,
- RTSP protocol flags for ffprobe command as here: https://github.com/bluecherrydvr/bluecherry-apps/blob/master/lib/lavf_device.cpp#L90 . The protocol string I expect to be in
$this->info['protocol']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit to fix and let's merge it.
Issue #645 requests that a feature be added to the Web UI that allows the end user to test and preview the connection status the their devices in an effort to make it more obvious when authentication settings and paths are not correct.
This PR contains changes to the
editip
stack (/templates/ajax/editip.php
,/templates/dist/js/editip.js
,/ajax/editip.php
) and the function already contained withinlib.php
titledcheckConnection()
.This function previously incorrectly made a request for the header data of http URLs and assumed a 200 code would mean that the authentication succeeded. This was not the case. These have been substituted for an
ffprobe
command with a timeout of 5 seconds.