New issue
Advanced search Search tips

Issue 878682 link

Starred by 8 users

Issue metadata

Status: Available
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug


Participants' hotlists:
WebRTC-1.0-Spec-Compliance


Sign in to add a comment

RTCDataChannel doesn't fire bufferedamountlow event

Reported by erik.d.h...@gmail.com, Aug 29

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce the problem:
1. Open the attached sample and display the JavaScript console in the browser
2. Click 'Generate and send data'
3. The progress bar is not advancing and the output in the console shows that no bufferedamountlow event is ever triggered

What is the expected behavior?
bufferedamountlow event show be triggered after each send() call

What went wrong?
No bufferedamountlow event event is triggered.

Did this work before? Yes 67

Does this work in other browsers? No
 bufferedamountlow event is not triggered in current Firefox stable release as well.

Chrome version: 68.0.3440.106  Channel: stable
OS Version: 10.13.6
Flash Version: 

You can use the attached sample and tweak the function sendGeneratedData() to test around with different values of bufferedAmountLowThreshold or the modify the chunkSize.

This sample is a slightly modified version of the one available at https://webrtc.github.io/samples/
 
datatransfer.zip
5.4 KB Download
Owner: guidou@chromium.org
Status: Assigned (was: Unconfirmed)
I guess this test is the closes to testing it?

https://wpt.fyi/results/webrtc/RTCDataChannel-bufferedAmount.html
I've checked the old simplewebrtc filetransfer demo from https://github.com/andyet/SimpleWebRTC/blob/gh-pages/filetransfer.html
which use the bufferedAmountLow mechanism and it still works in both Chrome and Firefox.

I suspect this is about the semantics of the bufferedAmountLowThreshold which I found weird at some point.
"When the bufferedAmount decreases from above this threshold to equal or below it, the bufferedamountlow event fires"
-- i suspect the modified sample never gets above the buffered amount so never crossing the threshold?
The SimpleWebRTC sample uses setTimeout(). I can't get it to trigger a bufferedamountlow event either.

I've tried all variations of my attached sample and it never triggers the event. Do 100 send() calls and check bufferedAmount, it will still be 0.
simplewebrtc falls back to setTimeout... which meant it still seems to work. bufferedAmountLow works in Firefox afaics, bufferedAmount goes up to 128k and then back to 0, triggering the event. Not in your demo though.

Another test is to go to make a connection, then drop all udp traffic (which causes an ice failure after some time)
and then pump data into the channel. bufferedAmount goes to 16384. When unblocking udp (quickly enough not to trigger an ice failure) bufferedAmount can go to zero, crossing the treshold without the event firing.

I would say this worked at some point but going back as far as chrome 46 that doesn't seem to be the case.
I'm testing this is Firefox right now and can't get bufferedAmount to change from 0 regardless of how many calls to send() I do. ¯\_(ツ)_/¯
If you're sending small chunks on a local network, they probably all go out before the next round of the event loop, leading you to observe zero.

We need to do proper engineering on bufferedAmount in Chrome. I think it's busted - either we don't report it up at all, or we report the running number rather than have it be only updated when the browser is in a stable state. And on a fast network, the bufferedAmount will effectively remain zero - the NIC card is faster than your writer.

Since the receiver can't slow down receiption (another API design bug in my opinion), it's really hard to write a test for this on a fast network - packets just don't queue up.

You're probably right, but I couldn't get it to work even between my phone of 4G and my laptop at home, so something is very wrong.

IMO, the event should be considered to be triggered after each call to send(). After all, the purpose of the event is to let us trigger the next call to send() correctly. 

Currently, the only way to send a large amount of data on a datachannel is using setTimeout, which will impact the performance.
the problem seems to be that browser don't follow the spec WRT increasing and decreasing which breaks the reasonable assumptions you make.

bufferedAmountLow can trigger -- checkout revision 685f04cb738bfbcc778e8eaa931b8d22578390c9 of the samples.


What does not work is this:
- setting bufferedamountlow to 8192
- calling .send with 16384 bytes. According to spec (https://w3c.github.io/webrtc-pc/#dfn-send step 6) this should increase the bufferedAmount and queue a task to reduce it. But it seems (someone needs to step in with a debugger) to hand off the bytes immediately, breaking the logic you have now and not firing bufferedamountlow.
Labels: -Type-Bug-Regression Type-Bug
Changing from Bug-Regression to Bug, based on the discussion.
Cc: marinaciocea@chromium.org solenberg@chromium.org
Labels: M-70
This and making sure RTCDataChannel attributes are updated per spec ("queue a task that updates internal slot", https://w3c.github.io/webrtc-pc/#dom-rtcdatachannel-bufferedamount) sounds like a good ramp-up project.

It requires some third_party/webrtc updates, a new callback, plumbing that all the way up to Chrome and writing (or making sure existing ones passes, and maybe they need updating) layout tests in WPT, e.g. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCDataChannel-bufferedAmount.html and friends.

There may also be room for Onion Souping (getting rid of content layer RTCDataChannelHandler in favor of a direct blink -> webrtc dependency, https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/rtc_data_channel_handler.h), see https://docs.google.com/document/d/14fsyfCRV_w2VcHDSnAXnhkB4DE83P-u_ewp6bg-HblU/edit?usp=sharing (though that focuses on RTCPeerConnection, but same should apply for RTCDataChannel).
(There are multiple tasks here, to be split up into multiple issues.)
#8: Per-spec, the bufferedAmount should be updated synchronously on send(), and shouldn't be updated to 0 again until a task has been queued and executed (after it was sent by another thread). If the implementation was correct, there would be no race here and bufferedAmount would always be > 0. But the current implementation fetches the most up-to-date value (which most likely is already 0) from the signaling thread here: [1].

The fact that this does a synchronous invoke main thread -> signaling thread upon channel()->buffered_amount() is completely implicit. For good or for bad, the PeerConnectionInterface::CreateDataChannel[2] is implemented to create[3] a proxy[4]. When invoking proxy methods a blocking thread invoke occurs. This is a design pattern prevalent in third_party/webrtc exposed stuff.

[1] https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/rtc_data_channel_handler.cc?sq=package:chromium&dr=CSs&g=0&l=254
[2] https://cs.chromium.org/chromium/src/third_party/webrtc/api/peerconnectioninterface.h?sq=package:chromium&dr=CSs&g=0&l=829
[3] https://cs.chromium.org/chromium/src/third_party/webrtc/pc/peerconnection.cc?sq=package:chromium&dr=CSs&g=0&l=1690
[4] https://cs.chromium.org/chromium/src/third_party/webrtc/pc/datachannel.h?sq=package:chromium&dr=CSs&g=0&l=297
<diatribe> This design pattern (doing blocking invoke() without the thread jump being obvious on the surface) is a royal PITA, and one of the reasons why I'm very skeptical of the idea of merging the webrtc signalling thread with the blink main thread.

If we had only message passing, code would be more complex, but less blocking.
Labels: -M-70 M-71
references:
* https://bugs.chromium.org/p/chromium/issues/detail?id=496700 -- original implementation bug
* https://bugzilla.mozilla.org/show_bug.cgi?id=1178091#c1 -- firefox implementation bug. Note what jesup says about sctp there
Cc: guidou@chromium.org hbos@chromium.org
Owner: ----
Status: Available (was: Assigned)
I meant M-71, though we'll see when this gets worked on, it's not a deadline.

#16: Yes I don't like it. The proxies are obscured by abstract interfaces (the fact that the references are to proxies are treated as an implementation detail, but it's anything but). Deadlocks have occurred due to non-obvious threading behavior, and it's not always predictable (e.g. last reference to a webrtc object would cause a thread block due to destructor being invoked on signaling thread).

The proxies were not primarily chosen due to reducing complexity by hiding message passing but due to the fact that the spec requiring the APIs to be synchronous, which they can't truly be when on different threads (unless you introduce even more threads), so webrtc decided to fake it. And its been faking it good enough that you can only tell the non-compliance in edge cases.

Merging the main thread with the signaling thread would simplify all of this (no concurrency/deadlocks and code actually conforming to the spec) the only question is performance. According to KIR folks the signaling thread shouldn't be used for anything other than API calls which would limit the concerns to getStats() being slow and partially executed on the signaling thread. I think we need an AI to analyze what gets invoked though.

But anyway that's a separate discussion.
Cc: orphis@chromium.org
 Issue 582085  has been merged into this issue.
All the discussions about updating of attributes aside (should file other issues), this issue is about the event not firing. (This caused ehellman@ to have to do a workaround in samples: https://github.com/webrtc/samples/pull/1110)

I looked at the code and I don't see why it wouldn't fire, so we should do a repro fiddle and debug it.
All implementations pass data directly into usrsctp_sendv and don't include sizes of data into the calculation of bufferedAmount which are being sent directly. Therefore, the event is not fired.

Both Firefox and Chrome additionally buffer outside usrsctp and thereby realise that the buffered amount went above 0 after "enough" data has been buffered (at the point where usrsctp_sendv returns EAGAIN). This is reflected by the bufferedAmount attribute but the value is always slightly incorrect (too low).

See:

bufferedAmount: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/datachannel.cc?g=0&l=216 (only considers the queued size)
bufferedamountlowevent: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/datachannel.cc?g=0&l=575 https://cs.chromium.org/chromium/src/third_party/webrtc/pc/datachannel.cc?g=0&l=639 (only compares the queued size)

---

I think it actually doesn't matter that usrsctp does not have a way to access the currently amount of buffered data. Since the value must be frozen until the current JS task returns, we need to calculate this on our own anyway. We could...

1. always add the send size to bufferedAmount if the message has not been queued,
2. fire a task to check if we...
  a) need to fire the event, and
  b) reduce the buffered amount by the message's size if the message was not queued.

---

Working around this bug for now is not too hard. In addition to the code in https://lgrahl.de/examples/dc/dc-stress-buffering.html one should also momentarily pause sending (for example with setTimeout(..., 0)) after having synchronously called .send a few times (depending on the size of the sent data) to avoid blocking the event loop.

Harald, I have never seen a case where the network is faster than the JS generating data and enqueuing it into the SCTP stack. Thus, the above example is very likely to also reliably work in a WPT test. But FWIW I do share your reluctance because of it being racy, which is why I haven't added one so far.
Owner: guidou@chromium.org
Status: Assigned (was: Available)
Owner: ----
Status: Available (was: Assigned)
Owner: marinaciocea@chromium.org

Sign in to add a comment