1178091 - Implement RTC-DataChannel-bufferedAmountLow event

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Shachar]

Comply to spec:
https://github.com/w3c/webrtc-pc/pull/233/files

Chrome's equivalent issue:
https://code.google.com/p/chromium/issues/detail?id=496700

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement RTCDataChannel BufferedAmountLowThreshold and bufferedamountlow event

tested locally and works with a modified data_test.html; a second patch is coming to add it to the tests in automation.  Note that bufferedAmount can't currently include data buffered in the stack; Michael Tuexen has that on his todo list (see comments)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: tests for large transfers and onbufferedamountlow event

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4076c821c374

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8666376 [details] [diff] [review]
Implement RTCDataChannel BufferedAmountLowThreshold and bufferedamountlow event

Please don't add any onfoo properties to .idl, only to .webidl.
And do we need attribute unsigned long bufferedAmountLowThreshold; in the .idl either? .webidl is the thing exposed to scripts these days.
(if you end up modifying .idl, update uuid there.)

Those fixed, r+

Didn't review netwerk/sctp/datachannel/DataChannel.cpp

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8666376 [details] [diff] [review]
Implement RTCDataChannel BufferedAmountLowThreshold and bufferedamountlow event

Review of attachment 8666376 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +2640,5 @@
>  DataChannel::GetBufferedAmount()
>  {
> +  size_t buffered = 0;
> +  for (auto& buffer : mBufferedData) {
> +    buffered += buffer->mLength;

With real paranoia this can already overflow.

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8666381 [details] [diff] [review]
tests for large transfers and onbufferedamountlow event

Review of attachment 8666381 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: dom/media/tests/mochitest/dataChannel.js
@@ +169,5 @@
>  ];
>  
> +var commandsCheckLargeXfer = [
> +  function SEND_BIG_BUFFER(test) {
> +    var buffer = new ArrayBuffer(1024*1024);

Would be nice to fill the buffer with some test data rather then just 0's.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23cd3f0edf0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e2d33b759e

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23cd3f0edf0a
https://hg.mozilla.org/mozilla-central/rev/40e2d33b759e

Comment 10

[Eric Shepherd [:sheppy]]

This was already listed on Firefox 44 for developers, and bufferedAmountLowThreshold was documented, along with onbufferedamountlow, but now bufferedamountlow is documented too. This is now complete.