Bug 112314

Summary: Add support of sending blob data for RTCDataChannel.
Product: WebKit Reporter: Li Yin <li.yin>
Component: WebCore Misc.Assignee: Li Yin <li.yin>
Status: NEW ---    
Severity: Normal CC: abarth, ap, dglazkov, eric.carlson, feature-media-reviews, haraken, hta, jer.noble, mkwst+watchlist, nick.diego, peter+ews, syoichi, tkent, tommyw, toyoshim, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch abarth: review-

Description Li Yin 2013-03-13 19:46:33 PDT
From spec: http://dev.w3.org/2011/webrtc/editor/webrtc.html#widl-RTCDataChannel-send-void-Blob-data
RTCDataChannel can send blob data, WebKit doesn't implement it.
I am preparing the patch.
Comment 1 Li Yin 2013-03-14 21:17:01 PDT
Created attachment 193229 [details]
Patch
Comment 2 Kentaro Hara 2013-03-14 21:22:45 PDT
tommyw: would you take a look?
Comment 3 Tommy Widenflycht 2013-03-15 02:39:11 PDT
Comment on attachment 193229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193229&action=review

It looks really good but maybe you should try to get someone from e.g. WebSocket to have a look as well.

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:172
>  {

Please check that the data channel isn't closed first.

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:299
> +            // FIXME: Will determine if raising exception should be necessary.

It isn't possible to throw an exception here so you can remove the comment.

> LayoutTests/fast/mediastream/RTCPeerConnection-datachannel.html:87
> +shouldBe("dc.binaryType", "'blob'");

Nit: You can use shouldBeEqualToString here instead.
Comment 4 Li Yin 2013-03-15 03:17:40 PDT
Hi Takashi Toyoshima,
Could you please review this patch? The implementation looks like WebSocket. Thanks in advance.
Comment 5 Li Yin 2013-03-15 03:40:07 PDT
Created attachment 193273 [details]
Patch
Comment 6 Li Yin 2013-03-15 03:42:04 PDT
(In reply to comment #3)
> (From update of attachment 193229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193229&action=review
> 
> It looks really good but maybe you should try to get someone from e.g. WebSocket to have a look as well.
> 
> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:172
> >  {
> 
> Please check that the data channel isn't closed first.
> 
> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:299
> > +            // FIXME: Will determine if raising exception should be necessary.
> 
> It isn't possible to throw an exception here so you can remove the comment.
> 
> > LayoutTests/fast/mediastream/RTCPeerConnection-datachannel.html:87
> > +shouldBe("dc.binaryType", "'blob'");
> 
> Nit: You can use shouldBeEqualToString here instead.

Fixed.
Thanks for your review.
Comment 7 Takashi Toyoshima 2013-03-16 18:13:10 PDT
+tkent

Hi Li,
I'm familiar with WebSocket, but not a WebKit reviewer yet.
tkent or ap looks proper reviewers in this case.

Kent-san, can you take a look?
Comment 8 Li Yin 2013-03-18 06:40:40 PDT
Created attachment 193549 [details]
Patch
Comment 9 Li Yin 2013-03-18 06:44:57 PDT
Update the patch for handling failed blob loading.
When the blob data can't be loaded correctly (e.g. the File or Blob resource could not be found at the time the read was processed), skip the blob, and continue to send the next.
Comment 10 Mike West 2013-03-18 06:59:37 PDT
Comment on attachment 193549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193549&action=review

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:278
> +        String errorMessage("Failed to load Blob: error code = " + String::number(errorCode));

Nit: There's no need for a temporary String here; you can simply add the string directly to the call to 'addConsoleMessage'.

Relatedly, is there any more detail we can provide here beyond an opaque number? Does "1" in the error code that shows up in the tests actually mean anything? As a developer, I'm not sure what I'd do with this information.
Comment 11 Peter Beverloo (cr-android ews) 2013-03-18 07:05:17 PDT
Comment on attachment 193549 [details]
Patch

Attachment 193549 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17066817
Comment 12 Li Yin 2013-03-19 05:25:30 PDT
Created attachment 193800 [details]
Patch
Comment 13 Li Yin 2013-03-19 05:30:21 PDT
(In reply to comment #9)
> Update the patch for handling failed blob loading.
> When the blob data can't be loaded correctly (e.g. the File or Blob resource could not be found at the time the read was processed), skip the blob, and continue to send the next.

When the blob data can't be loaded successfully, user agent must close the channel, like what WebSocket is doing.
And add the sub test for failed blob loading.
Comment 14 Tommy Widenflycht 2013-03-19 05:43:12 PDT
(In reply to comment #13)
> (In reply to comment #9)
> > Update the patch for handling failed blob loading.
> > When the blob data can't be loaded correctly (e.g. the File or Blob resource could not be found at the time the read was processed), skip the blob, and continue to send the next.
> 
> When the blob data can't be loaded successfully, user agent must close the channel, like what WebSocket is doing.
> And add the sub test for failed blob loading.

Actually it isn't specified in the WebRTC spec what should happen if a blob fails to load but after discussing it with hta we agree that this is probably the best thing to do.
Comment 15 Harald Alvestrand 2013-03-19 05:56:52 PDT
I think this (closing) is the right way - but can you point me at the place in the WebSocket API spec that says what to do when the data can't be loaded correctly? I couldn't find it on a quick read - the send data methods all say "fail with prejudice if the data can't be sent" - is that the one you're thinking of?
Comment 16 Li Yin 2013-03-19 06:02:46 PDT
(In reply to comment #15)
> I think this (closing) is the right way - but can you point me at the place in the WebSocket API spec that says what to do when the data can't be loaded correctly? I couldn't find it on a quick read - the send data methods all say "fail with prejudice if the data can't be sent" - is that the one you're thinking of?

Yes, and see the discussion https://bugs.webkit.org/show_bug.cgi?id=112534#c5
Comment 17 Adam Barth 2013-03-20 17:43:38 PDT
Comment on attachment 193800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193800&action=review

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:178
> +    m_outgoingBlobQueue.append(Blob::create(data->url(), data->type(), data->size()));

Why do we need to create a new blob here?

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:229
> +        OwnPtr<Vector<char> > binaryData = adoptPtr(new Vector<char>(dataLength));
> +        memcpy(binaryData->data(), data, dataLength);
> +        RefPtr<RawData> rawData = RawData::create();
> +        binaryData->swap(*rawData->mutableData());
> +        OwnPtr<BlobData> blobData = BlobData::create();
> +        blobData->appendData(rawData.release(), 0, BlobDataItem::toEndOfFile);
> +        RefPtr<Blob> blob = Blob::create(blobData.release(), dataLength);

This all seems really low-level for this file.  Does Blob not know how to create itself from a sequence of bytes?

The benefit of using a blob is that you don't need to bring all the data into memory.  If you're going to bring all the data into memory anyway, you might as well use an array buffer.  To have a high-quality blob implementation, we'd need to pass an opaque handle to the data from its source all the way here.
Comment 18 Li Yin 2013-03-21 06:45:56 PDT
(In reply to comment #17)
> (From update of attachment 193800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193800&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:178
> > +    m_outgoingBlobQueue.append(Blob::create(data->url(), data->type(), data->size()));
> 
> Why do we need to create a new blob here?
> 
> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:229
> > +        OwnPtr<Vector<char> > binaryData = adoptPtr(new Vector<char>(dataLength));
> > +        memcpy(binaryData->data(), data, dataLength);
> > +        RefPtr<RawData> rawData = RawData::create();
> > +        binaryData->swap(*rawData->mutableData());
> > +        OwnPtr<BlobData> blobData = BlobData::create();
> > +        blobData->appendData(rawData.release(), 0, BlobDataItem::toEndOfFile);
> > +        RefPtr<Blob> blob = Blob::create(blobData.release(), dataLength);
> 
> This all seems really low-level for this file.  Does Blob not know how to create itself from a sequence of bytes?

Blob has no direct method to do that, it can be created through BlobData.

> 
> The benefit of using a blob is that you don't need to bring all the data into memory.  If you're going to bring all the data into memory anyway, you might as well use an array buffer.  To have a high-quality blob implementation, we'd need to pass an opaque handle to the data from its source all the way here.

The data has been in memory already, I encapsulated the raw data into a blob object, meet the requirement of RTCDataChannel API. And WebSocket is doing the same thing.
Please See: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/websockets/WebSocket.cpp#L531
Comment 19 Adam Barth 2013-03-21 13:49:19 PDT
> Blob has no direct method to do that, it can be created through BlobData.

Yes, we would need to change that.

> The data has been in memory already, I encapsulated the raw data into a blob object, meet the requirement of RTCDataChannel API.

I understand.  I'm saying that's a poor implementation.

> And WebSocket is doing the same thing.

WebSockets has a different architecture.  In WebSockets, we need to bring all the data into memory because the protocol handling is done in WebCore.  For WebRTC, the protocol handling is done outside of WebCore.  That means the handle to the data should be created outside of WebCore.
Comment 20 Li Yin 2013-03-21 18:19:07 PDT
(In reply to comment #19)
> > Blob has no direct method to do that, it can be created through BlobData.
> 
> Yes, we would need to change that.

I agree with you, we can move the implementation to Blob.cpp, avoid code copies in WebSocket and RTCDataChannel.

> 
> > The data has been in memory already, I encapsulated the raw data into a blob object, meet the requirement of RTCDataChannel API.
> 
> I understand.  I'm saying that's a poor implementation.
> 
> > And WebSocket is doing the same thing.
> 
> WebSockets has a different architecture.  In WebSockets, we need to bring all the data into memory because the protocol handling is done in WebCore.  For WebRTC, the protocol handling is done outside of WebCore.  That means the handle to the data should be created outside of WebCore.

Do you mean we save the raw data into a file and return the file object to Web Developers when onmessage event handle was triggered?
If that is true, the data will be copied from memory to disk at first, and then copied from disk to memory when Web developers read the blob(in fact File object).
It will have two extra functions, reading and writing file, I think there will be a lower performance.
Comment 21 Li Yin 2013-03-24 19:58:02 PDT
(In reply to comment #17)
> (From update of attachment 193800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193800&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:178
> > +    m_outgoingBlobQueue.append(Blob::create(data->url(), data->type(), data->size()));
> 
> Why do we need to create a new blob here?

Because blob will be handled asynchronously, we need create a new Blob object.
The original blob may be destructed because of gc when the loading resource function is invoked.
Comment 22 Li Yin 2013-03-24 20:15:33 PDT
(In reply to comment #19)
> WebSockets has a different architecture.  In WebSockets, we need to bring all the data into memory because the protocol handling is done in WebCore.  For WebRTC, the protocol handling is done outside of WebCore.  That means the handle to the data should be created outside of WebCore.

I don't think so, there are two reasons as followed:
1. the binary data will be interpreted to be arraybuffer or blob, which is determined by m_binaryType, and m_binaryType variable is only accessed in WebCore.
2. If we move this code away from WebCore, all other ports will implement the same codes like chromium.
Comment 23 Li Yin 2013-04-02 17:35:35 PDT
Hi abarth,
Do you think my clarification is reasonable? Could you please have a look again?