Bug 98416 - MediaStream API: Update the MediaStream constructor
Summary: MediaStream API: Update the MediaStream constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Bergkvist
URL:
Keywords: WebExposed
Depends on: 103226 103642
Blocks: 56459
  Show dependency treegraph
 
Reported: 2012-10-04 09:07 PDT by Adam Bergkvist
Modified: 2012-12-21 01:16 PST (History)
21 users (show)

See Also:


Attachments
Proposed patch (31.41 KB, patch)
2012-10-04 09:20 PDT, Adam Bergkvist
abarth: review-
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch (27.52 KB, patch)
2012-11-16 05:57 PST, Adam Bergkvist
abarth: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (23.55 KB, patch)
2012-12-04 08:03 PST, Adam Bergkvist
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch (23.55 KB, patch)
2012-12-04 22:43 PST, Adam Bergkvist
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
patch for landing (18.69 KB, patch)
2012-12-20 09:22 PST, Adam Bergkvist
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Bergkvist 2012-10-04 09:07:19 PDT
Update the MediaStream constructor to match the Media Capture and Stream specification as of 13 August 2012 [1]. The patch also based on a minor change [2] that limits track kinds to "audio" and "video" (was "audio", "video" and user agent defined) not yet published as editors draft.

[1] http://dev.w3.org/2011/webrtc/editor/getusermedia-20120813.html#mediastream
[2] https://github.com/fluffy/webrtc-w3c/commit/d531859862578af2265eedb790428d530ddff5ec
Comment 1 Adam Bergkvist 2012-10-04 09:20:54 PDT
Created attachment 167115 [details]
Proposed patch

The custom bindings could share some helper functions. If desirable, what's the best way to do that?
Comment 2 kov's GTK+ EWS bot 2012-10-04 09:53:36 PDT
Comment on attachment 167115 [details]
Proposed patch

Attachment 167115 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14172265
Comment 3 Adam Barth 2012-10-04 12:02:17 PDT
Comment on attachment 167115 [details]
Proposed patch

Why do we need to use a custom constructor?  Can we improve the code generator so we don't need to add more custom code?
Comment 4 Tommy Widenflycht 2012-10-05 00:00:11 PDT
Yeah, I am sorry that custom code are coming back, especially since I fixed a few issues with the code generators and threw out all the old custom code earlier this year, but I can's see how the code generator could easily be modified for this use case.

The constructor now takes an array that can contain one to many objects of three different kinds. I tried to "talk sense" to the standardization committee but they want it like this.

One other possible approach that would avoid custom code is to heavily modify the Dictionary class to be able to return the real objects, but I don't know if that would be much better.
Comment 5 Adam Barth 2012-10-05 09:27:24 PDT
I think we already support overloaded constructors.  I don't think it would be that hard to support overloaded constructors with [].  The IDL declaration would look something like:

interface [
    Conditional=MEDIA_STREAM,
    EventTarget,
    Constructor(in MediaStream[] streams),
    Constructor(in MediaStreamTrackList[] trackLists),
    Constructor(in MediaStreamTrack[] tracks),
    CallWith=ScriptExecutionContext,
    ConstructorRaisesException
] MediaStream {
...
Comment 6 Adam Bergkvist 2012-10-05 09:30:51 PDT
(In reply to comment #4)
> Yeah, I am sorry that custom code are coming back, especially since I fixed a few issues with the code generators and threw out all the old custom code earlier this year, but I can's see how the code generator could easily be modified for this use case.

Yes, it's a tricky translation with a union type. I know that at least the WebSocket constructor has a union type argument to the constructor and it uses a custom binding too.

> The constructor now takes an array that can contain one to many objects of three different kinds. I tried to "talk sense" to the standardization committee but they want it like this.

The discussion [1] kind of died out. I don't know if that means that people are happy with the current behavior or if they don't care. I'm not sure that the current behavior will be the final, but it seems that people like some kind of flexible argument list (some combination of MediaStream, MediaStreamTrackList and MediaStreamTrack objets).

[1] http://lists.w3.org/Archives/Public/public-media-capture/2012Sep/0027.html

Just thinking out loud around this below.

The current definition in WebIDL is:

typedef (MediaStream? or MediaStreamTrackList or MediaStreamTrack) TrackContainer;
[Constructor (TrackContainer[]? trackContainers)]

Could we introduce a type in WebKit that represents the typedef'ed TrackContainer? The corresponding static create function would look something like:

static PassRefPtr<MediaStream> create(ScriptExecutionContext*, TrackContainerVector);

The functionality to extract the tracks from the track containers would then be in MediaStream.cpp instead of the custom binding. That could make it easier to move over to an automatically generated binding since it's pure argument translation.

An alternative to the create function above would be with one list for each type of track container:

static PassRefPtr<MediaStream> create(ScriptExecutionContext*, MediaStreamVector, MediaStreamTrackListVector, MediaStreamTrackVector);
Comment 7 Adam Bergkvist 2012-10-05 09:33:15 PDT
(In reply to comment #5)
> I think we already support overloaded constructors.  I don't think it would be that hard to support overloaded constructors with [].  The IDL declaration would look something like:
> 
> interface [
>     Conditional=MEDIA_STREAM,
>     EventTarget,
>     Constructor(in MediaStream[] streams),
>     Constructor(in MediaStreamTrackList[] trackLists),
>     Constructor(in MediaStreamTrack[] tracks),
>     CallWith=ScriptExecutionContext,
>     ConstructorRaisesException
> ] MediaStream {
> ...

The behavior would be different since you couldn't use two different types in one constructor call. For example:

var audioStream = new MediaStream([localStream.audioTracks, remoteStream.audioTracks[0]]);
Comment 8 Adam Barth 2012-10-05 09:36:53 PDT
I see.  IMHO, we should push back on the standards body to make the API more like other web platform APIs.
Comment 9 Adam Bergkvist 2012-10-09 07:27:17 PDT
We can take it another round the the Media Capture TF. But if the outcome does use a WebIDL union, wouldn't it be OK to let it start off as a custom binding and then modify the bindings generator when some other object needs similar functionality and we know how this type of binding should look.
Comment 10 Adam Bergkvist 2012-11-16 05:57:43 PST
Created attachment 174660 [details]
Updated patch

The spec has been updated with a simplified version of the MediaStream constructor.

[Constructor (),
Constructor (MediaStream? stream),
Constructor (MediaStreamTrack[] tracks)]

It's still not possible to do this without a custom binding, but the bindings are simpler.
Comment 11 Tommy Widenflycht 2012-11-16 06:05:22 PST
Comment on attachment 174660 [details]
Updated patch

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

> Source/WebCore/Modules/mediastream/MediaStream.idl:29
> +    ConstructorParameters=0,

Have you tried to use 
Constructor(in sequence<MediaStreamTrack> tracks)
Constructor(in [Optional] MediaStream stream)
instead of custom bindings?
Comment 12 WebKit Review Bot 2012-11-16 06:45:39 PST
Comment on attachment 174660 [details]
Updated patch

Attachment 174660 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14858479
Comment 13 Adam Barth 2012-11-16 09:35:58 PST
Comment on attachment 174660 [details]
Updated patch

We should be able to do this without custom bindings.
Comment 14 Adam Bergkvist 2012-11-19 07:41:52 PST
(In reply to comment #11)
> (From update of attachment 174660 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174660&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaStream.idl:29
> > +    ConstructorParameters=0,
> 
> Have you tried to use 
> Constructor(in sequence<MediaStreamTrack> tracks)
> Constructor(in [Optional] MediaStream stream)
> instead of custom bindings?

Thanks for the pointer, but it won't work afaik without modifying the bindings generator. I've only seen [] and sequence<> used with basic types (mainly long) and DOMString for arguments.

Constructor(in sequence<MediaStreamTrack> tracks)
will look for:
::create(Vector<MediaStreamTrack>&)
while we want something like:
::create(Vector<RefPtr<MediaStreamTrack> >&)
Comment 15 Adam Barth 2012-11-19 12:29:11 PST
> Thanks for the pointer, but it won't work afaik without modifying the bindings generator.

Then we'll need to improve the bindings generator.  We don't want to add custom code to implement this behavior.  The bindings generator should handle it for us.
Comment 16 Adam Bergkvist 2012-11-19 23:35:16 PST
(In reply to comment #15)
> Then we'll need to improve the bindings generator.  We don't want to add custom code to implement this behavior.  The bindings generator should handle it for us.

OK. Should that be done in a separate bug/patch?
Comment 17 Adam Barth 2012-11-19 23:48:58 PST
> OK. Should that be done in a separate bug/patch?

Yes, that's probably easiest.  You can add a test using run-bindings-tests if you like.
Comment 18 Adam Bergkvist 2012-12-04 08:03:46 PST
Created attachment 177482 [details]
Updated patch

Updated patch with generated bindings for V8 (custom for JSC). Tommy is working on support for overloaded constructors in the JSC bindings generator (http://webkit.org/b/103226) and once he's done, we can remove the JSC custom binding.
Comment 19 kov's GTK+ EWS bot 2012-12-04 10:01:01 PST
Comment on attachment 177482 [details]
Updated patch

Attachment 177482 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15154015
Comment 20 Adam Bergkvist 2012-12-04 22:43:04 PST
Created attachment 177672 [details]
Updated patch

Fixed GTK build error.
Comment 21 Adam Barth 2012-12-04 22:51:34 PST
Comment on attachment 177672 [details]
Updated patch

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:41
> +    for (size_t i = 0; i < sourceVector.size(); ++i)

Please use { } when the body has more than one (textual) line.

> Source/WebCore/bindings/js/JSMediaStreamCustom.cpp:46
> +EncodedJSValue JSC_HOST_CALL JSMediaStreamConstructor::constructJSMediaStream(ExecState* exec)

I thought we were going to avoid custom code here by improving CodeGeneratorJS...
Comment 22 Adam Bergkvist 2012-12-05 00:00:46 PST
(In reply to comment #21)
> (From update of attachment 177672 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177672&action=review
> 
> > Source/WebCore/bindings/js/JSMediaStreamCustom.cpp:46
> > +EncodedJSValue JSC_HOST_CALL JSMediaStreamConstructor::constructJSMediaStream(ExecState* exec)
> 
> I thought we were going to avoid custom code here by improving CodeGeneratorJS...

Yes that's the goal. We had two issues with the code generators. The first, general support for arrays and sequences has been fixed in b103642. The second, support for overloaded constructors is only fixed for V8 at the moment, but Tommy is working on support in CodeGeneratorJS (b103226). What's your view on using a custom binding for JSC for now and remove it as soon as support for overloaded constructors in JSC are fixed?
Comment 23 Adam Barth 2012-12-05 17:34:07 PST
> What's your view on using a custom binding for JSC for now and remove it as soon as support for overloaded constructors in JSC are fixed?

I'd prefer if we fixed bug 103226 first and avoiding having the custom code (even for a short time).  My experience is that once the custom code lands, it's easy for folks to leave it there rather than improving the code generator.
Comment 24 Tommy Widenflycht 2012-12-06 00:48:46 PST
(In reply to comment #23)
> > What's your view on using a custom binding for JSC for now and remove it as soon as support for overloaded constructors in JSC are fixed?
> 
> I'd prefer if we fixed bug 103226 first and avoiding having the custom code (even for a short time).  My experience is that once the custom code lands, it's easy for folks to leave it there rather than improving the code generator.

I feel guilty for not having finished the code generator feature yet but I got drafted to do some WebAudio and SpeechRecognition coding while at the same time trying to get some WebRTC code finished for M25 :/
Not an excuse but maybe an explanation.

However whatever happens it's my P0 task for next week.
Comment 25 Adam Bergkvist 2012-12-06 04:26:39 PST
(In reply to comment #23) 
> I'd prefer if we fixed bug 103226 first and avoiding having the custom code (even for a short time).  My experience is that once the custom code lands, it's easy for folks to leave it there rather than improving the code generator.

Ok I understand.

Could you disregard from the custom binding and continue to review this patch anyhow? It would save us one iteration since the non custom patch is almost identical to this patch without the custom binding.
Comment 26 Adam Barth 2012-12-06 08:50:06 PST
Comment on attachment 177672 [details]
Updated patch

The rest of this patch looks fine.  :)
Comment 27 Adam Bergkvist 2012-12-06 09:41:03 PST
(In reply to comment #26)
> (From update of attachment 177672 [details])
> The rest of this patch looks fine.  :)

Great. :)

Then I'll update the patch when bug 103226 is resolved. If the change only consists of removing the custom binding and "JSCustomConstructor" from the idl I'll land it. Otherwise, if there are any major changes I'll mark it for review again. Sounds ok?
Comment 28 Adam Bergkvist 2012-12-06 09:43:11 PST
I'll fix the first comment from #21 as well.
Comment 29 Adam Barth 2012-12-06 10:06:47 PST
Sgtm
Comment 30 Adam Bergkvist 2012-12-20 09:22:32 PST
Created attachment 180351 [details]
patch for landing

Patch for landing without custom binding for JSC (thanks to bug 103226)
Comment 31 WebKit Review Bot 2012-12-21 01:16:12 PST
Comment on attachment 180351 [details]
patch for landing

Clearing flags on attachment: 180351

Committed r138354: <http://trac.webkit.org/changeset/138354>
Comment 32 WebKit Review Bot 2012-12-21 01:16:20 PST
All reviewed patches have been landed.  Closing bug.