Bug 95198 – MediaStream API: Introduce MediaConstraints

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tommy Widenflycht]

This introduces MediaConstraints together with relevant infrastructure, a chromium mock and LayoutTests.
I had to add a function to Dictionary for extracting the names of the attributes to get this to work.

[Tommy Widenflycht]

Created attachment 160992 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 160992 [details]
Patch

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

> Source/WebCore/bindings/v8/Dictionary.cpp:514
> +        String stringKey = toWebCoreString(key);
> +        names.append(stringKey);

I'd combine these two lines.

[Adam Barth]

I need to read this again.  MediaConstraintsImpl is an interesting approach.

[Tommy Widenflycht]

(In reply to comment #4)
> I need to read this again.  MediaConstraintsImpl is an interesting approach.

Interesting is such a ambivalent word ;) I did it like this since I wanted to keep a Dictionary in an object that will be sent through the WebKit interface.


Also a better description of the MediaConstraints objects is in order, it's not trivial.

A constraint is a key:value pair, and example is minVideoWidth:1024. The constraint can be either mandator or optional.

MediaConstraints contains two members, mandatory and optional, whose structures are not the same.

"mandatory" is unordered and therefore is a simple sub-Dictionary. If a web developer specifies a mandatory constraint which isn't supported by the user agent an exception should be thrown.

"optional" is an ordered list of key:value pairs and each constraint should be in its own Dictionary.

{
  mandatory: {
    minVideoWidth:640
  },
  optional: {
    [ { minVideoWidth:1024 }, { magic:true }, { awesomeness:"super" } ]
  }
}

[Tommy Widenflycht]

Created attachment 161032 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 160992 [details]
Patch

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

>> Source/WebCore/bindings/v8/Dictionary.cpp:514
>> +        names.append(stringKey);
> 
> I'd combine these two lines.

Done.

[Adam Barth]

> Interesting is such a ambivalent word ;)

I didn't mean to be ambivalent.  I just meant that I need to look at it in more detail to understand.

[Adam Barth]

Comment on attachment 161032 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:63
> +    Dictionary m_mandatoryConstraints;

I don't think you can hold a Dictionary are a member variable.  I'll explain why in a moment.

> Source/WebCore/bindings/js/Dictionary.cpp:88
> +    ExecState* exec = m_dictionary.execState();

The problem is that you can't hold an ExecState* as a member.  The ExecState's lifetime is limited to the call stack.

> Source/WebCore/platform/chromium/support/WebMediaConstraints.cpp:67
> +    m_private->getConstraintNames(mandatory, constraintNames);

As you've written things, this function requires a executing JavaScript, but given that this is a public API, there's no reason to believe that we're inside a JavaScript context (or even have a HandleScope on the stack).

