Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9309078:
Adding a skeleton MediaStreamCenter. (Closed)
|
Created:
8 years, 10 months ago by tommyw Modified:
8 years, 9 months ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, acolwell+watch_chromium.org, jam, annacc+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org Visibility:
Public. |
DescriptionReplaced by http://codereview.chromium.org/9619006/
Adding a skeleton MediaStreamCenter.
This patch adds the foundation for the WebKit WebMediaStreamCenter. No actual functionality.
BUG=
TEST=
Patch Set 1 #
Total comments: 9
Patch Set 2 : Review comments attended #
Total comments: 2
Patch Set 3 : Fixed review comment #
Total comments: 8
Patch Set 4 : Review comments fixed #
Total comments: 6
Patch Set 5 : Fixed review comments #
Total comments: 1
Messages
Total messages: 22 (0 generated)
https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... File content/renderer/media/media_stream_center.h (right): https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... content/renderer/media/media_stream_center.h:23: const WebKit::WebMediaStreamSourcesRequest& request); when overriding virtual methods, use the OVERRIDE macro: virtual void Foo() OVERRIDE; https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... File content/renderer/media/media_stream_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... content/renderer/media/media_stream_impl.cc:113: DVLOG(1) << "A PeerConnection already exists"; should this be a NOTREACHED()? (a question for HenrikG) https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... content/renderer/media/media_stream_impl.cc:131: media_stream_center_ = new MediaStreamCenter(client); why do we hold on to the pointer? Isn't the ownership passed to the caller? https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/render... File content/renderer/render_view_impl.h (right): https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/render... content/renderer/render_view_impl.h:226: WebKit::WebMediaStreamCenter* CreateMediaStreamCenter( documentation? particularly about ownership and how many CMSC objects can be created.
https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... File content/renderer/media/media_stream_center.h (right): https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... content/renderer/media/media_stream_center.h:23: const WebKit::WebMediaStreamSourcesRequest& request); On 2012/02/03 14:39:45, tommi wrote: > when overriding virtual methods, use the OVERRIDE macro: > > virtual void Foo() OVERRIDE; Done. https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... File content/renderer/media/media_stream_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... content/renderer/media/media_stream_impl.cc:113: DVLOG(1) << "A PeerConnection already exists"; On 2012/02/03 14:39:45, tommi wrote: > should this be a NOTREACHED()? (a question for HenrikG) No, we need to support more than one PeerConnection per tab, this is just a temporary limitation. https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... content/renderer/media/media_stream_impl.cc:131: media_stream_center_ = new MediaStreamCenter(client); On 2012/02/03 14:39:45, tommi wrote: > why do we hold on to the pointer? > Isn't the ownership passed to the caller? Eventually MediaStreamImpl (aka the glue code) need to use this pointer to send messages to WebKit, so just future-proofing. https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/render... File content/renderer/render_view_impl.h (right): https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/render... content/renderer/render_view_impl.h:226: WebKit::WebMediaStreamCenter* CreateMediaStreamCenter( On 2012/02/03 14:39:45, tommi wrote: > documentation? > particularly about ownership and how many CMSC objects can be created. Done.
lgtm. I'll wait for Henrik's comments.
LGTM, but I'd like a note about the lifetime though it's stated in render view. https://chromiumcodereview.appspot.com/9309078/diff/11/content/renderer/media... File content/renderer/media/media_stream_impl.h (right): https://chromiumcodereview.appspot.com/9309078/diff/11/content/renderer/media... content/renderer/media/media_stream_impl.h:166: // media_stream_center_ is a weak reference, owned by WebKit. Add a comment about the lifetime of |media_stream_center_| would be good. Or a todo for this since this class isn't used yet. Is |media_stream_center_| guaranteed to live longer than this?
https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... File content/renderer/media/media_stream_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/1/content/renderer/media/... content/renderer/media/media_stream_impl.cc:113: DVLOG(1) << "A PeerConnection already exists"; On 2012/02/03 14:58:25, tommyw wrote: > On 2012/02/03 14:39:45, tommi wrote: > > should this be a NOTREACHED()? (a question for HenrikG) > > No, we need to support more than one PeerConnection per tab, this is just a > temporary limitation. Agree, furthermore WebKit should be allowed to call several times even if the Chrome embedder has this limitation at the moment. Maybe the log message isn't perfect but we will support several PCs pretty soon.
https://chromiumcodereview.appspot.com/9309078/diff/11/content/renderer/media... File content/renderer/media/media_stream_impl.h (right): https://chromiumcodereview.appspot.com/9309078/diff/11/content/renderer/media... content/renderer/media/media_stream_impl.h:166: // media_stream_center_ is a weak reference, owned by WebKit. On 2012/02/06 20:00:38, Henrik Grunell wrote: > Add a comment about the lifetime of |media_stream_center_| would be good. Or a > todo for this since this class isn't used yet. Is |media_stream_center_| > guaranteed to live longer than this? Done.
Two try bots running: http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/15121 http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/14119 The mac is waaaay behind, so I'll post that link later.
Need owners approval for content/renderer. Thanks in advance.
Could you give me a pointer to the webkit side? https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... File content/renderer/media/media_stream_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... content/renderer/media/media_stream_impl.cc:133: DCHECK(!media_stream_center_); I haven't seen the webkit side, but what ensures this is called only once per RenderViewImpl? https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... File content/renderer/media/media_stream_impl.h (right): https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... content/renderer/media/media_stream_impl.h:166: // media_stream_center_ is a weak reference, owned by WebKit. It's valid for "weak reference" suggests base::WeakPtr, which this is not. Could you say "raw pointer" instead? https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... File content/renderer/render_view_impl.h (right): https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... content/renderer/render_view_impl.h:227: // Only one is created per render process. This comment doesn't match the implementation: there seems to be one per RenderViewImpl. https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... content/renderer/renderer_webkitplatformsupport_impl.cc:698: RenderViewImpl* render_view = findRenderView(); Is there any way we can avoid this? This is taking the render view from the javascript context, which overall seems iffy. Is it possible to create the WebMediaStreamCenter through the WebViewClient on the webkit side, that goes directly to RenderViewImpl.
Hi, Thanks for your review. The corresponding WebKit change can be viewed here: http://trac.webkit.org/changeset/106581 https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... File content/renderer/media/media_stream_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... content/renderer/media/media_stream_impl.cc:133: DCHECK(!media_stream_center_); The corresponding WebKit object MediaStreamCenter is a singleton. On 2012/02/08 01:52:24, piman wrote: > I haven't seen the webkit side, but what ensures this is called only once per > RenderViewImpl? https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... File content/renderer/media/media_stream_impl.h (right): https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... content/renderer/media/media_stream_impl.h:166: // media_stream_center_ is a weak reference, owned by WebKit. It's valid for On 2012/02/08 01:52:24, piman wrote: > "weak reference" suggests base::WeakPtr, which this is not. Could you say "raw > pointer" instead? Done. https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... File content/renderer/render_view_impl.h (right): https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... content/renderer/render_view_impl.h:227: // Only one is created per render process. On 2012/02/08 01:52:24, piman wrote: > This comment doesn't match the implementation: there seems to be one per > RenderViewImpl. Done. https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... content/renderer/renderer_webkitplatformsupport_impl.cc:698: RenderViewImpl* render_view = findRenderView(); Short answer: No Longer answer: the MediaStreamCenter is not a page client, it lives in the WebCore platform layer. Therefore we can't do what you suggest. On 2012/02/08 01:52:24, piman wrote: > Is there any way we can avoid this? This is taking the render view from the > javascript context, which overall seems iffy. Is it possible to create the > WebMediaStreamCenter through the WebViewClient on the webkit side, that goes > directly to RenderViewImpl.
On 2012/02/08 13:14:18, tommyw wrote: > Hi, > Thanks for your review. > > The corresponding WebKit change can be viewed here: > http://trac.webkit.org/changeset/106581 > > https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... > File content/renderer/media/media_stream_impl.cc (right): > > https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... > content/renderer/media/media_stream_impl.cc:133: DCHECK(!media_stream_center_); > The corresponding WebKit object MediaStreamCenter is a singleton. > > On 2012/02/08 01:52:24, piman wrote: > > I haven't seen the webkit side, but what ensures this is called only once per > > RenderViewImpl? > > https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... > File content/renderer/media/media_stream_impl.h (right): > > https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/med... > content/renderer/media/media_stream_impl.h:166: // media_stream_center_ is a > weak reference, owned by WebKit. It's valid for > On 2012/02/08 01:52:24, piman wrote: > > "weak reference" suggests base::WeakPtr, which this is not. Could you say "raw > > pointer" instead? > > Done. > > https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... > File content/renderer/render_view_impl.h (right): > > https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... > content/renderer/render_view_impl.h:227: // Only one is created per render > process. > On 2012/02/08 01:52:24, piman wrote: > > This comment doesn't match the implementation: there seems to be one per > > RenderViewImpl. > > Done. > > https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... > File content/renderer/renderer_webkitplatformsupport_impl.cc (right): > > https://chromiumcodereview.appspot.com/9309078/diff/9001/content/renderer/ren... > content/renderer/renderer_webkitplatformsupport_impl.cc:698: RenderViewImpl* > render_view = findRenderView(); > Short answer: No > > Longer answer: the MediaStreamCenter is not a page client, it lives in the > WebCore platform layer. Therefore we can't do what you suggest. If it's really a singleton, shouldn't it hang off the RenderThreadImpl then, instead of the RenderViewImpl? Antoine > > On 2012/02/08 01:52:24, piman wrote: > > Is there any way we can avoid this? This is taking the render view from the > > javascript context, which overall seems iffy. Is it possible to create the > > WebMediaStreamCenter through the WebViewClient on the webkit side, that goes > > directly to RenderViewImpl.
On 2012/02/08 17:25:04, piman wrote: > If it's really a singleton, shouldn't it hang off the RenderThreadImpl then, > instead of the RenderViewImpl? > > Antoine > Sorry about the confusing choice of words, when I wrote singleton I meant it in the context of a frame/webkit instance. In short there will be 0 or 1 MediaStreamCenters per frame.
On Thu, Feb 9, 2012 at 5:29 AM, <tommyw@google.com> wrote: > On 2012/02/08 17:25:04, piman wrote: > >> If it's really a singleton, shouldn't it hang off the RenderThreadImpl >> then, >> instead of the RenderViewImpl? >> > > Antoine >> > > > Sorry about the confusing choice of words, when I wrote singleton I meant > it in > the context of a frame/webkit instance. Those are different things. You have 1 webkit instance per renderer process, but it services multiple views (windows), and within that multiple frames (pages, iframes, ...). You also have 1 RenderThreadImpl per process, one WebViewImpl / RenderViewImpl per view. Multiple frames (WebFrameImpl in webkit) can share the same RenderViewImpl (iframe case). > In short there will be 0 or 1 > MediaStreamCenters per frame. > According to http://trac.webkit.org/browser/trunk/Source/WebCore/platform/mediastream/Medi... a single MediaStreamCenter per process in WebKit, i.e. a singleton, and it's 1:1 with the WebMediaStreamCenter, so if MediaStreamCenter is really meant to be a singleton, it can't be linked to the WebViewImpl, because otherwise you will end up with one view that accesses the WebMediaStreamCenter from another view which could then get destroyed. Anyway, if it's a singleton it needs to be handled by the RenderThreadImpl. Most of these things are generally per-frame or per-view instances, singletons are rare. If they are either, it should go through the WebFrameClient or WebViewClient respectively. See e.g. WebFrameClient::createMediaPlayer or WebViewClient::createGraphicsContext3D. RenderViewImpl is both a WebViewClient and a WebFrameClient. Except for very few exceptions, the chrome side shouldn't deal with JavaScript at all, which is what WebFrame::frameForCurrentContext does. > https://chromiumcodereview.**appspot.com/9309078/<https://chromiumcodereview.... >
Adding Darin to the discussion since he reviewed the WebKit side code. Thanks for the very detailed comment, I learnt a lot. Normally I only code in WebKit but since I already needed this code for testing my WebKit patch I thought I would get it commited as well. Some parts of the WebRTC code base goes indeed through a page client, but recently WebKit has started to split code into a platform interface and MediaStreamCenter goes through that instead.
https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/me... File content/renderer/media/media_stream_center.h (right): https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/me... content/renderer/media/media_stream_center.h:16: I think this class should be in the content namespace since it is part of src/content. Note: we have not fully converted everything over to the content namespace, but are instead doing it gradually. https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... File content/renderer/render_view_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... content/renderer/render_view_impl.cc:604: WebKit::WebMediaStreamCenter* RenderViewImpl::CreateMediaStreamCenter( In this file, please add using directives at the top to avoid the WebKit:: prefixing. https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... content/renderer/renderer_webkitplatformsupport_impl.cc:674: WebKit::WebMediaStreamCenter* nit: in this file, please add using directives to avoid the WebKit:: prefixing.
Darin, I will of course fix all your comments but can you tell me what the best way is to hook this into Chromium? piman had some good comments above that you might want to look at. On 2012/02/13 16:27:01, darin wrote: > https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/me... > File content/renderer/media/media_stream_center.h (right): > > https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/me... > content/renderer/media/media_stream_center.h:16: > I think this class should be in the content namespace since it is part of > src/content. Note: we have not fully converted everything over to the content > namespace, but are instead doing it gradually. > > https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... > File content/renderer/render_view_impl.cc (right): > > https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... > content/renderer/render_view_impl.cc:604: WebKit::WebMediaStreamCenter* > RenderViewImpl::CreateMediaStreamCenter( > In this file, please add using directives at the top to avoid the WebKit:: > prefixing. > > https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... > File content/renderer/renderer_webkitplatformsupport_impl.cc (right): > > https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... > content/renderer/renderer_webkitplatformsupport_impl.cc:674: > WebKit::WebMediaStreamCenter* > nit: in this file, please add using directives to avoid the WebKit:: prefixing.
https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/me... File content/renderer/media/media_stream_center.h (right): https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/me... content/renderer/media/media_stream_center.h:16: On 2012/02/13 16:27:01, darin wrote: > I think this class should be in the content namespace since it is part of > src/content. Note: we have not fully converted everything over to the content > namespace, but are instead doing it gradually. Done. https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... File content/renderer/render_view_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... content/renderer/render_view_impl.cc:604: WebKit::WebMediaStreamCenter* RenderViewImpl::CreateMediaStreamCenter( On 2012/02/13 16:27:01, darin wrote: > In this file, please add using directives at the top to avoid the WebKit:: > prefixing. Done. https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/16001/content/renderer/re... content/renderer/renderer_webkitplatformsupport_impl.cc:674: WebKit::WebMediaStreamCenter* On 2012/02/13 16:27:01, darin wrote: > nit: in this file, please add using directives to avoid the WebKit:: prefixing. Done.
https://chromiumcodereview.appspot.com/9309078/diff/24001/content/renderer/re... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://chromiumcodereview.appspot.com/9309078/diff/24001/content/renderer/re... content/renderer/renderer_webkitplatformsupport_impl.cc:699: RenderViewImpl* render_view = findRenderView(); yeah, what piman said. if you need the interface to be RenderView specific, then this create function needs to live on Web{View,Frame}Client instead.
On 2012/02/13 17:44:30, darin wrote: > https://chromiumcodereview.appspot.com/9309078/diff/24001/content/renderer/re... > File content/renderer/renderer_webkitplatformsupport_impl.cc (right): > > https://chromiumcodereview.appspot.com/9309078/diff/24001/content/renderer/re... > content/renderer/renderer_webkitplatformsupport_impl.cc:699: RenderViewImpl* > render_view = findRenderView(); > yeah, what piman said. if you need the interface to be RenderView specific, > then this create function needs to live on Web{View,Frame}Client instead. To turn to discussion around, how would you recommend this to be implemented considering that the WebCore MediaStreamCenter is a singleton and lives in the platform layer? The only requierement is that we somehow dynamically can find all RenderViewImpls in the same process so that we can find the correct one (ie which one "owns" the relevant MediaStreamDescriptor). /Tommy
On Tue, Feb 14, 2012 at 2:16 AM, <tommyw@google.com> wrote: > On 2012/02/13 17:44:30, darin wrote: > > https://chromiumcodereview.**appspot.com/9309078/diff/** > 24001/content/renderer/**renderer_**webkitplatformsupport_impl.cc<https://chromiumcodereview.appspot.com/9309078/diff/24001/content/renderer/renderer_webkitplatformsupport_impl.cc> > >> File content/renderer/renderer_**webkitplatformsupport_impl.cc (right): >> > > > https://chromiumcodereview.**appspot.com/9309078/diff/** > 24001/content/renderer/**renderer_**webkitplatformsupport_impl.cc#** > newcode699<https://chromiumcodereview.appspot.com/9309078/diff/24001/content/renderer/renderer_webkitplatformsupport_impl.cc#newcode699> > >> content/renderer/renderer_**webkitplatformsupport_impl.cc:**699: >> RenderViewImpl* >> render_view = findRenderView(); >> yeah, what piman said. if you need the interface to be RenderView >> specific, >> then this create function needs to live on Web{View,Frame}Client instead. >> > > To turn to discussion around, how would you recommend this to be > implemented > considering that the WebCore MediaStreamCenter is a singleton and lives in > the > platform layer? > Wait, if MediaStreamCenter is a singleton, then why isn't WebMediaStreamCenter also a singleton? The bit of information that I'm missing is, why does WebMediaStreamCenter need to depend on RenderView? > > The only requierement is that we somehow dynamically can find all > RenderViewImpls in the same process so that we can find the correct one (ie > which one "owns" the relevant MediaStreamDescriptor). > So, you need a way to map a WebMediaStreamDescriptor to a RenderView? Are MediaStreamDescriptors created in the context of a Frame? If so, then you might consider adding a FrameLoaderClient method that informs the upper layer when a MediaStreamDescriptor is created for the Frame. Then, give {Web}MediaStreamDescriptor a way to store arbitrary opaque user data. Let the embedder tag the WMSD with the routing_id of the corresponding RenderView. -Darin > > /Tommy > > > > > https://chromiumcodereview.**appspot.com/9309078/<https://chromiumcodereview.... >
A classic case that code reviews over 9 time zones and 5390 miles can be tricky
;)
Here's a use case of the existing WebCore/WebKit code where the
MediaStreamCenter isn't created yet:
(1) Somehow the enabled attribute is changed on a MediaStreamTrack
WebCode/mediastream/MediaStreamTrack.cpp
(2) The MediaStreamTrack implementation requests the MediaStreamCenter instance
which is created now.
Source/WebCore/platform/mediastream/MediaStreamCenter.h
Source/WebCore/platform/mediastream/MediaStreamCenter.cpp (platform
independent)
Source/WebKit/chromium/bridge/MediaStreamCenter.cpp (Chromium)
(3) The MediaStreamCenter instance creates its "bridge" class
MediaStreamCenterInternal
Source/WebKit/chromium/bridge/MediaStreamCenterInternal.h
Source/WebKit/chromium/bridge/MediaStreamCenterInternal.cpp
(4) MediaStreamCenterInternal creates its proper WebKit instance
WebMediaStreamCenter through
WebKit::webKitPlatformSupport()->createMediaStreamCenter(this)
Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h
(5) The MediaStreamTrack implementation, see (2), calls
didSetMediaStreamTrackEnabled on the MediaStreamCenter instance, which forwards
it to the MediaStreamCenterInternal instance which calls one of
didEnableMediaStreamTrack or didDisableMediaStreamTrack on the
WebMediaStreamCenter instance. The associated, if any, MediaStreamDescriptor is
sent along.
Now the patch needs to do these three tasks:
(4a) Create and store the singleton-ish WebMediaStreamCenter-based class
somewhere appropriate.
(5a) When didEnableMediaStreamTrack or didDisableMediaStreamTrack is called on
the above instance, somehow iterate over all MediaStreamImpl instances that
hangs of a RenderViewImpl to find the owner of the
{Web}MediaStreamDescriptor/{Web}MediaStreamComponent. This might be best
implemented as you suggested by storing the routing_id in the WMSD.
(Unrelated to the above use case) When a MediaStreamImpl instance need to tell
WebCore that a Track has died, it calls stopLocalMediaStream on the
WebMediaStreamCenterClient instance passed in the createMediaStreamCenter call
in (4).
MediaStreamCenterInternal implements the WebMediaStreamCenterClient interface,
and sends up the call through MediaStreamCenterInternal -> MediaStreamCenter ->
MediaStreamDescriptor.
Source/WebKit/chromium/public/platform/WebMediaStreamCenterClient.h
I hope that I have managed to give a better picture of the current and desired
future implementation. If not I think we better chat IRL.
FYI I am handing this patch over to a colleague with actual Chrome knowledge. |
