MediaStream API: Update the MediaStream constructor

[Adam Bergkvist]

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

[Adam Bergkvist]

Created attachment 167115 [details]
Proposed patch

The custom bindings could share some helper functions. If desirable, what's the best way to do that?

[kov's GTK+ EWS bot]

Comment on attachment 167115 [details]
Proposed patch

Attachment 167115 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14172265

[Adam Barth]

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?

[Tommy Widenflycht]

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.

[Adam Barth]

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 {
...

[Adam Bergkvist]

(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);

[Adam Bergkvist]

(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]]);

[Adam Barth]

I see.  IMHO, we should push back on the standards body to make the API more like other web platform APIs.

[Adam Bergkvist]

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.

[Adam Bergkvist]

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.

[Tommy Widenflycht]

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?

[WebKit Review Bot]

Comment on attachment 174660 [details]
Updated patch

Attachment 174660 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14858479

[Adam Barth]

Comment on attachment 174660 [details]
Updated patch

We should be able to do this without custom bindings.

[Adam Bergkvist]

(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> >&)

[Adam Barth]

> 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.

[Adam Bergkvist]

(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?

[Adam Barth]

> 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.

[Adam Bergkvist]

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.

[kov's GTK+ EWS bot]

Comment on attachment 177482 [details]
Updated patch

Attachment 177482 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15154015

[Adam Bergkvist]

Created attachment 177672 [details]
Updated patch

Fixed GTK build error.

[Adam Barth]

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...

[Adam Bergkvist]

(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?

[Adam Barth]

> 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.

[Tommy Widenflycht]

(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.

[Adam Bergkvist]

(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.

[Adam Barth]

Comment on attachment 177672 [details]
Updated patch

The rest of this patch looks fine.  :)

[Adam Bergkvist]

(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?

[Adam Bergkvist]

I'll fix the first comment from #21 as well.

[Adam Barth]

Sgtm

[Adam Bergkvist]

Created attachment 180351 [details]
patch for landing

Patch for landing without custom binding for JSC (thanks to bug 103226)

[WebKit Review Bot]

Comment on attachment 180351 [details]
patch for landing

Clearing flags on attachment: 180351

Committed r138354: <http://trac.webkit.org/changeset/138354>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.