Bug 95198 - MediaStream API: Introduce MediaConstraints
Summary: MediaStream API: Introduce MediaConstraints
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: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 80589
  Show dependency treegraph
 
Reported: 2012-08-28 07:24 PDT by Tommy Widenflycht
Modified: 2012-08-30 11:30 PDT (History)
16 users (show)

See Also:


Attachments
Patch (47.09 KB, patch)
2012-08-28 08:37 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (47.97 KB, patch)
2012-08-28 12:32 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (49.53 KB, patch)
2012-08-30 09:33 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (49.60 KB, patch)
2012-08-30 10:09 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-08-28 07:24:47 PDT
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.
Comment 1 Tommy Widenflycht 2012-08-28 08:37:35 PDT
Created attachment 160992 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-28 08:40:42 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2012-08-28 08:51:41 PDT
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.
Comment 4 Adam Barth 2012-08-28 08:52:25 PDT
I need to read this again.  MediaConstraintsImpl is an interesting approach.
Comment 5 Tommy Widenflycht 2012-08-28 11:50:19 PDT
(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" } ]
  }
}
Comment 6 Tommy Widenflycht 2012-08-28 12:32:05 PDT
Created attachment 161032 [details]
Patch
Comment 7 Tommy Widenflycht 2012-08-28 12:33:19 PDT
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.
Comment 8 Adam Barth 2012-08-28 13:28:49 PDT
> 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.
Comment 9 Adam Barth 2012-08-29 11:01:53 PDT
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.
Comment 10 Adam Barth 2012-08-29 11:06:37 PDT
> 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?
Comment 11 Adam Barth 2012-08-29 11:09:00 PDT
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
Comment 12 Adam Barth 2012-08-29 11:09:29 PDT
Finally, I don't get why we need to separate MediaConstraints from MediaConstraintsImpl.  What does that buy us?
Comment 13 Tommy Widenflycht 2012-08-30 01:24:15 PDT
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.
Comment 14 Tommy Widenflycht 2012-08-30 01:36:18 PDT
(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.
Comment 15 Adam Barth 2012-08-30 01:41:28 PDT
No abstraction is required.  WebKit can use WebCore types directly.
Comment 16 Adam Barth 2012-08-30 01:43:02 PDT
I'd go with the safer approach.  We can optimize later if this is a bottleneck.
Comment 17 Tommy Widenflycht 2012-08-30 09:33:37 PDT
Created attachment 161494 [details]
Patch
Comment 18 Tommy Widenflycht 2012-08-30 09:34:19 PDT
(In reply to comment #16)
> I'd go with the safer approach.  We can optimize later if this is a bottleneck.

Done.
Comment 19 Adam Barth 2012-08-30 09:37:52 PDT
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
Comment 20 Build Bot 2012-08-30 09:43:32 PDT
Comment on attachment 161494 [details]
Patch

Attachment 161494 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13691754
Comment 21 Tommy Widenflycht 2012-08-30 09:48:42 PDT
(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.
Comment 22 Tommy Widenflycht 2012-08-30 09:51:26 PDT
Hmm, I don't think the mac build problem is due to my code...
Comment 23 Adam Barth 2012-08-30 09:55:39 PDT
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?
Comment 24 Adam Barth 2012-08-30 09:56:19 PDT
> Hmm, I don't think the mac build problem is due to my code...

Yeah, looks like xcode just crashed for some reason.
Comment 25 Tommy Widenflycht 2012-08-30 10:04:29 PDT
(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).
Comment 26 Tommy Widenflycht 2012-08-30 10:09:58 PDT
Created attachment 161505 [details]
Patch
Comment 27 Tommy Widenflycht 2012-08-30 10:11:25 PDT
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.
Comment 28 Adam Barth 2012-08-30 10:34:56 PDT
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.
Comment 29 WebKit Review Bot 2012-08-30 11:30:21 PDT
Comment on attachment 161505 [details]
Patch

Clearing flags on attachment: 161505

Committed r127165: <http://trac.webkit.org/changeset/127165>
Comment 30 WebKit Review Bot 2012-08-30 11:30:27 PDT
All reviewed patches have been landed.  Closing bug.