-
Notifications
You must be signed in to change notification settings - Fork 15
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
Updates After testing with Sony SNC-CH210 #2
base: master
Are you sure you want to change the base?
Conversation
…t for devices with multiple NIC's
… for some devices (Sony SNC-CH210).
…ented fault (Sony SNC-CH210).
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.
Thank you for the contribution. Just out of curiosity, what are you using ONVIFViewer for?
src/onvifdevice.cpp
Outdated
m_cachedDeviceInformation(new OnvifDeviceInformation()), | ||
m_cachedSnapshotDownloader(new OnvifSnapshotDownloader()) | ||
m_cachedDeviceInformation(new OnvifDeviceInformation(this)), | ||
m_cachedSnapshotDownloader(new OnvifSnapshotDownloader(this)) |
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.
This commit was already merged, right? It would be great if you could rebase, so that this commit is not merged twice.
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.
Snuck in somehow ... I'll have to create a new branch and cherry-pick.
@@ -80,6 +81,8 @@ class OnvifDevice : public QObject | |||
void setPreferredVideoStreamProtocol(const QString& preferredVideoStreamProtocol); | |||
|
|||
void initByUrl(const QUrl& url); | |||
|
|||
QStringList profileNames() const; |
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.
I don't like the interface you are creating here. I think you this should be a list of OnvifMediaProfile
, so that the application can make an educated decision and doesn't have to guess the correct profile based on the name.
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.
Not a bad idea. I'll look into it when I have time. This was a quick addition for a needed feature.
@@ -106,6 +110,7 @@ public slots: | |||
void ptzStop(); | |||
void ptzZoomIn(); | |||
void ptzZoomOut(); | |||
void selectMediaProfile(int index); |
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.
I think the argument should be something like a reference to OnvifMediaProfile
, but I am not sure how this would work. I think the index it too much dependent on the actual implementation.
console.log(modelData.xAddr) | ||
newDevice.hostName = modelData.host; | ||
console.log(modelData.xAddr[0]) | ||
newDevice.hostName = modelData.host[0]; |
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 do you want a list of hostnames if you are just choosing the first?
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.
I'm not using this qml file. This was just a quick change to ensure that it continues to function. The previous implementation just chose the last host. This is something you'll have to rethink on your end. Ultimately I believe a host list should be returned since certain devices report more than one host interface.
@@ -329,6 +329,17 @@ void OnvifDeviceConnectionPrivate::handleSoapError(const KDSoapMessage& fault, c | |||
if (!isHttpDigestSupported && !isUsernameTokenSupported) { | |||
errorString = "None of the authentication methods are available"; | |||
} | |||
} else if (location.contains("OnvifPtzServicePrivate::getServiceCapabilitiesError") && | |||
fault.faultAsString().contains("not implemented")) { |
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.
Is the problem here that getServiceCapabilities is not implemented or that whole PTZ functionality is not implemented. Because I believe that the getServiceCapabilities is a relative new call. So the message could mean that the PTZ is available, but you need to use a different method of determining capabilities.
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.
In this case the device indeed has no PTZ capability but previous calls indicate that the service is available. Perhaps the firmware is shared on multiple devices where PTZ may or may not be present? In any case, I had to add this line in order to access the video from the camera. If not getServiceCapabilities what command is required?
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.
Well, getServiceCapabilities doesn't return anything of my interest currently, so maybe it is not needed at all.
The longer term solution I want to work towards is some kind of feature detection, which will try all the different ONVIF calls and determine which features are supported and which are not supported. I think this is needed, because out of the four ONVIF cameras that I tried, three needed a workaround like this. It appears that the feature reporting of all these cameras are slightly off. But that will need a lot of work, before actually usable ;-)
…required for some devices (Sony SNC-CH210)." This reverts commit 6574a50.
No description provided.