Skip to content
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

setting the port for pull messages to user-provided ports #219

Merged
merged 2 commits into from
Jan 11, 2022

Conversation

ThabetSabha
Copy link
Contributor

Hi, first of all, thank you so much for this project, it has really saved me quite a bit of time, anyways first time contributing to an open source project, so if anything should be improved please let me know.

I found an issue when the camera receives requests via port forwarding, eg: requests to external_ip:554, get forwarded to the camera on the actual Onvif port 80, this will cause the camera to respond with port 80 for the address that is used for establishing a subscription, which in turn means no events will be received, below is a trivial workaround that from my experience works.

I wasn't sure how I could add a test case for this, so any pointers would be appreciated.

@agsh
Copy link
Owner

agsh commented Jan 10, 2022

@ThabetSabha Hi! Thanks for noticing this problem.
port property is mandatory for the cam instance, so we can just change this line: https://github.com/agsh/onvif/blob/master/lib/events.js#L253 to

url: this.port,

@RogerHardiman what do you think?

@ThabetSabha
Copy link
Contributor Author

@agsh while it makes sense that a connection won't be established without a port, I didn't find any line of code that enforces the instance to have a port option, so I left it just in case, anyways, I just checked out the _parseUrl function, and I felt that it would be cleaner to just update that one instead, what do you think?

@agsh
Copy link
Owner

agsh commented Jan 11, 2022

@ThabetSabha By default instance will have 80 or 443 port: https://github.com/agsh/onvif/blob/master/lib/cam.js#L84
And yes, _parseUrl - is a good point! I totally forgot about it, my bad. Just try to use preserveAddress: true in the constructor options.

@ThabetSabha
Copy link
Contributor Author

ThabetSabha commented Jan 11, 2022

@agsh oh man, totally missed that haha, mb. as for preserveAddress: true yeah this is what I am doing currently

@agsh agsh merged commit ca99a87 into agsh:master Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants