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

Binarypack mandatory? #611

Closed
ivelin opened this issue Dec 24, 2019 · 7 comments
Closed

Binarypack mandatory? #611

ivelin opened this issue Dec 24, 2019 · 7 comments
Labels
help wanted looking for contributors investigating currently being looked into

Comments

@ivelin
Copy link
Member

ivelin commented Dec 24, 2019

Wondering if binary pack/unpack is mandatory with the latest browser implementations of WebRTC serialization?

As I am working on the Python client port I noticed that the binary pack dependency would also have to be ported. In the meanwhile it seems like more recent peer libraries like simple-peer rely on the default serialization standard which simplifies cross language portability.

Does it make sense to make binary packing optional?

@afrokick
Copy link
Member

It needs for object serialization. You can simply call dataConnection.send({"score":1}) without any manual converting to blob/arraybuffer.

You can implement only JSON type for serialization for Python port.

@ivelin
Copy link
Member Author

ivelin commented Dec 24, 2019

@afrokick thank you for the suggestions.

Let me make sure I understand:

  1. If I use send(score:1) peerjs will use the default WebRTC serialization which can be decoded without bunarypack port on the Python side?

  2. Does the opposite hold? Can I send binary data from the python side with a parameter that hints to peerjs to skip binarypack?

  3. For JSON serialization. Would that work for json encoding of large binary files such as images or recorded videos? Other than the base64 overhead of json binary encoding , would there be an issue with chunking it into smaller datachannel packets?

Thank you!

@ivelin
Copy link
Member Author

ivelin commented Dec 24, 2019

Looking at the dataconnection code, it appears that either its binary mode and pack/unpack is used or its json mode which expects the whole chunk to be a valid json object. I don't see a scenario that allows large messages using a default webrtc serialization mode:

    if (isBinarySerialization) {
      if (datatype === Blob) {
        // Datatype should never be blob
        util.blobToArrayBuffer(data as Blob, (ab) => {
          const unpackedData = util.unpack(ab);
          this.emit(ConnectionEventType.Data, unpackedData);
        });
        return;
      } else if (datatype === ArrayBuffer) {
        deserializedData = util.unpack(data as ArrayBuffer);
      } else if (datatype === String) {
        // String fallback for binary data for browsers that don't support binary yet
        const ab = util.binaryStringToArrayBuffer(data as string);
        deserializedData = util.unpack(ab);
      }
    } else if (this.serialization === SerializationType.JSON) {
      deserializedData = this.parse(data as string);
    }

@afrokick
Copy link
Member

afrokick commented Dec 24, 2019

@ivelin
1,2) If you want to send/receive "raw" data, you can set dataConnection.serialization = "raw";(instead of 'raw' you can use any string except json, binary, binary-utf8)
3) AFAIK we didn't support chunking for JSON. So you need to use binary serialization to slice it to chunks.

You can use your own implementation of parse/stringify for json serialization
#592

@ivelin
Copy link
Member Author

ivelin commented Dec 28, 2019

1,2) If you want to send/receive "raw" data, you can set dataConnection.serialization = "raw";(instead of 'raw' you can use any string except json, binary, binary-utf8)

Thank you, @afrokick . I get it now. You are saying that any unknown binary serialization type will skip the unpack logic in _handleDataMessage() when DataConnection receives messages and respectively the pack logic in send() when messages are sent out.

@ivelin
Copy link
Member Author

ivelin commented Jan 15, 2020

OK, I am able to exchange raw data messages with peerjs python. Next stop is handling bigger chunks of data. Since PeerJS only chunks in binary format using binarypack, I would have to port all of peerjs binarypack as well.

Question: Is it feasible to switch binarypack with the popular MessagePack library which has support in 50 programming languages?

I tried unpacking binary pack messages on the python end with the msgpack library referenced on the msgpack.org site, but it looks like peerjs binarypack is not sending data in a msgpack compatible format:

msgpack.exceptions.ExtraData: unpack(b) received extra data.

Since peerjs binary pack claims to be 95% msgpack, I wonder if we can make the switch to simplify support for PeerJS in more programming languages.

@jonasgloning
Copy link
Member

jonasgloning commented Aug 30, 2023

Hey @ivelin, I know it took too long, but I just published support for MessagePack and CBOR.

github-actions bot pushed a commit that referenced this issue Sep 3, 2023
# [1.5.0](v1.4.7...v1.5.0) (2023-09-03)

### Bug Fixes

* **datachannel:** sending order is now preserved correctly ([#1038](#1038)) ([0fb6179](0fb6179)), closes [#746](#746)
* **deps:** update dependency @swc/helpers to ^0.4.0 ([a7de8b7](a7de8b7))
* **deps:** update dependency cbor-x to v1.5.4 ([c1f04ec](c1f04ec))
* **deps:** update dependency eventemitter3 to v5 ([caf01c6](caf01c6))
* **deps:** update dependency peerjs-js-binarypack to v1.0.2 ([7452e75](7452e75))
* **deps:** update dependency webrtc-adapter to v8 ([431f00b](431f00b))
* **deps:** update dependency webrtc-adapter to v8.2.2 ([62402fc](62402fc))
* **deps:** update dependency webrtc-adapter to v8.2.3 ([963455e](963455e))
* **MediaConnection:** `close` event is fired on remote Peer ([0836356](0836356)), closes [#636](#636) [#1089](#1089) [#1032](#1032) [#832](#832) [#780](#780) [#653](#653)
* **npm audit:** Updates all dependencies that cause npm audit to issue a warning ([6ef5707](6ef5707))

### Features

* `.type` property on `Error`s emitted from connections ([#1126](#1126)) ([debe7a6](debe7a6))
* `PeerError` from connections ([ad3a0cb](ad3a0cb))
* **DataConnection:** handle close messages and flush option ([6ca38d3](6ca38d3)), closes [#982](#982)
* **MediaChannel:** Add experimental `willCloseOnRemote` event to MediaConnection. ([ed84829](ed84829))
* MsgPack/Cbor serialization ([fcffbf2](fcffbf2))
* MsgPack/Cbor serialization ([#1120](#1120)) ([4367256](4367256)), closes [#611](#611)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted looking for contributors investigating currently being looked into
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants