peerjs

peerjs copied to clipboard

Problem

MediaConnection.close() doesn't propagate the close event to the remote peer.

Solution

The proposed solution uses a similar approach to the DataConnection, where an aux data channel is created for the connection. This way, when we MediaConnection.close() the data channel will be closed and the close signal will be propagated to the remote peer.

Notes

I was not sure if there was another cleaner way of achieving this, without the extra data channel, but this seems to work pretty well (at least until a better solution comes up).

This should fix: https://github.com/peers/peerjs/issues/636

Sorry for getting to this so late. Thanks for the PR, LGTM! It’s on npm with the rc tag now!

@jonasgloning Thanks! Just did some quick testing with the rc build and it still seems to work fine. Hopefully more people will get to confirm the results.

Hi, I can confirm that 1.4.6-rc.2 build works for me too. Thanks for the work!

When is this going live?

@matallui hello, can I help you somehow to go this to production? really need this fix

@asurovenko-zeta Release 1.4.8-rc1 seems to include a fix for this. I haven't really tested it, but give it a try. If that works I think we can drop this PR.

Problem

MediaConnection.close() doesn't propagate the close event to the remote peer.

Solution

The proposed solution uses a similar approach to the DataConnection, where an aux data channel is created for the connection. This way, when we MediaConnection.close() the data channel will be closed and the close signal will be propagated to the remote peer.

Notes

I was not sure if there was another cleaner way of achieving this, without the extra data channel, but this seems to work pretty well (at least until a better solution comes up).

This should fix: #636

I can confirm this to work, but it is not the proper way to solve this IMHO.

It does not merge into current master w/o conflicts, but I merged it manually. As said, it works, but it is a hack.

IMHO the proper solution would be to send a new OFFER on publisher's close, not containing the video channel and accepting the answer. This then should lead to a close event on subscriber side.

Anyway it works. But that this bug is open since so long is quite embarrassing for the maintainer(s) and not a good sign of reliability.

@neilyoung take a look at release 1.4.8-rc.1. They seem to have added a fix for this (different than the changes from this PR). Like I said, I opened this PR because I needed a fix and this was the way I got it working, but I assumed there would be better fixes.

If the fixes from RC don't work, you can always open a PR if you have some thoughts on how to fix this.

~~Hmm. Thanks. At least from the release notes it looks like so. But I can't checkout that tag, somehow. Any hint?~~

Disregard please. Typo in version

Yes it works. And they have adopted your solution.

It remains a hack (no offence)

@neilyoung they have? I thought they did something much simpler https://github.com/peers/peerjs/commit/ec1d5ae8436b5265a59ef277e3fa05608f19f1ae

Am I missing something?

Hmm. Now I'm not sure anymore. Let me checkout master again and then the RC. I had applied your patch manually... I stashed it and checked out the RC... Later checked the sources and found your changes...

That parcel build is strange. Look at this. Error in run 1, ok in run 2:

~/Documents/test/peerjs $ npm run build

> [email protected] build
> rm -rf dist && parcel build


@parcel/transformer-typescript-types: Property 'byteLength' does not exist on type 'Blob'.

  /Users/decades/Documents/test/peerjs/lib/dataconnection.ts:262:25
    261 | 
  > 262 |       if (!chunked && blob.byteLength > util.chunkedMTU) {
  >     |                            ^^^^^^^^^^^ Property 'byteLength' does not exist on type 'Blob'.
    263 |         this._sendChunks(blob);
    264 |         return;


@parcel/transformer-typescript-types: Argument of type 'Blob' is not assignable to parameter of type 'ArrayBuffer'.

  /Users/decades/Documents/test/peerjs/lib/dataconnection.ts:263:22
    262 |       if (!chunked && blob.byteLength > util.chunkedMTU) {
  > 263 |         this._sendChunks(blob);
  >     |                          ^^^^^ Argument of type 'Blob' is not assignable to parameter of type 'ArrayBuffer'.
    264 |         return;
    265 |       }

✨ Built in 2.00s

dist/types.d.ts        10.72 KB     12ms
dist/bundler.cjs      112.43 KB     44ms
dist/bundler.mjs       122.3 KB     41ms
dist/peerjs.min.js    115.04 KB    622ms
dist/peerjs.js        273.74 KB     57ms
~/Documents/test/peerjs $ npm run build

> [email protected] build
> rm -rf dist && parcel build

✨ Built in 129ms

dist/types.d.ts        10.72 KB     12ms
dist/bundler.cjs      112.43 KB     44ms
dist/bundler.mjs       122.3 KB     41ms
dist/peerjs.min.js    115.04 KB    622ms
dist/peerjs.js        273.74 KB     57ms
~/Documents/test/peerjs $ npm run build

Hasn't this been one of your changes? To negoiator.ts? It is in now.

		if (options.originator) {
			if (this.connection.type === ConnectionType.Data) {
				const dataConnection = <DataConnection>(<unknown>this.connection);

				const config: RTCDataChannelInit = { ordered: !!options.reliable };

				const dataChannel = peerConnection.createDataChannel(
					dataConnection.label,
					config,
				);
				dataConnection.initialize(dataChannel);
			} else if (this.connection.type === ConnectionType.Media) {
				const mediaConnection = <MediaConnection>(<unknown>this.connection);
				const dataChannel = peerConnection.createDataChannel(
					mediaConnection.connectionId,
				);
				mediaConnection.initialize(dataChannel);
			}

			this._makeOffer();
		} else {
			this.handleSDP("OFFER", options.sdp);
		}
	}

oh, maybe they just made changes on top of my changes... did't realize that. I agree, I don't think this is the solution, I just didn't know any better. If you have an idea on how to fix this in a better way, go for it. It'll be much appreciated.

I'm not sure if it is worth the efforts. This project seems to be pretty dead. And it doesn't support iterative addition/removal of streams, like lets start with data, then audio and later video and lets switch happily all these things on and off (like mediasoup). I tend to write my own client.

BTW: You did fine. You solved the problem. That's ok.

:tada: This PR is included in version 1.5.0-rc.1 :tada:

The release is available on:

• npm package (@rc dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: