Summary: | [GStreamer][WebRTC] Allow backend to keep track of buffered-amount | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||
Component: | WebRTC | Assignee: | Philippe Normand <pnormand> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, philn, sergio, tommyw, webkit-bug-importer, youennf | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 235885 | ||||||
Attachments: |
|
Description
Philippe Normand
2022-01-31 01:29:47 PST
Created attachment 450388 [details]
Patch
Comment on attachment 450388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450388&action=review > Source/WebCore/ChangeLog:10 > + corresponding property instead. m_bufferedAmount is mapping precisely to https://w3c.github.io/webrtc-pc/#dfn-bufferedamount. By adding the logic in RTCDataChannel, we can follow the spec to the letter, for instance: - https://w3c.github.io/webrtc-pc/#datachannel-send step 6 (Increase [[BufferedAmount]] for each send call synchronously). - https://w3c.github.io/webrtc-pc/#dom-datachannel-bufferedamount (backend to queue a task to reduce [[BufferedAmount]] asynchronously). What is the goal of this change? Won't it affect interop for GstWebRTC? GstWebRTC follows the spec and defines a public interface in: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-bad/gst-libs/gst/webrtc/datachannel.c So with the current code we end-up managing buffered-amount and low-threshold twice, which is prone to inconsistencies, and actually introduces test failures in webrtc/ data-channel tests with the GstWebRTC backend enabled. That's why I propose this patch :) Fortunately I didn't have to change much of the platform-agnostic WebRTC code WebKit, most of the GStreamer-specific code will be contained to dedicated platform modules. (In reply to Philippe Normand from comment #3) > GstWebRTC follows the spec and defines a public interface in: > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/ > gst-plugins-bad/gst-libs/gst/webrtc/datachannel.c > > So with the current code we end-up managing buffered-amount and > low-threshold twice, which is prone to inconsistencies, and actually > introduces test failures in webrtc/ data-channel tests with the GstWebRTC > backend enabled. That's why I propose this patch :) > > Fortunately I didn't have to change much of the platform-agnostic WebRTC > code WebKit, most of the GStreamer-specific code will be contained to > dedicated platform modules. I am not sure this will work properly, due to blob data being sent asynchronously to the backend, but the blob size impacting bufferedAmount synchronously (see NetworkSendQueue). Here is an example: // amount is initially zero for channel and backend. channel.send(string1); // RTCDataChannel amount is string1.size. backend amount is also string1.size. channel.send(blob1); // RTCDataChannel amount is string1.size+blob1.size, backend amount is string1.size since blob1 data will be received asynchronously. channel.send(string2); // RTCDataChannel amount is string1.size+blob1.size+string2.size, backend amount is still string1.size. Right, this approach is a bit flawed indeed, as the gstwebrtc implementation also has an internal queue (in the appsrc element)... Pull request: https://github.com/WebKit/WebKit/pull/1157 Committed 251990@main (af95424cb233): <https://commits.webkit.org/251990@main> Reviewed commits have been landed. Closing PR #1157 and removing active labels. |