Bug 68464 – Update MediaStream to use WebCore platform interfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Bergkvist]

Modify MediaStream/MediaStreamTrack to use MediaStreamManager and store its data in MediaStreamDescriptor.

[Adam Bergkvist]

Created attachment 110604 [details]
proposed patch

[Adam Barth]

Comment on attachment 110604 [details]
proposed patch

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

This looks pretty good.  There's still the question of whether |context| should be first in constructors.  Also, almost all the places in this patch where we're using unsigned, we probably should be using size_t.

> Source/WebCore/dom/MediaStream.cpp:48
> +    MediaStreamTrackVector trackVector;
> +    unsigned numberOfTracks = m_descriptor->numberOfComponents();

You can call reserveCapacity on the vector if you know ahead of time how many items you're going to append.

> Source/WebCore/dom/MediaStreamTrack.cpp:50
> +    DEFINE_STATIC_LOCAL(AtomicString, kind, (m_streamDescriptor->component(m_trackIndex)->source()->type() == MediaStreamSource::TypeAudio ? "audio" : "video"));

It's strange to use a static local that's based on member variables...  This doesn't seem right.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Adam Barth]

@fishd: This just deletes interfaces from the WebKit API.  The plan is to add new interfaces to WebKitPlatformSupport later.

[Adam Bergkvist]

Created attachment 110871 [details]
Updated patch

