Change dependency from protobuf to protobufjs

[justinmchase]

This change primarly consists of changing over to protobufjs, because it is a pure js implementation of protobuf.

Other than swapping dependencies the real logic change is in MumbleMessageMap.js where the differences in the apis surface.

The other changes are largely dealing with the fact that protobufjs returns null instead of undefined for mapped properties, and it doesn't prettify properties such as channel_id into channelId like protobuf did.

[Rantanen]

Just pointing out I saw this last week but was off for the weekend. Will try to get this merged in later today!

[justinmchase]

No problem, thanks!

[Rantanen]

Couple of problems with this.

None of the examples work :)

Error: Illegal value for Message.Field .MumbleProto.Authenticate.password of type string: undefined (not a string)
    at Error (native)
    at null.<anonymous> (/home/wace/projects/node-mumble/node_modules/protobufjs/dist/ProtoBuf.js:2641:118)
    at ProtoBuf.Reflect.FieldPrototype.verifyValue (/home/wace/projects/node-mumble/node_modules/protobufjs/dist/ProtoBuf.js:2721:29)
    at MessagePrototype.set (/home/wace/projects/node-mumble/node_modules/protobufjs/dist/ProtoBuf.js:1799:63)
    at Message (/home/wace/projects/node-mumble/node_modules/protobufjs/dist/ProtoBuf.js:1725:42)
    at Object.module.exports.buildPacket (/home/wace/projects/node-mumble/lib/MumbleMessageMap.js:43:12)
    at EventEmitter.MumbleConnection.sendMessage (/home/wace/projects/node-mumble/lib/MumbleConnection.js:125:24)
    at EventEmitter.MumbleConnection.authenticate (/home/wace/projects/node-mumble/lib/MumbleConnection.js:106:10)
    at EventEmitter.MumbleClient.authenticate (/home/wace/projects/node-mumble/lib/MumbleClient.js:294:28)
    at /home/wace/projects/node-mumble/examples/advanced.js:13:16

This seems to be caused by how protobuf.js 3.x.x handles undefined vs null. 4.0.0 that was released 5 days ago fixes this issue and upgrading to that one fixed connecting.

Another problem is that for some reason the clients now join another channel by default. Normally when using say MUMBLE_URL=host.example.org node examples/sin.js the client will join the root channel and stay there. With the protobufjs the client will join root channel and then enter another channel. I guess this has something to do with how UserState message is formed during handshake.

[justinmchase]

Ok cool, I'll check those out. I wasn't seeing channel changing in my testing so that is interesting. I'll try the samples out.

[justinmchase]

Ok I updated protobufjs and the issue with joining a subchannel, it was a null vs. undefined issue.

I am on the loopback example now however, and while it appears to be looping back the sound is garbled. I'd be curious to find out how it sounds to you? I am encountering the same problem in my electron version, where sending microphone data from the browser to mumble results in garbled sound. I'm digging into that now.

[Rantanen]

If you do a longer test with the loopback, will it remain garbled or is it just the start of the transmission?

The loopback example is a bit problematic and doesn't work very well. It has issues synchronizing the received audio data with the outgoing packets which results in garbled sound for couple of seconds at the start. If the transmission is longer, the jitterbuffer on the receiver should come to the rescue and buffer the audio a bit.

I tried looking at the input/output sync code again but didn't come up with a solution that would remove the problem. I think the solution would have something to do with tweaking the timer interval in MumbleInputStream but can't come up with the fix now. :|

[justinmchase]

Yeah, even after a few seconds it keeps happening.

Im testing actually by connecting to the murmur server once with the standard mumble client and once with the loopback example. I can see that the loopback is transmitting always but no sound is coming out, which is good since nothing is being sent yet. Then I press my push-to-talk button and the standard client begins transmitting. I can see both of the transmit icons are lit up red and the sound goes from the client to the server, back down to the loopback, then back up to the client. The that is coming back out sounds bizarre. I can hear the volume appears to be related to what is being sent, but the sound is not right.

If instead of looping back, I pipe the audio to the headphones via electron/html5 apis I can tell it sounds good. Which tells me something on the outbound pipe is going wrong. I'm trying to debug into what that could be.

But you tried out this branch, on a mac I'm assuming? If you were to try the loopback example and it were to work as is then it would help me narrow things down. That would seem to imply that the issue was in a native windows component.

[Rantanen]

I checked out your branch on a Linux server I'm using for development and launched the loopback client there. Then tested it by connecting to the same murmur server with my Windows desktop.

I can see if I can replicate the issue with the Windows desktop.

[justinmchase]

Out of curiosity, is it going through celt or opus on yours? Its going through opus on mine. Also, the jitterbuffer is only used on incoming streams not outgoing, correct?

[Rantanen]

Worked okay-ish on Windows as well. The extra latency caused some problems in quality but again, once the jitterbuffer in the client caught up, it was okay.

The jitterbuffer is used on incoming streams yes. But it's used in both the node-mumble as well as the Mumble client.

I'm using Opus.

[justinmchase]

Well that's good news about this PR then. Is it ready to be accepted or should I do anything else? I'll keep digging into what the deal with the sound is on my machine. I'll try it on another machine and see how it goes.

[Rantanen]

Yeah. It looks good and seems ready. Just clarifying I understand the changes:

• protobufjs uses the exact protobuf naming with variable_name while the old protobuf module used variableName
• protobufjs uses null for unspecified optional fields while protobuf used undefined for these

These are the main differences, right?

I'll still take a quick look at the iojs issue, but for now I feel like I'm fine merging this even without the iojs compatibility.

[Rantanen]

Merged. The iojs issue was due to node-opus and node-jitterbuffer. Updated nan dependencies there which made the whole project compatible with iojs.

[Rantanen]

Oh. And thanks! :)

[justinmchase]

Cool. And thank you too!