Bug 70895 - Add WebCore platform interface needed by updated MediaStream API design
Summary: Add WebCore platform interface needed by updated MediaStream API design
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: Nobody
URL:
Keywords:
Depends on:
Blocks: 67946
  Show dependency treegraph
 
Reported: 2011-10-26 02:42 PDT by Adam Bergkvist
Modified: 2011-11-22 06:26 PST (History)
7 users (show)

See Also:


Attachments
Proposed patch (10.92 KB, patch)
2011-11-17 09:55 PST, Adam Bergkvist
abarth: review-
Details | Formatted Diff | Diff
Updated patch (15.02 KB, patch)
2011-11-21 08:01 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 2011-10-26 02:42:39 PDT
Add the remaining platform files needed by MediaStream and LocalMediaStream to WebCore/platform/mediastream.
Comment 1 Adam Bergkvist 2011-11-17 09:55:08 PST
Created attachment 115608 [details]
Proposed patch
Comment 2 Adam Barth 2011-11-17 10:20:08 PST
Comment on attachment 115608 [details]
Proposed patch

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

> Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:57
> +void MediaStreamCenter::endLocalMediaStream(MediaStreamDescriptor* streamDescriptor)
> +{
> +    MediaStream* stream = streamDescriptor->owner();
> +    if (stream)
> +        stream->streamEnded();
> +    else
> +        streamDescriptor->setEnded();
> +}

This function doesn't appear to have any callers.  Why wouldn't the callers just do this work directly?  What's the value in having a static MediaStreamCenter to help?

> Source/WebCore/platform/mediastream/MediaStreamCenter.h:44
> +class MediaStreamCenter {

Can you help me understand what role the MediaStreamCenter plays?
Comment 3 Adam Bergkvist 2011-11-18 03:01:24 PST
(In reply to comment #2)
> (From update of attachment 115608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115608&action=review
> 
> > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:57
> > +void MediaStreamCenter::endLocalMediaStream(MediaStreamDescriptor* streamDescriptor)
> > +{
> > +    MediaStream* stream = streamDescriptor->owner();
> > +    if (stream)
> > +        stream->streamEnded();
> > +    else
> > +        streamDescriptor->setEnded();
> > +}
> 
> This function doesn't appear to have any callers.  Why wouldn't the callers just do this work directly?  What's the value in having a static MediaStreamCenter to help?

It works as a utility function, but its main purpose is layering so that the platform doesn't have to be aware of DOM objects (MediaStream).
 
> > Source/WebCore/platform/mediastream/MediaStreamCenter.h:44
> > +class MediaStreamCenter {
> 
> Can you help me understand what role the MediaStreamCenter plays?

DOM objects:                              | Browser chrome:
                                          |
 +--------------+  +-------------------+  |  +-------------------+  +----------------+
 | MediaStream: |  | LocalMediaStream: |  |  | Mute Video Button |  | getUserMedia   |
 | stream1      |  | stream2           |  |  +-------------------+  | Selector UI    |
 +--------------+  +-------------------+  |                         | (user consent) |
                                          |                         +----------------+
                                          |

                                 | Requests from DOM objects and the WebKit API
                                 V
                              +-------------------+
==============================| MediaStreamCenter |====================================
                              +-------------------+
                                 ^
                                 | Requests from platform layer to higher layers
Platform:

 +=================+
 | Active streams: |
 +=================+
 | stream1         |
 +-----------------+
 | stream2         |
 +-----------------+

MediaStreamCenter responsibilities:
* Expose media stream source probing functionality to higher layers
* Notify the platform that:
 - A MediaStreamTrack has been enabled/disabled (from JavaScript)
 - A LocalMediaStream has been stopped (from JavaScript)
 - A local MediaStreamSource has been muted (via mute button in browser chrome)
* Notify a MediaStream that the corresponding stream in the platfom has ended (e.g. USB camera was unplugged)

Our platform specific implementation of MediaStreamCenter is responsible for all the plumbing of the media pipelines (e.g. from local media stream sources to player sinks and network transport sinks).
Comment 4 Adam Barth 2011-11-18 19:27:41 PST
Does MediaStreamCenter keep any state?  If not, perhaps it should be a collection of free functions.
Comment 5 Adam Barth 2011-11-18 19:30:31 PST
Comment on attachment 115608 [details]
Proposed patch

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

> Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:38
> +#include "MediaStream.h"

Another issue is that MediaStream.h is outside the Platform directory and Platform isn't allowed to include headers from the rest of WebCore.

> Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:48
> +MediaStreamCenter& MediaStreamCenter::instance()
> +{
> +    ASSERT(isMainThread());
> +    DEFINE_STATIC_LOCAL(MediaStreamCenter, center, ());
> +    return center;
> +}

The reason I ask these questions is that "static" is almost certainly the wrong scope for any state in WebKit.
Comment 6 Adam Barth 2011-11-18 19:30:46 PST
Comment on attachment 115608 [details]
Proposed patch

Marking r- for now.
Comment 7 Adam Bergkvist 2011-11-21 08:01:51 PST
Created attachment 116089 [details]
Updated patch

(In reply to comment #4)
> Does MediaStreamCenter keep any state?  If not, perhaps it should be a collection of free functions.

Yes, e.g., each media capture device is only represented by a single MediaStreamSource instance so the MediaStreamCenter must keep track of the MediaStreamSource instances it has created.

(In reply to comment #5)
> (From update of attachment 115608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115608&action=review
> 
> > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:38
> > +#include "MediaStream.h"
> 
> Another issue is that MediaStream.h is outside the Platform directory and Platform isn't allowed to include headers from the rest of WebCore.

Fixed (introduced MediaStreamDescriptorOwner on the platform level). Fixed similar issue with UserMediaRequest.

> > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:48
> > +MediaStreamCenter& MediaStreamCenter::instance()
> > +{
> > +    ASSERT(isMainThread());
> > +    DEFINE_STATIC_LOCAL(MediaStreamCenter, center, ());
> > +    return center;
> > +}
> 
> The reason I ask these questions is that "static" is almost certainly the wrong scope for any state in WebKit.

One reason it's static is to allow MediaStreamSource instances (which correspond to specific media capture devices) to out-live everything else. A MediaStreamSource can be used by several unrelated pages and muting a source takes effect in all pages.
Comment 8 Adam Barth 2011-11-21 11:04:27 PST
I see, so there really is global static state in this API.  That's unfortunate, but it sounds unavoidable.  Thanks for answering my questions.
Comment 9 Adam Bergkvist 2011-11-22 04:07:48 PST
Comment on attachment 116089 [details]
Updated patch

Thank you for the review.
Comment 10 WebKit Review Bot 2011-11-22 06:26:47 PST
Comment on attachment 116089 [details]
Updated patch

Clearing flags on attachment: 116089

Committed r100997: <http://trac.webkit.org/changeset/100997>
Comment 11 WebKit Review Bot 2011-11-22 06:26:56 PST
All reviewed patches have been landed.  Closing bug.