> Source/WebCore/platform/mediastream/MediaConstraints.h:41
> +class MediaConstraints : public RefCounted<MediaConstraints> {

I don't understand why we need the abstract base class.  There's only one implementation of this class that I can see.

[Adam Barth]

> Also a better description of the MediaConstraints objects is in order, it's not trivial.

I see.  Maybe it would be better to represent this state explicitly with C++ types rather than leaving it in JavaScript objects?  Presumably we need to copy it out of the JavaScript VM at some point.  Perhaps we're trying to save a copy by copying it directly into the embedder?

The problem is that Dictionary object can't really live past the stack frame in which they are created.  They hold onto a bunch of ephemeral state that explodes as you unwind the stack.  Maybe you've set things up so that MediaConstraints are only ever used in their own stack frame?

I don't see anything that prevents the embedder from holding onto a WebMediaConstraints object.  Even if this work today, it's a pretty fragile API.

Is there a strong reason not to just copy the data out of JS?

[Adam Barth]

If you're worried that these things are variants and C++ doesn't have a good way of representing variants, we can use InspectorValue.

For example, we can use v8ToInspectorValue or ScriptValue::toInspectorValue

[Adam Barth]

Finally, I don't get why we need to separate MediaConstraints from MediaConstraintsImpl.  What does that buy us?

[Tommy Widenflycht]

Comment on attachment 161032 [details]
Patch

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

>> Source/WebCore/platform/mediastream/MediaConstraints.h:41
>> +class MediaConstraints : public RefCounted<MediaConstraints> {
> 
> I don't understand why we need the abstract base class.  There's only one implementation of this class that I can see.

It's this platform abstraction again... The abstract base class defines the interface for the object that will go from WebCore/platform to WebKit, whereas the implementation is then free to use WebCore proper objects and APIs.

[Tommy Widenflycht]

(In reply to comment #10)
> > Also a better description of the MediaConstraints objects is in order, it's not trivial.
> 
> I see.  Maybe it would be better to represent this state explicitly with C++ types rather than leaving it in JavaScript objects?  Presumably we need to copy it out of the JavaScript VM at some point.  Perhaps we're trying to save a copy by copying it directly into the embedder?
> 
> The problem is that Dictionary object can't really live past the stack frame in which they are created.  They hold onto a bunch of ephemeral state that explodes as you unwind the stack.  Maybe you've set things up so that MediaConstraints are only ever used in their own stack frame?
> 
> I don't see anything that prevents the embedder from holding onto a WebMediaConstraints object.  Even if this work today, it's a pretty fragile API.

Right, MediaConstraints are only supposed to be used in the context of the call but as you rightly point out the API doesn't stop holding on to the object.

> 
> Is there a strong reason not to just copy the data out of JS?

Here's the reason I did it this way:

I can create a C++ class but I wanted to push the responsibility for knowing about valid constraints and their types to the user agent, otherwise both WebKit and the UA have to be patched as soon as a new constraint is added. This way only the UA is affected.

Another alternative is for webkit to extract everything as strings and let the UA convert to the right type as needed but it adds some extra overhead. No big deal though, and converting to InspectorValues will do the same, so maybe this is a better approach. It is certainly much safer.

[Adam Barth]

No abstraction is required.  WebKit can use WebCore types directly.

[Adam Barth]

I'd go with the safer approach.  We can optimize later if this is a bottleneck.

[Tommy Widenflycht]

Created attachment 161494 [details]
Patch

[Tommy Widenflycht]

(In reply to comment #16)
> I'd go with the safer approach.  We can optimize later if this is a bottleneck.

Done.

[Adam Barth]

Comment on attachment 161494 [details]
Patch

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

This looks good, but we still have the extra MediaConstraints class.

> Source/WebCore/bindings/js/Dictionary.h:64
> +    bool getOwnPropertyNames(WTF::Vector<String>&) const;

WTF::Vector -> Vector

[Build Bot]

Comment on attachment 161494 [details]
Patch

Attachment 161494 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13691754

[Tommy Widenflycht]

(In reply to comment #15)
> No abstraction is required.  WebKit can use WebCore types directly.

The entire MediaStream feature is built upon these rules:

The WebKit API only sees and uses WebCore/platform classes (WebMediaConstraints)
WebCore/platform classes doesn't use anything from WebCore proper (MediaConstraints)
WebCore proper uses webCore/platform classes (MediaConstraintsImpl)

Previously this has been solved by the *Descriptor pattern, whereas here I thought inheritance would work better.

So if I understand you correctly you would prefer if MediaConstraints in WebCore/platform directly uses the Dictionary class. I would prefer not so that the WebCore/platform pulls in as little as possible.

[Tommy Widenflycht]

Hmm, I don't think the mac build problem is due to my code...

[Adam Barth]

Ah!  I misunderstood what you wrote in Comment #13.

Now that we're not keeping the Dictionary around, do you think it would be more consistent with the rest of the feature to copy the data into a Descriptor object in the platform layer rather than into the DOM object?

[Adam Barth]

> Hmm, I don't think the mac build problem is due to my code...

Yeah, looks like xcode just crashed for some reason.

[Tommy Widenflycht]

(In reply to comment #23)
> Ah!  I misunderstood what you wrote in Comment #13.

No problems.

> 
> Now that we're not keeping the Dictionary around, do you think it would be more consistent with the rest of the feature to copy the data into a Descriptor object in the platform layer rather than into the DOM object?

If you don't scream I would like to keep it like this, and even use it for two more classes (to be reviewed soon).

[Tommy Widenflycht]

Created attachment 161505 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 161494 [details]
Patch

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

>> Source/WebCore/bindings/js/Dictionary.h:64
>> +    bool getOwnPropertyNames(WTF::Vector<String>&) const;
> 
> WTF::Vector -> Vector

Fixed.

[Adam Barth]

Comment on attachment 161505 [details]
Patch

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

Ok.  If you think this pattern is better in some cases, we can give it a try.  Thanks for being patient with me.  :)

> Source/WebCore/bindings/v8/Dictionary.cpp:497
> +bool Dictionary::getOwnPropertyNames(WTF::Vector<String>& names) const

Technically this doesn't need a WTF either.  The WTF headers tend to have "using" statements that import these names.

[WebKit Review Bot]

Comment on attachment 161505 [details]
Patch

Clearing flags on attachment: 161505

Committed r127165: <http://trac.webkit.org/changeset/127165>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.