agsh / onvif

ONVIF node.js implementation
http://agsh.github.io/onvif/
MIT License
692 stars 234 forks source link

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

Closed ThabetSabha closed 2 years ago

ThabetSabha commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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