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

Maintain a single png or tcp stream in client #46

Merged
merged 1 commit into from May 28, 2013

Conversation

Projects
None yet
3 participants
@eschnou
Collaborator

eschnou commented May 25, 2013

Here is a first partial solution for issue #43 that is sufficient to solve some my problems. It should not break the current API and issues a warning for deprecation on the createPngStream method.

However it is not a a complete solution, since the PngStream will instantiate its own TcpVideoStream, thus disconnecting any other stream obtained via getTcpVideoStream.

In addition, using any other module which directly manage its own tcpstream (e.g. node-dronestream) will also conflict and break everything.

Feedback/comments welcome.

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge May 26, 2013

Owner

I'm not sure if this should be merged yet given the discussion in #43 (comment) - I'll trust you to handle the merging once you're happy with the solution.

Owner

felixge commented May 26, 2013

I'm not sure if this should be merged yet given the discussion in #43 (comment) - I'll trust you to handle the merging once you're happy with the solution.

@eschnou

This comment has been minimized.

Show comment
Hide comment
@eschnou

eschnou May 26, 2013

Collaborator

@felixge Thanks for your input, I went for the second solution and I think I managed not to break anything of the old API. So, if you use getPngStream and getVideoStream the underlying tcpstream is the same, and the lifecycle is managed by the client (connect/reconnect). If you use the underlying classes then you are on your own and the behavior hasn't changed.

I've fixed the tests and tested it in WebFlight, works great for me. I can have the videostream piped to the browser while processing the png with opencv for face tracking, and recording the video on disk, all at the same time :-)

I'll wait a few days for comments and to do a bit more testing and then merge.

Collaborator

eschnou commented May 26, 2013

@felixge Thanks for your input, I went for the second solution and I think I managed not to break anything of the old API. So, if you use getPngStream and getVideoStream the underlying tcpstream is the same, and the lifecycle is managed by the client (connect/reconnect). If you use the underlying classes then you are on your own and the behavior hasn't changed.

I've fixed the tests and tested it in WebFlight, works great for me. I can have the videostream piped to the browser while processing the png with opencv for face tracking, and recording the video on disk, all at the same time :-)

I'll wait a few days for comments and to do a bit more testing and then merge.

@felixge

This comment has been minimized.

Show comment
Hide comment
@felixge

felixge May 27, 2013

Owner

@eschnou 💖 SGTM - thank you so much.

Owner

felixge commented May 27, 2013

@eschnou 💖 SGTM - thank you so much.

Maintain a single png and tcp stream in client
Introducing two new API calls: getPngStream() and getVideoStream().
Multiple calls to these methods return the same objects. The underlying
TcpVideoStream is the same and its lifecylce (connect/reconnect) is
managed by the client.

eschnou added a commit that referenced this pull request May 28, 2013

Merge pull request #46 from eschnou/refactor_client_getstream
Maintain a single png or tcp stream in client. 
Closes #43

@eschnou eschnou merged commit 8627328 into felixge:master May 28, 2013

1 check passed

default The Travis CI build passed
Details

@eschnou eschnou deleted the eschnou:refactor_client_getstream branch May 28, 2013

@bkw

This comment has been minimized.

Show comment
Hide comment
@bkw

bkw Oct 21, 2013

Collaborator

This line breaks socket disconnect for recent versions of node, since we also remove the internal finish event of the socket.
I'm still experimenting wether we should a) only remove the listeners we set, or b) removeAllListeners on the socket's close event.

Collaborator

bkw commented on lib/video/TcpVideoStream.js in daf486d Oct 21, 2013

This line breaks socket disconnect for recent versions of node, since we also remove the internal finish event of the socket.
I'm still experimenting wether we should a) only remove the listeners we set, or b) removeAllListeners on the socket's close event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment