Add support of sending blob data for RTCDataChannel

Add support of sending blob data for RTCDataChannel

Spec: http://dev.w3.org/2011/webrtc/editor/webrtc.html#widl-RTCDataChannel-send-void-Blob-data

BUG=

[Li Yin, 2014-05-04 08:16:11]

Now, Blob is created through BlobDataHandle, the raw data is stored in Browser
process, not in the WebCore anymore.

[abarth-chromium, 2014-05-05 21:42:15]

Do we need to route the data through the render process?  Can't we just send the
data from the blob to the data channel in the browser process?

[abarth-chromium, 2014-05-05 21:42:45]

+michaeln

[Li Yin, 2014-05-06 01:51:28]

On 2014/05/05 21:42:15, abarth wrote:
> Do we need to route the data through the render process?  Can't we just send
the
> data from the blob to the data channel in the browser process?

Yes, data package is handled in webrtc library, which is running in render
process.
Please see:
https://chromium.googlesource.com/chromium/src.git/+/master/content/renderer/...

[michaeln, 2014-05-06 02:36:49]

On 2014/05/06 01:51:28, Li Yin wrote:
> On 2014/05/05 21:42:15, abarth wrote:
> > Do we need to route the data through the render process?  Can't we just send
> the
> > data from the blob to the data channel in the browser process?
> 
> Yes, data package is handled in webrtc library, which is running in render
> process.
> Please see:
>
https://chromium.googlesource.com/chromium/src.git/+/master/content/renderer/...

It's unfortunate that data is routed so inefficiently, WebSockets is similarly
arranged (XHR.responseBlob too). This design pattern made more sense when blink
was webkit. Now that it's just blink and chromium, should things like this be
redesigned to function in a way that makes more sense in blink + chromium?
Eventually it should, when does eventually get here?

https://codereview.chromium.org/268923002/diff/1/Source/modules/mediastream/R...
File Source/modules/mediastream/RTCDataChannel.cpp (right):

https://codereview.chromium.org/268923002/diff/1/Source/modules/mediastream/R...
Source/modules/mediastream/RTCDataChannel.cpp:261: OwnPtr<Vector<char> >
binaryData = adoptPtr(new Vector<char>(dataLength));
this also probably won't work well with large blobs

https://codereview.chromium.org/268923002/diff/1/Source/modules/mediastream/R...
Source/modules/mediastream/RTCDataChannel.cpp:377: RefPtr<ArrayBuffer> result =
m_blobLoader->arrayBufferResult();
this probably won't work well with large blobs, have you considered while
(!done) { readsome(); sendsome(); } to avoid having to read the entire thing
into memory at once?

[michaeln, 2014-05-06 03:38:12]

This looks a lot more workable than websockets, iircc, websockets has a lot of
message framing logic in webcore/blink. This code does not, there's virtually no
logic between RTCDataChannel::send(blob) and a call out into chromium running in
the renderer. And vice versa on message reception.

Seems like the webkit api layer could be changed to express sendBlob in addition
to sendStringDat) and sendRawData in the out going direction, and similary for
the incoming, didReceiveBlob() in addition to didReceiveRawData and
didReceiveStringData. Instead of sending all the blob data back and forth, just
send it's uuid, content-type and size across process boundaries.

[Li Yin, 2014-05-06 06:48:53]

On 2014/05/06 03:38:12, michaeln wrote:
> This looks a lot more workable than websockets, iircc, websockets has a lot of
> message framing logic in webcore/blink. This code does not, there's virtually
no
> logic between RTCDataChannel::send(blob) and a call out into chromium running
in
> the renderer. And vice versa on message reception.
> 
> Seems like the webkit api layer could be changed to express sendBlob in
addition
> to sendStringDat) and sendRawData in the out going direction, and similary for
> the incoming, didReceiveBlob() in addition to didReceiveRawData and
> didReceiveStringData. Instead of sending all the blob data back and forth,
just
> send it's uuid, content-type and size across process boundaries.

RtcDataChannelHandler::sendRawData and RtcDataChannelHandler::sendStringData
will call webrtc::DataChannelInterface::Send() to send the data.
webrtc::DataChannelInterface existed in the third party libjingle repo, it is
also running in render process, and it requires real data, no matter string or
binary.
Please see:
https://chromium.googlesource.com/external/webrtc/trunk/talk.git/+/master/app...
So it seems to be a must that loading all the blob into memory before calling
third party library to send data.

[Li Yin, 2014-05-06 07:09:36]

https://codereview.chromium.org/268923002/diff/1/Source/modules/mediastream/R...
File Source/modules/mediastream/RTCDataChannel.cpp (right):

https://codereview.chromium.org/268923002/diff/1/Source/modules/mediastream/R...
Source/modules/mediastream/RTCDataChannel.cpp:261: OwnPtr<Vector<char> >
binaryData = adoptPtr(new Vector<char>(dataLength));
On 2014/05/06 02:36:49, michaeln wrote:
> this also probably won't work well with large blobs

When blob is large, it needs much memory, do you mean we need a protection for
out of memory? I found WebSocket and XHR meets the same issue.

https://codereview.chromium.org/268923002/diff/1/Source/modules/mediastream/R...
Source/modules/mediastream/RTCDataChannel.cpp:377: RefPtr<ArrayBuffer> result =
m_blobLoader->arrayBufferResult();
On 2014/05/06 02:36:49, michaeln wrote:
> this probably won't work well with large blobs, have you considered while
> (!done) { readsome(); sendsome(); } to avoid having to read the entire thing
> into memory at once?

Yeah, your concern looks reasonable, but unfortunately, DataChannelInterface in
libjingle doesn't have the splitting capability.
DataChannelInterface::Send() means data was sent completely, the remote peer
will receive a message.

[Li Yin, 2014-05-06 07:26:54]

On 2014/05/06 03:38:12, michaeln wrote:
> This looks a lot more workable than websockets, iircc, websockets has a lot of
> message framing logic in webcore/blink. This code does not, there's virtually
no
> logic between RTCDataChannel::send(blob) and a call out into chromium running
in
> the renderer. And vice versa on message reception.
> 
> Seems like the webkit api layer could be changed to express sendBlob in
addition
> to sendStringDat) and sendRawData in the out going direction, and similary for
> the incoming, didReceiveBlob() in addition to didReceiveRawData and
> didReceiveStringData. Instead of sending all the blob data back and forth,
just
> send it's uuid, content-type and size across process boundaries.

For WebRTC, both of media and data are handled in third party "libjingle" and
"webrtc", not in Blink and Chromium, which is the most different from
WebSocket&XHR.
P2PSocketDispatcher and P2PSocketDispatcherHost are the process boundaries
between render and browser process, they are just a simple wrapper for sending
and receiving, no understanding the real data.

[michaeln, 2014-05-07 01:26:37]

> P2PSocketDispatcher and P2PSocketDispatcherHost are the process boundaries
> between render and browser process, they are just a simple wrapper for sending
> and receiving, no understanding the real data.

Yup, i question that design for moving blob/bytes around in chromium. Since the
DataChannelInterface doesn't let you incrementally stream an individual message
you're kind of stuck loading the whole blob into memory. If
DataChannelInterface/DataChannelObserver could be modified to work with
PartialDataBuffers that have a .lastPart data member, then that interface could
be more suitable for use in the browser process.

From my point of view, implementing the loop de loop way with reading-all-in
across ipc,then writing-all-out across ipc is more technical debt. In practical
terms, sending/receiving blobs in this cl doesn't provide anything new. The blob
variants, don't give the ability to send/recv larger amounts of data.

What is the motivation do do this now? And back to my earlier question "when
does eventually get here" to pipe blobs around in a sane way (by reference and
not by value)?

[Li Yin, 2014-05-07 01:38:12]

On 2014/05/07 01:26:37, michaeln wrote:
> What is the motivation do do this now? And back to my earlier question "when
> does eventually get here" to pipe blobs around in a sane way (by reference and
> not by value)?

This CL added the Blob data type supporting for DataChannel, which is needed by
Spec.
After landing this CL, web developers can upload a file, and then directly send
file to remote peer, which is an important scenario.
Does it answer your question?

[Li Yin, 2014-05-07 01:51:10]

On 2014/05/07 01:26:37, michaeln wrote:
> From my point of view, implementing the loop de loop way with reading-all-in
> across ipc,then writing-all-out across ipc is more technical debt. In
practical
> terms, sending/receiving blobs in this cl doesn't provide anything new. The
blob
> variants, don't give the ability to send/recv larger amounts of data.

Yes, the data is transferring from browser to render, and then render to browser
process again, it looks bad.
But unfortunately, it seems have no methods to avoid it according to current
design.

When the blob data is very large, it is indeed a problem, but I don't think it
is the main requirement of web developers.
And we can sync with WebRTC team to provide the partial package supporting in
the following steps, if it is really needed to fix.

[michaeln, 2014-05-07 01:58:17]

On 2014/05/07 01:38:12, Li Yin wrote:
> On 2014/05/07 01:26:37, michaeln wrote:
> > What is the motivation do do this now? And back to my earlier question "when
> > does eventually get here" to pipe blobs around in a sane way (by reference
and
> > not by value)?
> 
> This CL added the Blob data type supporting for DataChannel, which is needed
by
> Spec.
> After landing this CL, web developers can upload a file, and then directly
send
> file to remote peer, which is an important scenario.
> Does it answer your question?