(In reply to comment #2)
> (From update of attachment 110604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110604&action=review
> 
> This looks pretty good.  There's still the question of whether |context| should be first in constructors.

I've updated the patch to follow the new context first convention.

> Also, almost all the places in this patch where we're using unsigned, we probably should be using size_t.

Fixed "internal" usage (left unsigned in the DOM API signatures).

> > Source/WebCore/dom/MediaStream.cpp:48
> > +    MediaStreamTrackVector trackVector;
> > +    unsigned numberOfTracks = m_descriptor->numberOfComponents();
> 
> You can call reserveCapacity on the vector if you know ahead of time how many items you're going to append.

Good point. Fixed.

> > Source/WebCore/dom/MediaStreamTrack.cpp:50
> > +    DEFINE_STATIC_LOCAL(AtomicString, kind, (m_streamDescriptor->component(m_trackIndex)->source()->type() == MediaStreamSource::TypeAudio ? "audio" : "video"));
> 
> It's strange to use a static local that's based on member variables...  This doesn't seem right.

True. That was a bad last minute optimization. :) Fixed.

[Adam Barth]

Comment on attachment 110871 [details]
Updated patch

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

In one of your earlier patches, you moved all the non-platform files into a mediastream directory.  I think that would be a valuable change to do sooner rather than later.

Please fix the out-of-bounds read before landing.

> Source/WebCore/ChangeLog:8
> +        Update MediaStream to use WebCore platform interfaces
> +        https://bugs.webkit.org/show_bug.cgi?id=68464
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Currently not testable.

Generally we like a litt more information in ChangeLogs.  For this change, I'd explain that this is part of a series of patches that convert the DOM interfaces over to using a new Platform API.

Also, I'd include some information about how you plan to test this code and at what stage of this process we'll be able to add tests.  I understand that there are a lots of wires to connect before everything is fully functional, but we might be able to add some basic API tests soon.

> Source/WebCore/dom/MediaStream.h:84
> +    ScriptExecutionContext* m_scriptExecutionContext;

This doesn't need to be a RefPtr?

> Source/WebCore/dom/MediaStreamTrack.cpp:51
> -const String& MediaStreamTrack::kind() const
> +String MediaStreamTrack::kind() const
>  {
> -    return m_kind;
> +    DEFINE_STATIC_LOCAL(AtomicString, audioKind, ("audio"));
> +    DEFINE_STATIC_LOCAL(AtomicString, videoKind, ("video"));

Should this function return an AtomicString?  If not, then should these statics be Strings rather than AtomicStrings?  (Let me know if you're unsure of the difference between Strings and AtomicStrings.)

> Source/WebCore/dom/MediaStreamTrack.cpp:61
> +    ASSERT_NOT_REACHED();
>  }

I'm surprised you don't need to return String() here.

> Source/WebCore/dom/MediaStreamTrack.cpp:70
> +    return m_streamDescriptor->component(m_trackIndex)->enabled();

It's kind of strange that all of these methods are basically trampolines into m_streamDescriptor components.  I guess this object is like a DOM wrapper around the Platform concept, which makes sense, but they we're holding on to the root objects in the Platform object tree and walking the tree each time.  I guess that's fine.  It's just slightly different from what I woud have expected.

> Source/WebCore/dom/MediaStreamTrack.h:54
> +    const size_t m_trackIndex;

We don't usually have const member variables, but it works nicely here.

> Source/WebCore/dom/MediaStreamTrackList.cpp:54
> +    ASSERT(index < length());

This is an API exposed to JavaScript, right?  What ensures that JavaScript won't call this function with a nutty index?

> Source/WebCore/page/MediaStreamController.h:44
>  class MediaStreamController {

All of MediaStreamController is going away eventually, right?

> Source/WebCore/page/MediaStreamFrameController.h:52
>      MediaStreamFrameController(Frame*);

This one is going away too, I presume.

[Adam Bergkvist]

Created attachment 111261 [details]
Patch 3

(In reply to comment #6)
> (From update of attachment 110871 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110871&action=review
> 
> In one of your earlier patches, you moved all the non-platform files into a mediastream directory.  I think that would be a valuable change to do sooner rather than later.
>

I've created a bug to move the files. See http://webkit.org/b/70233

> Please fix the out-of-bounds read before landing.

item() is an index getter and thus the binding will prevent out-of-bounds reads. 

> > Source/WebCore/ChangeLog:8
> > +        Update MediaStream to use WebCore platform interfaces
> > +        https://bugs.webkit.org/show_bug.cgi?id=68464
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Currently not testable.
> 
> Generally we like a litt more information in ChangeLogs.  For this change, I'd explain that this is part of a series of patches that convert the DOM interfaces over to using a new Platform API.
>

Fixed.

> Also, I'd include some information about how you plan to test this code and at what stage of this process we'll be able to add tests.  I understand that there are a lots of wires to connect before everything is fully functional, but we might be able to add some basic API tests soon.
>

Fixed (now referring to an existing bug).

> > Source/WebCore/dom/MediaStream.h:84
> > +    ScriptExecutionContext* m_scriptExecutionContext;
> 
> This doesn't need to be a RefPtr?

I made it a RefPtr to be safe.

> > Source/WebCore/dom/MediaStreamTrack.cpp:51
> > -const String& MediaStreamTrack::kind() const
> > +String MediaStreamTrack::kind() const
> >  {
> > -    return m_kind;
> > +    DEFINE_STATIC_LOCAL(AtomicString, audioKind, ("audio"));
> > +    DEFINE_STATIC_LOCAL(AtomicString, videoKind, ("video"));
> 
> Should this function return an AtomicString?  If not, then should these statics be Strings rather than AtomicStrings?  (Let me know if you're unsure of the difference between Strings and AtomicStrings.)
>

Fixed (they don't have to be AtomicStrings).

> 
> > Source/WebCore/dom/MediaStreamTrack.cpp:70
> > +    return m_streamDescriptor->component(m_trackIndex)->enabled();
> 
> It's kind of strange that all of these methods are basically trampolines into m_streamDescriptor components.  I guess this object is like a DOM wrapper around the Platform concept, which makes sense, but they we're holding on to the root objects in the Platform object tree and walking the tree each time.  I guess that's fine.  It's just slightly different from what I woud have expected.

We need to hold on to the MediaStreamDescriptor object since a MediaStreamTrack can exist without its MediaStream and the descriptor is the key to do platform operations on a stream (including the tracks). Since the descriptor is available we can get the information we want from it, but we could cache the information in member variables.

> > Source/WebCore/dom/MediaStreamTrackList.cpp:54
> > +    ASSERT(index < length());
> 
> This is an API exposed to JavaScript, right?  What ensures that JavaScript won't call this function with a nutty index?
>

The binding makes sure that this function is called with a valid index (0 <= index < length()).

> > Source/WebCore/page/MediaStreamController.h:44
> >  class MediaStreamController {
> 
> All of MediaStreamController is going away eventually, right?
>
> > Source/WebCore/page/MediaStreamFrameController.h:52
> >      MediaStreamFrameController(Frame*);
> 
> This one is going away too, I presume.

We have a single controller and a page client that only deals with user consent for getUserMedia(). I hope that they could be accepted as replacements for the current controllers and client. I'll file a bug to discuss this further.

[WebKit Review Bot]

Attachment 111261 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> @fishd: This just deletes interfaces from the WebKit API.  The plan is to add new interfaces to WebKitPlatformSupport later.

OK!

[Adam Barth]

Thanks!

[WebKit Review Bot]

Comment on attachment 111261 [details]
Patch 3

Rejecting attachment 111261 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rror: no match for 'operator!' in '!sources'
Source/WebCore/page/MediaStreamFrameController.cpp:256: note: candidates are: operator!(bool) <built-in>
Source/JavaScriptCore/wtf/text/WTFString.h:368: note:                 bool WTF::operator!(const WTF::String&)
  CXX(target) out/Debug/obj.target/webcore_remaining/Source/WebCore/page/WindowFeatures.o
make: *** [out/Debug/obj.target/webcore_remaining/Source/WebCore/page/MediaStreamFrameController.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/10145353

[Adam Bergkvist]

Created attachment 111616 [details]
Patch for landing

Removed bad ASSERT.

[WebKit Review Bot]

Comment on attachment 111616 [details]
Patch for landing

Clearing flags on attachment: 111616

Committed r97904: <http://trac.webkit.org/changeset/97904>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.