Skip to content
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

WebRTC data channels proposal #480

Open
wants to merge 6 commits into
base: development
Choose a base branch
from
Open

Conversation

tomasz-zajac
Copy link
Contributor

Adds WebRTC data channel usage specification and defines subprotocol for streaming XML metadata.

doc/WebRTC.xml Outdated Show resolved Hide resolved
doc/WebRTC.xml Outdated Show resolved Hide resolved
<personname>Tomasz Zajac</personname>
</author>
<revremark>Added data channels.</revremark>
</revision>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tomasz,
just want to highlight these lines as they specify at the start of this document that the use of Data Channels are is outside of the scope of this version of the specification.

Can I please ask you update the same also. I have reviewed the content and was going to approve the same but for these sentences.

specs/doc/WebRTC.xml

Lines 66 to 68 in 94b5e2c

<para> Sending event and analytics metadata as well as PTZ and other commands via WebRTC
datachannels is outside of the scope of this version of the specifcation. Similar future
versions of the specification may address a directory protocol to query organizations,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Kieran, I have updated this paragraph and also removed mentions of directory protocol, which I believe is no longer valid.

Server -> Client: { "method": "invite", "params": {"session": "s1", "offer": "SDP..."}, "id":3}
Client -> Server: { "result": {"answer": "SDP..."}, "id": 3}
Server -> Device: { "result": {"answer": "SDP..."}, "id": 3}
Device -> Server: { "method": "invite", "params": {"session": "s1", "offer": "SDP...",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the example flow, it seems dataChannelSubprotocols offer/response is outside of SDP but as part of INVITE transaction.

But the specification text in Section Enabling data channels has the text as quoted

  • "it shall signal this in the SDP offer of the invite method. If the client supports data channels and intends to use them during the WebRTC session, it shall signal this in the SDP answer."

May I suggest text should say

  • it shall signal this as part of the invite method. If the client supports data channels and intends to use them during the WebRTC session, it shall signal this in the invite response along with SDP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two levels of negotiation:

  1. First is standard WebRTC datachannel negotiation, which is part of the SDP offer/answer. This is required for WebRTC clients to enable data channels in the session.
  2. Second is our ONVIF protocols negotiation, which is required to enable specific ONVIF subprotocols and they are not part of the SDP, but instead INVITE request-answer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specification is not very clear about this 2-level negotiation, from my understanding the current example flow seems to cover/represent level-2 of negotiation process in INVITE, Would it be possible to add some example for the level-1 negotiation in SDP offer/answer? Or is that kind of covered in WebRTC RFC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a standard way of enabling data channels covered by WebRTC W3C specs/RFCs and it usually involves calling createDataChannel() on PeerConnection object before SDP is exchanged. In my opinion the first paragraph is sufficient, it avoids referring to specific WebRTC APIs on how this can be achieved, which may be implementation specific.

doc/WebRTC.xml Outdated
<para>If the device supports WebRTC data channels, it shall signal this in the SDP offer
of the invite method. If the client supports data channels and intends to use them during
the WebRTC session, it shall signal this in the SDP answer.</para>
<para>ONVIF defines data channel subprotocol names starting with the “vnd.onvif” prefix.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ONVIF defined data channel subprotocol names shall have “vnd.onvif” prefix. (start and prefix words add redundancy)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased as suggested

doc/WebRTC.xml Outdated
the WebRTC session, it shall signal this in the SDP answer.</para>
<para>ONVIF defines data channel subprotocol names starting with the “vnd.onvif” prefix.
Usage of this prefix is restricted to ONVIF-defined subprotocols only.</para>
<para>Before creating and using any data channel subprotocol, the device and client need
Copy link
Contributor

@bsriramprasad bsriramprasad Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested Text

  • Before creation and usage of any data channel subprotocol, the device and client shall exchange a list of allowed data channel subprotocols in the invite method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased as suggested

doc/WebRTC.xml Outdated
<para>At any time after obtaining a WebRTC connection, a client may request enabling a
pre-negotiated data channel subprotocol by creating a new data channel with the specified
subprotocol identifier. This triggers a new data channel event on the device, which shall
enable the requested data channel subprotocol only after ensuring it was negotiated during
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This triggers a new data channel event on the device, which shall enable the requested data channel subprotocol only after ensuring it was negotiated during the SDP exchange.

  • For me this whole text seems to be an implementation (may be part of a library) detail and we should discuss if this has to be in the specification?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree this sentence reflects a behavior of most WebRTC stacks implementation, however I added it with an intention to help understanding the flow of events between client requesting a data channel and device reacting to it. We can discuss if we want to keep it or not.

Copy link
Contributor

@bsriramprasad bsriramprasad Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to remove the same to avoid ambiguity of specification vs additional implementation related information, let's hear from WG too for their feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased

Copy link
Contributor

@kieran242 kieran242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a single question. Not a blocker at the moment so approving.

datachannels is outside of the scope of this version of the specifcation. Similar future
versions of the specification may address a directory protocol to query organizations,
devices and profiles. </para>
<para>Sending PTZ and other commands via WebRTC datachannels is outside of the scope of this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that there was discussion on the Uplink allowing PTZ communication via SOAP requests across the HTTP/2 Backchannel or WebSocket. In this context a Client and Device are making a Peer To Peer connection via WebRTC and that device may be a PTZ Camera and should this spec not allow the client to perform PTZ over the WebRTC backchannel.

Is there reasons why this is not updated as part of the Spec or are you only focusing on Metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial intention was to include SOAP commands over data channel as well, however during last F2F meeting there was no interest from the client side to standardize it. We can revisit this again and if there is any vendor wishing to prototype this on the client side, I will add it.

<para><emphasis role="bold">Subprotocol identifier</emphasis>: vnd.onvif.metadata+gzip</para>
<para>The Metadata data channel subprotocol enables streaming XML Metadata over WebRTC data
channels. Only GZIP compression is supported; uncompressed XML Metadata is not supported.</para>
<para>A device shall use the MetadataConfiguration from the media streaming profile selected
Copy link
Member

@HansBusch HansBusch Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to rephrase to positive requirement statement:

An ONVIF compliant device supporting streaming of metadata shall support gzip compressed metadata as signaled via the subprotocol vnd.onvif.metadata+gzip.

This automatically implies that there is no guarantee for devices supporting uncompressed transmission.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased as suggested

@kieran242
Copy link
Contributor

Re-reviewed the latest updates. Already Approved and happy with latest changes.

doc/WebRTC.xml Outdated
<para>At any time after obtaining a WebRTC connection, a client may start using a data
channel subprotocol by creating a new data channel with the specified subprotocol
identifier. When a new data channel event is triggered on a device, the device should
enable the relevant data channel subprotocol.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a subprotocol is attempted to be used by a client, but is not allowed (as per the initial handshakes), what should the device do? And how to verify in a test tool that the refusal works as expected? If the data channel remains open, but no data is sent by the device, should the verification step involve some kind of timeout limit before the test passes? It would be cleaner and more direct if the device instead actively closes the data channel? Or simply closes the whole peer-to-peer session?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think closing the data channel explicitly by the device makes most sense, as the client will also get the feedback. I will add such requirement if that's OK for everyone.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me. And again it's no big deal, just thought it could simplify verification

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomasz-zajac I am also happy with the update you proposed to @jcadev

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcadev Updated now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants