Implement RTC-DataChannel-bufferedAmountLow event

RESOLVED FIXED in Firefox 44

Status

()

defect
P2
normal
Rank:
23
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: shacharz, Assigned: jesup)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments)

Component: Web Apps → WebRTC
Product: Firefox → Core
Assignee: nobody → rjesup
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 25
Component: WebRTC → WebRTC: Networking
Priority: -- → P2
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)
Attachment #8666376 - Flags: review?(drno)
Attachment #8666376 - Flags: review?(bugs)
Duplicate of this bug: 1196145
Rank: 25 → 23
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
Attachment #8666376 - Flags: review?(bugs) → review+
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.
Attachment #8666376 - Flags: review?(drno) → review+
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.
Attachment #8666381 - Flags: review?(drno) → review+
https://hg.mozilla.org/mozilla-central/rev/23cd3f0edf0a
https://hg.mozilla.org/mozilla-central/rev/40e2d33b759e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1050930
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.
You need to log in before you can comment on or make changes to this bug.