So i think i hear to complete support the spec'd api in general, not a pressing
need for anything in particular. Do you know what the status of support for his
is in other browsers, are chromium based browsers the only browsers that don't
support this api (in their latest versions)? I'm wondering if not having this
api is a compat issue that's hurting anything right now (like only chrome
doesn't work on sitex because of this).

I was just chatting with adam about how we feel dirty about taking the easy way
out on blob handling use cases like this (and making the copies and such). If
there's real motivation to add the api now then expediency wins, but if not,
digging into handling blobs by reference for RTCDataChannel might be a good
thing.

[Li Yin, 2014-05-07 02:23:38]

There is a related article about WebRTC data channel, which is a good
description for the scenario.
http://www.html5rocks.com/en/tutorials/webrtc/datachannels/#build-a-file-shar...

[Li Yin, 2014-05-07 03:10:16]

On 2014/05/07 01:58:17, michaeln wrote:
> On 2014/05/07 01:38:12, Li Yin wrote:
> > On 2014/05/07 01:26:37, michaeln wrote:
> > > What is the motivation do do this now? And back to my earlier question
"when
> > > does eventually get here" to pipe blobs around in a sane way (by reference
> and
> > > not by value)?
> > 
> > This CL added the Blob data type supporting for DataChannel, which is needed
> by
> > Spec.
> > After landing this CL, web developers can upload a file, and then directly
> send
> > file to remote peer, which is an important scenario.
> > Does it answer your question?
> 
> So i think i hear to complete support the spec'd api in general, not a
pressing
> need for anything in particular. Do you know what the status of support for
his
> is in other browsers, are chromium based browsers the only browsers that don't
> support this api (in their latest versions)? I'm wondering if not having this
> api is a compat issue that's hurting anything right now (like only chrome
> doesn't work on sitex because of this).
> 
The blob data type was added spec before more one year ago. It should be enough
stable to implement. Firefox has already supported it.

[Li Yin, 2014-05-07 03:25:32]

On 2014/05/07 01:58:17, michaeln wrote:
> I was just chatting with adam about how we feel dirty about taking the easy
way
> out on blob handling use cases like this (and making the copies and such). If
> there's real motivation to add the api now then expediency wins, but if not,
> digging into handling blobs by reference for RTCDataChannel might be a good
> thing.

As I mentioned before, libjingle and webrtc third party is running in render
process, almost all the data is handled there.
P2PSocketDispatcher and P2PSocketDispatcherHost doesn't know what is real data,
since the data is under encryption.
IPC should not be avoided in current architecture.

[michaeln1, 2014-05-07 03:33:35]

Hi Li,

I think there's a misunderstanding. I'm not at all questioning the api or ready
or whether chrome should support it, ready enough and of course chromium should.

I'm questioning what characteristics the implementation should have, should it
be efficient or not? I see the current cl as not so efficient due to moving
bytes back and forth and due to requiring blobs be loaded entirely into memory.
If time is of the essence the inefficient cl would probably the best option. But
if there is no real time pressure, I'm suggesting it might be better to do this
more efficiently. It's harder to do that because i think it would require larger
changes to webrtc (like change the ipc protocol and probably put the webrtc
library in the browser process).

Do my question about how to do this make sense?

[Li Yin, 2014-05-07 05:30:58]

On 2014/05/07 03:33:35, michaeln1 wrote:
> Hi Li,
> 
> I think there's a misunderstanding. I'm not at all questioning the api or
ready
> or whether chrome should support it, ready enough and of course chromium
should.
> 
> I'm questioning what characteristics the implementation should have, should it
> be efficient or not? I see the current cl as not so efficient due to moving
> bytes back and forth and due to requiring blobs be loaded entirely into
memory.
> If time is of the essence the inefficient cl would probably the best option.
But
> if there is no real time pressure, I'm suggesting it might be better to do
this
> more efficiently. It's harder to do that because i think it would require
larger
> changes to webrtc (like change the ipc protocol and probably put the webrtc
> library in the browser process).
> 
> Do my question about how to do this make sense?

Thanks for your clarification.
Now, I understand what you mean.
Let me add more experts into this loop.

[Li Yin, 2014-05-07 05:33:57]

+perkj
+sergeyu

[perkj_chrome, 2014-05-16 10:37:24]

Sorry - I don't know anything about this and Tommy W is out sick and is not
expected back any time soon. 

abarth- can you please advice?

[perkj_chrome, 2014-05-16 10:46:41]

On 2014/05/16 10:37:24, perkj wrote:
> Sorry - I don't know anything about this and Tommy W is out sick and is not
> expected back any time soon. 
> 
> abarth- can you please advice?

Sorry abarth - I saw you first comment- RTCPeerConnection data channels are
implemented  by sctp-data channels in libjingle and thus need to be in the
RenderProcess. 
Tommy W would have been the person to ask about blink - Chrome integration but
as said, he is  ooo and it may be a along time before he comes back. 

For libjingle questions related to this - try to ask ldixon@ and jiayl@