Issue with multiple png/tcp stream to same drone

[eschnou]

Since the drone appears to accept only one tcp video connection, creating multiple tcpstream (or using both a tcp and png stream) will cause disconnections and timeouts.

[Error: TcpVideoStream timeout after 1000 ms.]

What I propose is to replace client.createPngStream() by client.getPngStream() and client.getTcpStream() and have all these based on a single tcp stream instantiated upon first call to one of these methods.

I'll try to experiment with this in the coming days, unless someone as another idea/solution to my problem.

[felixge]

SGTM - looking forward to reviewing a patch for this.

[yocontra]

Should the stream be created on the first call of getPngStream or on connection?

[felixge]

@Contra I think I'd prefer on "first call" to save network bandwidth when not using the video stream. That being said, the iOS client always fetches the video stream, so I'm not sure if this would be an actual issue. Either approach would be fine with me I think.

[eschnou]

This one is much trickier than I had expected. My pull requests fixes part of the problem but not everything. The key issue is to decide on who is responsible for the tcpstream lifecycle management (connect/reconnect). So far, this is happening outside (e.g. the pngstream creates a new tcpvideostream on error). In addition, a lot of modules are accessing the tcpvideostream directly.

I see two paths forward:

1. Maintain the singleton at the tcpvideostream level (difficult, especially since you could connec to multiple ip/ports).
2. Deprecate/warn against using the stream objects directly and implement the stream lifecycle management inside the client, thus exposing a single tcpstream from the client and taking care of connect/reconnect. So, when you do a getTcpVideoStream() you get a stream that is already connected and will reconnect automatically.

I would go for 2 and can give it a shot. We can discuss this here or on irc.

[eschnou]

In addition, it seems that multiplexing a stream to multiple outputs is tricky in node due to stream management (backpressure etc.). In particular, it seems one should not use pipe in this case but handlers on 'data'. I'm really not familiar with this stream world, so any idea is welcome :-)

[felixge]

@eschnou great analysis of the problem. I agree with favoring solution 2 but it does raise the back pressure problem you mentioned. I think this could be solved by using some for of "tee" stream module that can create copies of a given source stream and makes sure back pressure is handled correctly based on the slowest upstream consumer.

Another solution is to ignore back-pressure and return a stream where pause/resume do nothing. Consumers will then have to "drop" events if they can't process them correctly.

What do you think?

Edit: My second suggestion is essentially similar to what your second comment says, except that it would be enforced by the interface rather than convention.