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
Datachannel fix for sending arrayBuffers #170
Conversation
Tested with node-v0.10.9 and node-v0.12.0
Thanks for updating this! I'll have a look when I get home tonight. |
Np :) I need those changes for rtc-stream which depends on binary buffer.. |
I have the segmentation fault bug too when sending thousands of buffers. I tried to rebuild with these fixes, the seg fault is gone but i've got this error that happen quickly (about 50-200 buffers sent)
Thanks for your great work! |
@ecoulon Could you link to a gist with your test case? |
I forgot to precise : this bug happen when sending large buffers. I can reproduce my bug when i set the example/bridge.js to send 200 big buffers. |
@ecoulon with node-0.12.0 can you try to comment lines 267 and 268 @ src/datachannel.cc, -> // arraybuffer->Neuter(); and rebuild code with "node-gyp build". Is it working? |
Hmm with these two lines commented i have 3 different results (seems to be random) :
|
Thanks, I created gist for sending multiple buffers with different times / sizes. And yes it's failing almost every time.. |
Datachannel max buffer size on node is 66528 bytes and on browser it's 73905 bytes (why?) without splitting input buffers and cause of that splitting some bytes will be lost.. Most of segfaults are coming from webrtc thread. rtc::Buffer() allocates new memory segment each time and copies data from string / arraybuffer to that segment and webrtc thread releases that buffer after it's not needed. So the problem is not in node-webrtc module?.. @modeswitch can you update libwebrtc? node-v0.12.0 node-v0.11.16 |
@@ -41,6 +41,11 @@ function RTCDataChannel(internalDC) { | |||
return this.RTCDataStates[state]; | |||
} | |||
}, | |||
'bufferedAmount': { | |||
get: function getReadyState() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getReadyState is the wrong name for this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was pulled in from another patch, so you can revert the implementation for bufferedAmount. If you update your local copy it will be there.
} | ||
} | ||
} | ||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed instead of commented out. If we need it again, it's in the repo history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will gain better performance by using switch cases.
Can you elaborate on the problem this patch is meant to fix? It's a little difficult given the size of the patch to determine whether it addresses the original issue. |
@@ -7,6 +7,7 @@ language: node_js | |||
node_js: | |||
- "0.10" | |||
- "0.11" | |||
- "0.12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was pulled in from another patch, so you can revert it here.
I'm currently working with another webrtc fork. These changes will fix some of problems. https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection |
@vmolsa Why was this closed? |
No description provided.