Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
83023005:
Add VideoTrackSink interface to content/public (Closed)
|
Created:
7 years, 1 month ago by perkj_chrome Modified:
6 years, 11 months ago Reviewers:
tommi (sloooow) - chröme, Jói, Alpha Left Google, jam, no longer working on chromium, wuchengli CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a new VideoTrackSink interface.
It expose an interface to content/public that can be used for receiving video frames from a VideoTrack.
As a proof of concept this cl change the existing RtcVideoRenderer to implement this VideoTrackSink interface. This use of VideoTrackSink shows that an implementation can get video frames from both local and remote video tracks.
The actual VideoTrack implementation still use the existing webrtc interface based tracks.
This is a Piranha Plant cl where the end goal is to minimize the use of libjingle interfaces in the MediaStream implementation.
Design doc: https://goto.google.com/qrbzy
BUG=323223
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238694
Patch Set 1 #Patch Set 2 : #
Total comments: 43
Patch Set 3 : Renamed files and addressed comments. #
Total comments: 4
Patch Set 4 : Added AddToVideoTrack #
Total comments: 31
Patch Set 5 : Addressed Tommis comments. #
Total comments: 17
Patch Set 6 : Added OWNERS file and addressed comments. #
Total comments: 12
Patch Set 7 : Addressed jams comments. Discussion about hclams comments. #
Total comments: 2
Patch Set 8 : fix line lenght #Patch Set 9 : Rebased #Patch Set 10 : Fix merge error #Messages
Total messages: 47 (0 generated)
Hej Just wanted to upload this to show that I have at least started thinking. Does this fit approximately with your thinking?
On 2013/11/22 14:13:51, perkj wrote: > Hej > Just wanted to upload this to show that I have at least started thinking. > > Does this fit approximately with your thinking? I just made this cl more interesting by making sure that the existing video renderer use this interface and have written an adapter from a webrt VideoRendererInterface.
Great work, Per, thanks for the pioneer work. Some initial comments from me. SX https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/media_track_sink.h (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/media_track_sink.h:28: virtual void OnSourceChangedState(ReadyState state) {}; Do the sink need to know the source? or only track? https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.cc:38: default: nit, you don't need default case if you have already covered all the cases. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.h (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.h:34: void Register(const blink::WebMediaStreamTrack& track); Can AddSink, RemoveSink better names than Register and UnRegister ? https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... File content/renderer/media/video_track.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... content/renderer/media/video_track.cc:20: void VideoTrack::RegisterSink(VideoTrackSink* sink) { add thread check to make sure the methods are thread safe. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... content/renderer/media/video_track.cc:26: for (ScopedVector<WebRtcVideoSinkAdapter>::iterator it = sinks_.begin(); you can use std::find_if if you wrap the comparison code into a static method of WebRtcVideoSinkAdapter https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... File content/renderer/media/video_track.h (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... content/renderer/media/video_track.h:29: VideoTrack(webrtc::VideoTrackInterface* track); nit, explicit https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... content/renderer/media/video_track.h:37: ScopedVector<WebRtcVideoSinkAdapter> sinks_; so we are creating one WebRtcVideoSinkAdapter for each sink, I thought we would only do this for the sink connected to libjingle peer connection. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... File content/renderer/media/webrtc_videosink_adapter.h (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.h:5: #ifndef CONTENT_RENDERER_MEDIA_WEBRTC_VIDEOSINK_ADAPTER_H_ videosink is not one word https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.h:45: scoped_refptr<webrtc::VideoTrackInterface> video_track_; curiously, the video track is owning an array of adapters, and the adapter is keeping an reference of the track, isn't this circular reference?
Very cool! First few comments, I need to take a closer look at VideoTrack and VideoTrackAdapter. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/media_track_sink.h (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/media_track_sink.h:17: class CONTENT_EXPORT MediaTrackSink { We might want to name this just MediaSink (and VideoSink/AudioSink) since we'll probably want the same interface e.g. on a track for a source to push to, and evne on a source for e.g. a microphone to push to. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/media_track_sink.h:20: // TODO(perkj): Can we use blink::WebMediaStreamSource::ReadyState somehow We can include blink types in content/public/ (see e.g. content/public/renderer/render_view.h which uses blink::NavigationPolicy and others), so this shouldn't be a problem. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/media_track_sink.h:30: ReadyState GetSourceState(const blink::WebMediaStreamTrack& track) const; Once you create the sink, it presumably could store a non-owning pointer to its track, so you shouldn't have to pass the track into this method. This also makes the interface less specific to a track. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.h (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.h:34: void Register(const blink::WebMediaStreamTrack& track); Registration and unregistration should be on the VideoTrack object itself. VideoTrack could have a FromBlinkTrack static method, then instead of calling this->Register(blink_track) on itself, a sink would call FromBlinkTrack(blink_track)->Register(this);
Joi, should we create a content/public/renderer/media folder? or we should just put the code in content/public/renderer ?
https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... File content/renderer/media/webrtc_videosink_adapter.h (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.h:19: class CONTENT_EXPORT WebRtcVideoSinkAdapter Needs documentation of role/responsibilities.
It should go straight in content/public/renderer. The Content API uses no subfolders below content/public/xyz where xyz is the process name or "common". On Mon, Nov 25, 2013 at 3:49 PM, <xians@chromium.org> wrote: > > Joi, should we create a content/public/renderer/media folder? or we should > just > put the code in content/public/renderer ? > > https://codereview.chromium.org/83023005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.cc:8: #include "content/renderer/media/video_track.h" content/public cannot include content/renderer so this is not possible. The Register() and UnRegister() methods below should be some static methods implemented in content/renderer/media. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.h (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.h:34: void Register(const blink::WebMediaStreamTrack& track); On 2013/11/25 14:06:55, Jói wrote: > Registration and unregistration should be on the VideoTrack object itself. > > VideoTrack could have a FromBlinkTrack static method, then instead of calling > this->Register(blink_track) on itself, a sink would call > FromBlinkTrack(blink_track)->Register(this); I'm not sure this is possible unless VideoTrack is also in content/public/renderer. It looks like VideoTrack is the implementation of a video track which shouldn't be exposed to content/public. I think these can just be some static methods like this: namespace content { class VideoTrackSink : public MediaTrackSink { public: virtual void OnVideoFrame(...) = 0; }; // static void AddSinkToVideoTrack(VideoTrackSink*, const blink::WebMediaStreamTrack& track); void RemoveSinkFromVideoTrack(const blink::WebMediaStreamTrack& track); } // namespace content https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... File content/renderer/media/rtc_video_renderer.cc (left): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... content/renderer/media/rtc_video_renderer.cc:80: TRACE_EVENT_INSTANT2("rtc_video_renderer", Please keep this event in some form. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... content/renderer/media/rtc_video_renderer.cc:144: TRACE_EVENT0("video", "DoRenderFrameOnMainThread"); Please keep this event. We depend on it to for some testing. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... File content/renderer/media/rtc_video_renderer.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... content/renderer/media/rtc_video_renderer.cc:75: repaint_cb_.Run(frame); Great! Love this! https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... File content/renderer/media/webrtc_videosink_adapter.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.cc:30: video_track_->AddRenderer(this); Oh boy. Please tell me that this is temporary. The implementation seems to be adding the sink to libjingle's implementation of VideoTrackInterface. I thought we're having less dependencies on libjingle? We already have a VideoCaptureImpl in Chrome that does multiplexing so attaching the sink there would be the best, isn't it?
PTAL https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/media_track_sink.h (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/media_track_sink.h:17: class CONTENT_EXPORT MediaTrackSink { On 2013/11/25 14:06:55, Jói wrote: > We might want to name this just MediaSink (and VideoSink/AudioSink) since we'll > probably want the same interface e.g. on a track for a source to push to, and > evne on a source for e.g. a microphone to push to. Do we need anything to say that this is related to MediaStreams in the name? I like MediaSink but it is very generic. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/media_track_sink.h:20: // TODO(perkj): Can we use blink::WebMediaStreamSource::ReadyState somehow On 2013/11/25 14:06:55, Jói wrote: > We can include blink types in content/public/ (see e.g. > content/public/renderer/render_view.h which uses blink::NavigationPolicy and > others), so this shouldn't be a problem. Done. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/media_track_sink.h:28: virtual void OnSourceChangedState(ReadyState state) {}; On 2013/11/25 13:55:07, xians1 wrote: > Do the sink need to know the source? or only track? It will problably (as in this cl) need to know the readyState. In blink- this is implemented on the source. We can call this ReadyStateChanged. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/media_track_sink.h:30: ReadyState GetSourceState(const blink::WebMediaStreamTrack& track) const; On 2013/11/25 14:06:55, Jói wrote: > Once you create the sink, it presumably could store a non-owning pointer to its > track, so you shouldn't have to pass the track into this method. > > This also makes the interface less specific to a track. We don't need this if we can use the blink readyState. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.cc:8: #include "content/renderer/media/video_track.h" On 2013/11/26 04:59:32, Alpha wrote: > content/public cannot include content/renderer so this is not possible. > > The Register() and UnRegister() methods below should be some static methods > implemented in content/renderer/media. is that true? See content/public/renderer/render_video_observer.cc https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.cc:38: default: On 2013/11/25 13:55:07, xians1 wrote: > nit, you don't need default case if you have already covered all the cases. These are removed. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.h (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.h:34: void Register(const blink::WebMediaStreamTrack& track); On 2013/11/25 13:55:07, xians1 wrote: > Can AddSink, RemoveSink better names than Register and UnRegister ? Done. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.h:34: void Register(const blink::WebMediaStreamTrack& track); On 2013/11/26 04:59:32, Alpha wrote: > On 2013/11/25 14:06:55, Jói wrote: > > Registration and unregistration should be on the VideoTrack object itself. > > > > VideoTrack could have a FromBlinkTrack static method, then instead of calling > > this->Register(blink_track) on itself, a sink would call > > FromBlinkTrack(blink_track)->Register(this); > > I'm not sure this is possible unless VideoTrack is also in > content/public/renderer. It looks like VideoTrack is the implementation of a > video track which shouldn't be exposed to content/public. I think these can just > be some static methods like this: > > namespace content { > > class VideoTrackSink : public MediaTrackSink { > public: > virtual void OnVideoFrame(...) = 0; > }; > > // static > void AddSinkToVideoTrack(VideoTrackSink*, const blink::WebMediaStreamTrack& > track); > void RemoveSinkFromVideoTrack(const blink::WebMediaStreamTrack& track); > > } // namespace content Joi? I use static functions at the moment. It seems like the static methods must belong to the class. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... File content/renderer/media/rtc_video_renderer.cc (left): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... content/renderer/media/rtc_video_renderer.cc:144: TRACE_EVENT0("video", "DoRenderFrameOnMainThread"); On 2013/11/26 04:59:32, Alpha wrote: > Please keep this event. We depend on it to for some testing. This does seem to make sense with this change. Can the test be changed to use the trace in OnVideoFrame ? https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... File content/renderer/media/video_track.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... content/renderer/media/video_track.cc:20: void VideoTrack::RegisterSink(VideoTrackSink* sink) { On 2013/11/25 13:55:07, xians1 wrote: > add thread check to make sure the methods are thread safe. Done. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... content/renderer/media/video_track.cc:26: for (ScopedVector<WebRtcVideoSinkAdapter>::iterator it = sinks_.begin(); On 2013/11/25 13:55:07, xians1 wrote: > you can use std::find_if if you wrap the comparison code into a static method of > WebRtcVideoSinkAdapter Done. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... File content/renderer/media/video_track.h (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... content/renderer/media/video_track.h:29: VideoTrack(webrtc::VideoTrackInterface* track); On 2013/11/25 13:55:07, xians1 wrote: > nit, explicit Done. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/vi... content/renderer/media/video_track.h:37: ScopedVector<WebRtcVideoSinkAdapter> sinks_; On 2013/11/25 13:55:07, xians1 wrote: > so we are creating one WebRtcVideoSinkAdapter for each sink, I thought we would > only do this for the sink connected to libjingle peer connection. Yes- that is the end goal. But that requires huge changes to this is an intermediate step. The end goal is to only use this adapter on remote tracks. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... File content/renderer/media/webrtc_videosink_adapter.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.cc:30: video_track_->AddRenderer(this); On 2013/11/26 04:59:32, Alpha wrote: > Oh boy. Please tell me that this is temporary. The implementation seems to be > adding the sink to libjingle's implementation of VideoTrackInterface. I thought > we're having less dependencies on libjingle? > > We already have a VideoCaptureImpl in Chrome that does multiplexing so attaching > the sink there would be the best, isn't it? > Yes- this is the first step. We will need this for remote video tracks in the end but not for local video tracks. If we can use VideoCaptureImpl for multiplexing on all levels remains to be seen. We still need a track implementation that controls enable/disable and a source implementation to use for cloning. Ie- MediaStreams have multiple levels of multiplexing video data. You can create several track using the same source from JS. (cloning of tracks) You can add multiple sinks to a video track. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... File content/renderer/media/webrtc_videosink_adapter.h (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.h:5: #ifndef CONTENT_RENDERER_MEDIA_WEBRTC_VIDEOSINK_ADAPTER_H_ On 2013/11/25 13:55:07, xians1 wrote: > videosink is not one word Done. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.h:19: class CONTENT_EXPORT WebRtcVideoSinkAdapter On 2013/11/25 15:50:57, Jói wrote: > Needs documentation of role/responsibilities. Done. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.h:45: scoped_refptr<webrtc::VideoTrackInterface> video_track_; On 2013/11/25 13:55:07, xians1 wrote: > curiously, the video track is owning an array of adapters, and the adapter is > keeping an reference of the track, isn't this circular reference? This is the libjingle repressentation of a video track. The VideoTrack is not a libjingle track.
https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.cc:8: #include "content/renderer/media/video_track.h" On 2013/11/26 09:16:38, perkj wrote: > On 2013/11/26 04:59:32, Alpha wrote: > > content/public cannot include content/renderer so this is not possible. > > > > The Register() and UnRegister() methods below should be some static methods > > implemented in content/renderer/media. > > is that true? See content/public/renderer/render_video_observer.cc I cannot locate the file you mentioned. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... File content/renderer/media/rtc_video_renderer.cc (left): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... content/renderer/media/rtc_video_renderer.cc:80: TRACE_EVENT_INSTANT2("rtc_video_renderer", On 2013/11/26 04:59:32, Alpha wrote: > Please keep this event in some form. Ignore this and just remove the events please. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/rt... content/renderer/media/rtc_video_renderer.cc:144: TRACE_EVENT0("video", "DoRenderFrameOnMainThread"); On 2013/11/26 09:16:38, perkj wrote: > On 2013/11/26 04:59:32, Alpha wrote: > > Please keep this event. We depend on it to for some testing. > > This does seem to make sense with this change. Can the test be changed to use > the trace in OnVideoFrame ? Okay. Just remove the trace then, we'll handle that separately. https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... File content/renderer/media/webrtc_videosink_adapter.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/renderer/media/we... content/renderer/media/webrtc_videosink_adapter.cc:30: video_track_->AddRenderer(this); On 2013/11/26 09:16:38, perkj wrote: > On 2013/11/26 04:59:32, Alpha wrote: > > Oh boy. Please tell me that this is temporary. The implementation seems to be > > adding the sink to libjingle's implementation of VideoTrackInterface. I > thought > > we're having less dependencies on libjingle? > > > > We already have a VideoCaptureImpl in Chrome that does multiplexing so > attaching > > the sink there would be the best, isn't it? > > > > Yes- this is the first step. We will need this for remote video tracks in the > end but not for local video tracks. > If we can use VideoCaptureImpl for multiplexing on all levels remains to be > seen. We still need a track implementation that controls enable/disable and a > source implementation to use for cloning. Ie- MediaStreams have multiple levels > of multiplexing video data. > > You can create several track using the same source from JS. (cloning of tracks) > You can add multiple sinks to a video track. Sounds good for this being the first step. As long as we're depending less on libjingle I'm happy. :) That's true. But then for video there's no modification or postprocessing of video frame isn't it? So essentially the video frame from tracks and sources are the same, no? I'm just thing about the performance cost of multiplexing at multiple layers for video data.
Re multiplexing performance cost, it seems tiny to me since there is no copying, you're just passing around a pointer to a VideoFrame. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.cc (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.cc:8: #include "content/renderer/media/video_track.h" On 2013/11/26 10:00:47, Alpha wrote: > On 2013/11/26 09:16:38, perkj wrote: > > On 2013/11/26 04:59:32, Alpha wrote: > > > content/public cannot include content/renderer so this is not possible. > > > > > > The Register() and UnRegister() methods below should be some static methods > > > implemented in content/renderer/media. > > > > is that true? See content/public/renderer/render_video_observer.cc > > I cannot locate the file you mentioned. A .cc file in content/public/renderer can include files from content/renderer, but .h files cannot. https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... File content/public/renderer/media/video_track_sink.h (right): https://codereview.chromium.org/83023005/diff/90001/content/public/renderer/m... content/public/renderer/media/video_track_sink.h:34: void Register(const blink::WebMediaStreamTrack& track); On 2013/11/26 09:16:38, perkj wrote: > On 2013/11/26 04:59:32, Alpha wrote: > > On 2013/11/25 14:06:55, Jói wrote: > > > Registration and unregistration should be on the VideoTrack object itself. > > > > > > VideoTrack could have a FromBlinkTrack static method, then instead of > calling > > > this->Register(blink_track) on itself, a sink would call > > > FromBlinkTrack(blink_track)->Register(this); > > > > I'm not sure this is possible unless VideoTrack is also in > > content/public/renderer. It looks like VideoTrack is the implementation of a > > video track which shouldn't be exposed to content/public. I think these can > just > > be some static methods like this: > > > > namespace content { > > > > class VideoTrackSink : public MediaTrackSink { > > public: > > virtual void OnVideoFrame(...) = 0; > > }; > > > > // static > > void AddSinkToVideoTrack(VideoTrackSink*, const blink::WebMediaStreamTrack& > > track); > > void RemoveSinkFromVideoTrack(const blink::WebMediaStreamTrack& track); > > > > } // namespace content > > Joi? I use static functions at the moment. It seems like the static methods must > belong to the class. Yeah, makes sense to have static functions on VideoTrackSink to register/unregister given a WebMediaStreamTrack.
On 2013/11/26 10:18:15, Jói wrote: > Re multiplexing performance cost, it seems tiny to me since there is no copying, > you're just passing around a pointer to a VideoFrame. > Okay. As long as multiplexing doesn't involve copying of video data. :)
Hi guys, the comment is for all of you, please speak out if you think it is a bad or good idea. https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:23: class CONTENT_EXPORT MediaStreamVideoSink : public MediaStreamSink { Just an idea. the sink interfaces for audio and video are small and simple, I am wondering if we should put them into one file. like: class CONTENT_EXPORT MediaStreamSink { class VideoSink { public: virtual void OnVideoFrame(const scoped_refptr<media::VideoFrame>& frame) = 0; protected: virtual ~VideoSink() {} } class AudioSink { public: virtual void OnAudioData(const int16* data, int sample_rate, int channels, int frames) = 0; virtual void OnSetFormat(const media::AudioParameters& params) = 0; protected: virtual ~AudioSink() {} } } CONTENT_EXPORT void AddToAudioTrack( MediaStreamSink::AudioSink* sink, const blink::WebMediaStreamTrack& track); CONTENT_EXPORT void RemoveFromAudioTrack( MediaStreamSink::AudioSink* sink, const blink::WebMediaStreamTrack& track); CONTENT_EXPORT void AddToVideoTrack( MediaStreamSink::VideoSink* sink, const blink::WebMediaStreamTrack& track); CONTENT_EXPORT void RemoveFromVideoTrack( MediaStreamSink::VideoSink* sink, const blink::WebMediaStreamTrack& track); https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:30: static void AddToTrack(MediaStreamVideoSink* sink, these two interface should not be in the MediaStreamVideoSink. Extract them out and use CONTENT_EXPORT.
Hej What do you think is the next step? Review this and get it landed? Do we need other reviewers? https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:23: class CONTENT_EXPORT MediaStreamVideoSink : public MediaStreamSink { On 2013/11/27 10:12:42, xians1 wrote: > Just an idea. > the sink interfaces for audio and video are small and simple, I am wondering if > we should put them into one file. like: > class CONTENT_EXPORT MediaStreamSink { > class VideoSink { > public: > virtual void OnVideoFrame(const scoped_refptr<media::VideoFrame>& frame) = > 0; > protected: > virtual ~VideoSink() {} > } > > class AudioSink { > public: > virtual void OnAudioData(const int16* data, int sample_rate, int channels, > int frames) = 0; > virtual void OnSetFormat(const media::AudioParameters& params) = 0; > protected: > virtual ~AudioSink() {} > } > } > > CONTENT_EXPORT void AddToAudioTrack( > MediaStreamSink::AudioSink* sink, const blink::WebMediaStreamTrack& track); > > CONTENT_EXPORT void RemoveFromAudioTrack( > MediaStreamSink::AudioSink* sink, const blink::WebMediaStreamTrack& track); > > CONTENT_EXPORT void AddToVideoTrack( > MediaStreamSink::VideoSink* sink, const blink::WebMediaStreamTrack& track); > > CONTENT_EXPORT void RemoveFromVideoTrack( > MediaStreamSink::VideoSink* sink, const blink::WebMediaStreamTrack& track); Discussed off line. Added AddToVideoTrack and RemoveFromVideoTrack and removed the statics. https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:30: static void AddToTrack(MediaStreamVideoSink* sink, On 2013/11/27 10:12:42, xians1 wrote: > these two interface should not be in the MediaStreamVideoSink. Extract them out > and use CONTENT_EXPORT. Done.
Review and get landed I think. I will take a look at the latest patch set as soon as I can. Perhaps Tommi should review, and he can suggest other reviewers if appropriate. We will need jam@ to review the content/public change. I'm an OWNER there but he wants to review any new interfaces. Cheers, Jói On Wed, Nov 27, 2013 at 3:29 PM, <perkj@chromium.org> wrote: > Hej > What do you think is the next step? Review this and get it landed? Do we > need > other reviewers? > > > > https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... > File content/public/renderer/media_stream_video_sink.h (right): > > https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... > content/public/renderer/media_stream_video_sink.h:23: class > CONTENT_EXPORT MediaStreamVideoSink : public MediaStreamSink { > On 2013/11/27 10:12:42, xians1 wrote: >> >> Just an idea. >> the sink interfaces for audio and video are small and simple, I am > > wondering if >> >> we should put them into one file. like: >> class CONTENT_EXPORT MediaStreamSink { >> class VideoSink { >> public: >> virtual void OnVideoFrame(const scoped_refptr<media::VideoFrame>& > > frame) = >> >> 0; >> protected: >> virtual ~VideoSink() {} >> } > > >> class AudioSink { >> public: >> virtual void OnAudioData(const int16* data, int sample_rate, int > > channels, >> >> int frames) = 0; >> virtual void OnSetFormat(const media::AudioParameters& params) = > > 0; >> >> protected: >> virtual ~AudioSink() {} >> } >> } > > >> CONTENT_EXPORT void AddToAudioTrack( >> MediaStreamSink::AudioSink* sink, const > > blink::WebMediaStreamTrack& track); > >> CONTENT_EXPORT void RemoveFromAudioTrack( >> MediaStreamSink::AudioSink* sink, const > > blink::WebMediaStreamTrack& track); > >> CONTENT_EXPORT void AddToVideoTrack( >> MediaStreamSink::VideoSink* sink, const > > blink::WebMediaStreamTrack& track); > >> CONTENT_EXPORT void RemoveFromVideoTrack( >> MediaStreamSink::VideoSink* sink, const > > blink::WebMediaStreamTrack& track); > > Discussed off line. > > Added AddToVideoTrack and RemoveFromVideoTrack and removed the statics. > > > https://codereview.chromium.org/83023005/diff/130001/content/public/renderer/... > content/public/renderer/media_stream_video_sink.h:30: static void > AddToTrack(MediaStreamVideoSink* sink, > On 2013/11/27 10:12:42, xians1 wrote: >> >> these two interface should not be in the MediaStreamVideoSink. Extract > > them out >> >> and use CONTENT_EXPORT. > > > Done. > > https://codereview.chromium.org/83023005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Adding Tommi as suggested.
https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_sink.h (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_sink.h:14: class CONTENT_EXPORT MediaStreamSink { t'would be nice to have some comments https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_sink.h:17: blink::WebMediaStreamSource::ReadyState state) {} = 0? since there's only one method in the interface currently, it seems like nobody would want to inherit from it without overriding this function. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.cc (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.cc:15: DCHECK(track.source().type() == blink::WebMediaStreamSource::TypeVideo); DCHECK_EQ? https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.cc:17: static_cast <MediaStreamVideoTrack*>(track.extraData()); no space after static_cast https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.cc:24: static_cast <MediaStreamVideoTrack*>(track.extraData()); ditto https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... File content/renderer/media/media_stream_dependency_factory.cc (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_dependency_factory.cc:921: static_cast<webrtc::VideoTrackInterface*> (native_track))); no space after > https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_video_track.cc:16: const WebRtcVideoSinkAdapter* owner) { is wrapping necessary? (I mean, I know the class name is SinkWrapper but...) https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_video_track.cc:43: NOTREACHED(); NOTREACHED is considered dead code and works out to be a DCHECK. Is the intention here to do: DCHECK_NE(it, sinks_.end()); ? If so, then the if() shouldn't be necessary since this isn't a supported/expected scenario. If not, then NOTREACHED doesn't feel like the right thing to do since we will certainly reach this code and maybe we should log an error instead. WDYT? https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.h (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_video_track.h:23: class CONTENT_EXPORT MediaStreamVideoTrack : public MediaStreamTrackExtraData { comments about how this class is used and what it does? https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/w... File content/renderer/media/webrtc_video_sink_adapter.h (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/w... content/renderer/media/webrtc_video_sink_adapter.h:20: // implementation of a MediaStream VideoTrack and a MediaStreamVideoTrack? (no space) https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/w... content/renderer/media/webrtc_video_sink_adapter.h:23: // to chrome video types. Would it make sense to have the webrtc implementations (those that include from libjingle), in a separate directory so that we can keep track of where those includes are and even disallow them in other folders?
https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_sink.h (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_sink.h:17: blink::WebMediaStreamSource::ReadyState state) {} On 2013/11/27 16:53:07, tommi wrote: > = 0? > since there's only one method in the interface currently, it seems like nobody > would want to inherit from it without overriding this function. The interfaces MediaStreamVideoSink and MediaStreamAudioSink which inherit from this one have more methods and somebody implementing them might not want to implement OnReadyStateChanged. https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/w... File content/renderer/media/webrtc_video_sink_adapter.h (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/w... content/renderer/media/webrtc_video_sink_adapter.h:23: // to chrome video types. On 2013/11/27 16:53:07, tommi wrote: > Would it make sense to have the webrtc implementations (those that include from > libjingle), in a separate directory so that we can keep track of where those > includes are and even disallow them in other folders? Good idea, we should do that.
I agree with joi@ that we should try to land this as we can start writing code to use this API. I agree with tommi@ that we should isolate webrtc implementation into a separate directory. I have on concern about threading as it is not clear which thread should the new AddToVideoTrack() and OnVideoFrame() be called. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:23: class CONTENT_EXPORT MediaStreamVideoSink : public MediaStreamSink { I think we should document the threading requirement of this API. For example on which thread will this method be called? https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:34: CONTENT_EXPORT void AddToVideoTrack(MediaStreamVideoSink* sink, We should document the threading requirement of this API. On which thread should this be called? It seems this function should be called on the main thread because of the use of blink objects.
PTAL https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_sink.h (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_sink.h:14: class CONTENT_EXPORT MediaStreamSink { On 2013/11/27 16:53:07, tommi wrote: > t'would be nice to have some comments Done. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_sink.h:17: blink::WebMediaStreamSource::ReadyState state) {} On 2013/11/27 18:35:51, Jói wrote: > On 2013/11/27 16:53:07, tommi wrote: > > = 0? > > since there's only one method in the interface currently, it seems like nobody > > would want to inherit from it without overriding this function. > > The interfaces MediaStreamVideoSink and MediaStreamAudioSink which inherit from > this one have more methods and somebody implementing them might not want to > implement OnReadyStateChanged. I agree with Joi. This is not needed by all implementations but belong to a common base class. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.cc (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.cc:15: DCHECK(track.source().type() == blink::WebMediaStreamSource::TypeVideo); On 2013/11/27 16:53:07, tommi wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.cc:17: static_cast <MediaStreamVideoTrack*>(track.extraData()); On 2013/11/27 16:53:07, tommi wrote: > no space after static_cast Done. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.cc:24: static_cast <MediaStreamVideoTrack*>(track.extraData()); On 2013/11/27 16:53:07, tommi wrote: > ditto Done. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:23: class CONTENT_EXPORT MediaStreamVideoSink : public MediaStreamSink { On 2013/11/27 22:13:15, Alpha wrote: > I think we should document the threading requirement of this API. For example on > which thread will this method be called? Done. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:34: CONTENT_EXPORT void AddToVideoTrack(MediaStreamVideoSink* sink, On 2013/11/27 22:13:15, Alpha wrote: > We should document the threading requirement of this API. On which thread should > this be called? It seems this function should be called on the main thread > because of the use of blink objects. Done. https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... File content/renderer/media/media_stream_dependency_factory.cc (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_dependency_factory.cc:921: static_cast<webrtc::VideoTrackInterface*> (native_track))); On 2013/11/27 16:53:07, tommi wrote: > no space after > Done. https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_video_track.cc:16: const WebRtcVideoSinkAdapter* owner) { On 2013/11/27 16:53:07, tommi wrote: > is wrapping necessary? (I mean, I know the class name is SinkWrapper but...) What do you mean? xians suggested this trick in order to be able to compare MediaStreamVideoSink* with WebRtcVideoSinkAdapter*. https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_video_track.cc:43: NOTREACHED(); On 2013/11/27 16:53:07, tommi wrote: > NOTREACHED is considered dead code and works out to be a DCHECK. > Is the intention here to do: > DCHECK_NE(it, sinks_.end()); > ? > > If so, then the if() shouldn't be necessary since this isn't a > supported/expected scenario. > If not, then NOTREACHED doesn't feel like the right thing to do since we will > certainly reach this code and maybe we should log an error instead. > WDYT? Done.DCHECK_NE doesn't compile though. https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.h (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_video_track.h:23: class CONTENT_EXPORT MediaStreamVideoTrack : public MediaStreamTrackExtraData { On 2013/11/27 16:53:07, tommi wrote: > comments about how this class is used and what it does? Done. https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/w... File content/renderer/media/webrtc_video_sink_adapter.h (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/w... content/renderer/media/webrtc_video_sink_adapter.h:20: // implementation of a MediaStream VideoTrack and a On 2013/11/27 16:53:07, tommi wrote: > MediaStreamVideoTrack? (no space) Done. https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/w... content/renderer/media/webrtc_video_sink_adapter.h:23: // to chrome video types. On 2013/11/27 18:35:51, Jói wrote: > On 2013/11/27 16:53:07, tommi wrote: > > Would it make sense to have the webrtc implementations (those that include > from > > libjingle), in a separate directory so that we can keep track of where those > > includes are and even disallow them in other folders? > > Good idea, we should do that. Done. I keep the webrtc file name prefix here to mean webrtc -> content adapter as opposed to content->webrtc adapter.
lgtm https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_sink.h (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_sink.h:17: blink::WebMediaStreamSource::ReadyState state) {} On 2013/11/28 13:32:45, perkj wrote: > On 2013/11/27 18:35:51, Jói wrote: > > On 2013/11/27 16:53:07, tommi wrote: > > > = 0? > > > since there's only one method in the interface currently, it seems like > nobody > > > would want to inherit from it without overriding this function. > > > > The interfaces MediaStreamVideoSink and MediaStreamAudioSink which inherit > from > > this one have more methods and somebody implementing them might not want to > > implement OnReadyStateChanged. > > I agree with Joi. This is not needed by all implementations but belong to a > common base class. > OK, sounds good. https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/83023005/diff/150001/content/renderer/media/m... content/renderer/media/media_stream_video_track.cc:16: const WebRtcVideoSinkAdapter* owner) { On 2013/11/28 13:32:45, perkj wrote: > On 2013/11/27 16:53:07, tommi wrote: > > is wrapping necessary? (I mean, I know the class name is SinkWrapper but...) > > What do you mean? xians suggested this trick in order to be able to compare > MediaStreamVideoSink* with WebRtcVideoSinkAdapter*. Sorry, unfortunate mix up of wrapping vs wrapping :) I mean: bool operator()(const WebRtcVideoSinkAdapter* owner) { and not bool operator()( const WebRtcVideoSinkAdapter* owner) { :-) https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:26: // All methods calls to this implementation will be done from the main render s/methods/method remove "to this implementation" since this is an interface. https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_sink_adapter.cc (right): https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_sink_adapter.cc:43: void WebRtcVideoSinkAdapter::SetSize(int width, int height) { NOTIMPLEMENTED();?
https://codereview.chromium.org/83023005/diff/170001/content/content_renderer... File content/content_renderer.gypi (right): https://codereview.chromium.org/83023005/diff/170001/content/content_renderer... content/content_renderer.gypi:652: 'renderer/media/webrtc/webrtc_video_sink_adapter.cc', Can you add a DEPS file for the renderer/media/webrtc directory, and disallow the includes in the rest of renderer/media? Note that in renderer/media/DEPS you can have a specific_include_rules section that allows the includes in whatever files still need them. https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... File content/public/renderer/media_stream_sink.h (right): https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... content/public/renderer/media_stream_sink.h:16: // about state changes on a blink::WebMediaStreamSource object. "on a blink::WebMediaStreamSource object." -> "on a blink::WebMediaStreamSource object or such an object underlying a blink::WebMediaStreamTrack." (?) https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.h (right): https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/m... content/renderer/media/media_stream_video_track.h:39: // Piranha plant. plant -> Plant :) https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_sink_adapter.cc (right): https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_sink_adapter.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: I don't think we use the (c) bit any more.
One nit and one concern on the thread safe, lgtm if you address them. https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/150001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:35: const blink::WebMediaStreamTrack& track); nit, add and empty line. https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_sink_adapter.cc (right): https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_sink_adapter.cc:109: sink_->OnVideoFrame(video_frame); I am a bit worrying that this will crash for the following case: In RenderFrame, it post a task DoRenderFrameOnMainThread, meanwhile, the sink is going away and calls RemoveFromTrack(), where erase the item in the vector, then the MediaStreamVideoSink gets deleted. But DoRenderFrameOnMainThread is executed later on and called sink_->OnVideoFrame(video_frame); Please make sure this won't happen.
https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/m... content/renderer/media/media_stream_video_track.cc:43: sinks_.erase(it); To be more specific on the racing concern, I think you need to have a Reset() method on WebRtcVideoSinkAdapter, before you call sinks_.erase(it), you need to call it->Reset() to set the sink_ to NULL in WebRtcVideoSinkAdapter
PTAL https://codereview.chromium.org/83023005/diff/170001/content/content_renderer... File content/content_renderer.gypi (right): https://codereview.chromium.org/83023005/diff/170001/content/content_renderer... content/content_renderer.gypi:652: 'renderer/media/webrtc/webrtc_video_sink_adapter.cc', On 2013/11/28 14:37:17, Jói wrote: > Can you add a DEPS file for the renderer/media/webrtc directory, and disallow > the includes in the rest of renderer/media? Note that in renderer/media/DEPS you > can have a specific_include_rules section that allows the includes in whatever > files still need them. Do this later as discussed since we currently have 57 files including libjingle headers. https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... File content/public/renderer/media_stream_sink.h (right): https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... content/public/renderer/media_stream_sink.h:16: // about state changes on a blink::WebMediaStreamSource object. On 2013/11/28 14:37:17, Jói wrote: > "on a blink::WebMediaStreamSource object." -> "on a blink::WebMediaStreamSource > object or such an object underlying a blink::WebMediaStreamTrack." > > (?) Done. https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:26: // All methods calls to this implementation will be done from the main render to this class will be done.... https://codereview.chromium.org/83023005/diff/170001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:26: // All methods calls to this implementation will be done from the main render On 2013/11/28 14:32:21, tommi wrote: > s/methods/method > remove "to this implementation" since this is an interface. Done. https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.cc (right): https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/m... content/renderer/media/media_stream_video_track.cc:43: sinks_.erase(it); On 2013/11/28 15:20:46, xians1 wrote: > To be more specific on the racing concern, I think you need to have a Reset() > method on WebRtcVideoSinkAdapter, before you call sinks_.erase(it), you need to > call it->Reset() to set the sink_ to NULL in WebRtcVideoSinkAdapter Why would I need that? sinks_ is a scoped_vector ? WebRtcVideoSinkAdapter is not ref counted. https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/m... File content/renderer/media/media_stream_video_track.h (right): https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/m... content/renderer/media/media_stream_video_track.h:39: // Piranha plant. On 2013/11/28 14:37:17, Jói wrote: > plant -> Plant > > :) Done. https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_sink_adapter.cc (right): https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_sink_adapter.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/11/28 14:37:17, Jói wrote: > nit: I don't think we use the (c) bit any more. Done. https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_sink_adapter.cc:43: void WebRtcVideoSinkAdapter::SetSize(int width, int height) { On 2013/11/28 14:32:21, tommi wrote: > NOTIMPLEMENTED();? Done. https://codereview.chromium.org/83023005/diff/170001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_sink_adapter.cc:109: sink_->OnVideoFrame(video_frame); On 2013/11/28 15:12:35, xians1 wrote: > I am a bit worrying that this will crash for the following case: > In RenderFrame, it post a task DoRenderFrameOnMainThread, meanwhile, the sink is > going away and calls RemoveFromTrack(), where erase the item in the vector, then > the MediaStreamVideoSink gets deleted. But DoRenderFrameOnMainThread is executed > later on and called sink_->OnVideoFrame(video_frame); > > Please make sure this won't happen. sink_| is guarandeet by the user to be valid until RemoveFromTrack has been called. RemoveFromTrack deletes WebRtcVideoSinkAdapter. WebRtcVideoSinkAdapter supports weak ptrs so DoRenderFrameOnMainThread will not be called after ~WebRtcVideoSinkAdapter.
LGTM
Per, could you please change the description of this CL? It is no longer a hack if the solution is sound.
jam, would you like to review the content/public and as much of the rest as you want? wuchengli, do you want to take a look at the split of rtc_video_renderer.cc into rtc_video_renderer and webrtc_video_sink_adapter.cc? Thanks Per
On 2013/12/02 13:39:07, perkj wrote: > jam, would you like to review the content/public and as much of the rest as you > want? > > wuchengli, do you want to take a look at the split of rtc_video_renderer.cc into > rtc_video_renderer and webrtc_video_sink_adapter.cc? > > Thanks > Per LGTM although I'm no expert on rtc_video_renderer.
few high level comments 1) per http://www.chromium.org/developers/content-module/content-api, stuff should only go in content/public if the embedder of content needs this. i don't see any use of this code from src/chrome, so it's impossible for me to know if that's because that'll come in future changes, or if it's because chrome doesn't need it 2) content\renderer\media\webrtc seems excessive. given that the webrtc code is already in the media directories, why create a subdirectory? 3) why is the design doc not public?
https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.cc (right): https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... content/public/renderer/media_stream_video_sink.cc:18: video_track->AddSink(sink); track.isNull() can be true and track.extraData() can also be NULL. See: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Since the caller doesn't guarantee adding the sink will be successful I think it's best to handle error cases here and return a boolean. https://codereview.chromium.org/83023005/diff/210001/content/renderer/media/r... File content/renderer/media/rtc_video_renderer.cc (right): https://codereview.chromium.org/83023005/diff/210001/content/renderer/media/r... content/renderer/media/rtc_video_renderer.cc:33: AddToVideoTrack(this, video_track_); I don't think you can stash a WebMediaStreamTrack object, see my comments in the header file. I think you'll need to use a URL and device ID to access the track similar to: blink::WebMediaStreamRegistry::lookupMediaStreamDescriptor(). https://codereview.chromium.org/83023005/diff/210001/content/renderer/media/r... File content/renderer/media/rtc_video_renderer.h (right): https://codereview.chromium.org/83023005/diff/210001/content/renderer/media/r... content/renderer/media/rtc_video_renderer.h:68: blink::WebMediaStreamTrack video_track_; Sorry I missed this before. I don't think it is safe to stash a WebMediaStreamTrack. This object is just a transient wrapper for a WebCore::MediaStreamComponent. It merely wraps a pointer and doesn't hold a ref to the webcore object. See the definition here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Thanks John. 1) Cast needs this, so it will be used in the //chrome layer. 2) We plan to restrict usage of libjingle to just //content/renderer/media/webrtc, therefore the directory. 3) It will be soon. It was drafted internally and received a bunch of comments before I realized there is no need for it to be internal. I plan to move it to be on chromium.org as soon as I've responded to the first round of reviews. Cheers, Jói On Mon, Dec 2, 2013 at 5:34 PM, <jam@chromium.org> wrote: > few high level comments > > 1) per http://www.chromium.org/developers/content-module/content-api, stuff > should only go in content/public if the embedder of content needs this. i > don't > see any use of this code from src/chrome, so it's impossible for me to know > if > that's because that'll come in future changes, or if it's because chrome > doesn't > need it > > 2) content\renderer\media\webrtc seems excessive. given that the webrtc > code is > already in the media directories, why create a subdirectory? > > 3) why is the design doc not public? > > https://codereview.chromium.org/83023005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/12/02 19:19:26, Jói wrote: > Thanks John. > > 1) Cast needs this, so it will be used in the //chrome layer. Do you guys have an outline for how big the public api in content will be to support cast being in chrome? I had a discussion about this in https://codereview.chromium.org/26931002/ with Alpha; in summary I worry that putting cast in chrome means that we'd have to expose a lot of the internals of content. > 2) We plan to restrict usage of libjingle to just > //content/renderer/media/webrtc, therefore the directory. I see, so will all the webrtc code in content\renderer\media move there? I defer to you guys, just seemed a bit long to type to me. > 3) It will be soon. It was drafted internally and received a bunch of > comments before I realized there is no need for it to be internal. I > plan to move it to be on http://chromium.org as soon as I've responded to the > first round of reviews. great, thanks. > > Cheers, > Jói
> > 1) Cast needs this, so it will be used in the //chrome layer. > > Do you guys have an outline for how big the public api in content will be to > support cast being in chrome? I had a discussion about this in > https://codereview.chromium.org/26931002/ with Alpha; in summary I worry that > putting cast in chrome means that we'd have to expose a lot of the internals of > content. As we discussed before. Putting Cast in Chrome we need the following new interfaces: 1. Expose p2p socket to access socket: https://codereview.chromium.org/45183002/ 2. Expose video capture interface, which is this CL. 3. Expose audio capture interface: https://codereview.chromium.org/90743004/ We don't need more interfaces to support Cast.
>> 2) We plan to restrict usage of libjingle to just >> //content/renderer/media/webrtc, therefore the directory. > > > I see, so will all the webrtc code in content\renderer\media move there? That's the idea. I think our preference is to have this as a separate directory since the DEPS and OWNERS rules will then be more natural to write than if we do it based on file patterns or the like. Cheers, Jói On Mon, Dec 2, 2013 at 7:56 PM, <jam@chromium.org> wrote: > On 2013/12/02 19:19:26, Jói wrote: >> >> Thanks John. > > >> 1) Cast needs this, so it will be used in the //chrome layer. > > > Do you guys have an outline for how big the public api in content will be to > support cast being in chrome? I had a discussion about this in > https://codereview.chromium.org/26931002/ with Alpha; in summary I worry > that > putting cast in chrome means that we'd have to expose a lot of the internals > of > content. > > >> 2) We plan to restrict usage of libjingle to just >> //content/renderer/media/webrtc, therefore the directory. > > > I see, so will all the webrtc code in content\renderer\media move there? > > I defer to you guys, just seemed a bit long to type to me. > >> 3) It will be soon. It was drafted internally and received a bunch of >> comments before I realized there is no need for it to be internal. I >> plan to move it to be on http://chromium.org as soon as I've responded to >> the >> first round of reviews. > > > great, thanks. > >> Cheers, >> Jói > > > https://codereview.chromium.org/83023005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry I replied too soon. We need one more interface: expose video encoder accelerator. The file is here: https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_.... We need to include the entire file in content, for example something like: namespace content { scoped_ptr<media::VideoEncodeAccelerator> CreateVideoEncodeAccelerator(media::VideoEncodeAccelerator::Client* client); } // namespace content
lgtm with nits https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... File content/public/renderer/media_stream_sink.h (right): https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... content/public/renderer/media_stream_sink.h:18: class CONTENT_EXPORT MediaStreamSink { nit: I dont' think CONTENT_EXPORT is needed https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:27: class CONTENT_EXPORT MediaStreamVideoSink : public MediaStreamSink { nit: export not needed https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:41: CONTENT_EXPORT void RemoveFromVideoTrack( please move these two methods to be inside MediaStreamVideoSink, as we try to avoid static methods in the content namespace. i.e. class MediaStreamVideoSink : public MedaiStreamSink { public: static Add(...); static Remove(...); virtual void OnVideoFrame(...);
https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... File content/public/renderer/media_stream_sink.h (right): https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... content/public/renderer/media_stream_sink.h:18: class CONTENT_EXPORT MediaStreamSink { On 2013/12/03 00:48:20, jam wrote: > nit: I dont' think CONTENT_EXPORT is needed Done. https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.cc (right): https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... content/public/renderer/media_stream_video_sink.cc:18: video_track->AddSink(sink); On 2013/12/02 17:44:37, Alpha wrote: > track.isNull() can be true and track.extraData() can also be NULL. > > See: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Since the caller doesn't guarantee adding the sink will be successful I think > it's best to handle error cases here and return a boolean. I would argue that it is a programming error if track.isNull()==true or the extra data don't exist. We create the extra data as soon as a blink::WebMediaStreamTrack is created currently. If this is not true, this will crash and we should fix it. https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:27: class CONTENT_EXPORT MediaStreamVideoSink : public MediaStreamSink { On 2013/12/03 00:48:20, jam wrote: > nit: export not needed Done. https://codereview.chromium.org/83023005/diff/210001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:41: CONTENT_EXPORT void RemoveFromVideoTrack( On 2013/12/03 00:48:20, jam wrote: > please move these two methods to be inside MediaStreamVideoSink, as we try to > avoid static methods in the content namespace. > > i.e. > > class MediaStreamVideoSink : public MedaiStreamSink { > public: > static Add(...); > static Remove(...); > > virtual void OnVideoFrame(...); Done. https://codereview.chromium.org/83023005/diff/210001/content/renderer/media/r... File content/renderer/media/rtc_video_renderer.cc (right): https://codereview.chromium.org/83023005/diff/210001/content/renderer/media/r... content/renderer/media/rtc_video_renderer.cc:33: AddToVideoTrack(this, video_track_); On 2013/12/02 17:44:37, Alpha wrote: > I don't think you can stash a WebMediaStreamTrack object, see my comments in the > header file. > > I think you'll need to use a URL and device ID to access the track similar to: > blink::WebMediaStreamRegistry::lookupMediaStreamDescriptor(). See my comment in the header file. We added the extra data field to avoid that since track ids are not guaranteed to be globally unique. If stashing a Web object really is a problem we can consider storing a weak ptr to the MediaStreamVideoTrack object here instead and change MediaStreamVideoTrack to support weak ptrs. The question is just how you would be able to deregister a sink from a video track in chrome if you are not allowed to hold on to a WebMediaStreamTrack object.... So I hope holding on to a WebMediaStreamTrack instance is ok. https://codereview.chromium.org/83023005/diff/210001/content/renderer/media/r... File content/renderer/media/rtc_video_renderer.h (right): https://codereview.chromium.org/83023005/diff/210001/content/renderer/media/r... content/renderer/media/rtc_video_renderer.h:68: blink::WebMediaStreamTrack video_track_; Are you sure this is a problem? To my understanding blink::WebMediaStreamTrack is a kind of smart pointer that make sure the underlaying WebCore object stays alive until all references to it has been deleted. This instance will go out of scope when the MediaStreamPlayer goes out of scope. This is used in several places, at least by webrtc. On 2013/12/02 17:44:37, Alpha wrote: > Sorry I missed this before. I don't think it is safe to stash a > WebMediaStreamTrack. > > This object is just a transient wrapper for a WebCore::MediaStreamComponent. It > merely wraps a pointer and doesn't hold a ref to the webcore object. > > See the definition here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
LGTM after our discussion. Thanks!
https://codereview.chromium.org/83023005/diff/230001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/230001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:29: // An implementation of MediaStreamVideoSink should call AddToVideoTrack when it nit: 80 chars
https://codereview.chromium.org/83023005/diff/230001/content/public/renderer/... File content/public/renderer/media_stream_video_sink.h (right): https://codereview.chromium.org/83023005/diff/230001/content/public/renderer/... content/public/renderer/media_stream_video_sink.h:29: // An implementation of MediaStreamVideoSink should call AddToVideoTrack when it On 2013/12/03 22:00:40, jam wrote: > nit: 80 chars sorry
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/perkj@chromium.org/83023005/250001
Failed to apply patch for content/content_renderer.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/content_renderer.gypi
Hunk #1 FAILED at 585.
Hunk #2 succeeded at 662 (offset 8 lines).
1 out of 2 hunks FAILED -- saving rejects to file
content/content_renderer.gypi.rej
Patch: content/content_renderer.gypi
Index: content/content_renderer.gypi
diff --git a/content/content_renderer.gypi b/content/content_renderer.gypi
index
d85e91234a54c0fb13ccee7ecb728cbf6e69de04..e5c4f8a846f22692083df30c0cfa3144096e8f77
100644
--- a/content/content_renderer.gypi
+++ b/content/content_renderer.gypi
@@ -585,10 +585,15 @@
],
'sources': [
'public/renderer/webrtc_log_message_delegate.h',
+ 'public/renderer/media_stream_sink.h',
+ 'public/renderer/media_stream_video_sink.cc',
+ 'public/renderer/media_stream_video_sink.h',
'renderer/media/media_stream_audio_processor.cc',
'renderer/media/media_stream_audio_processor.h',
'renderer/media/media_stream_audio_processor_options.cc',
'renderer/media/media_stream_audio_processor_options.h',
+ 'renderer/media/media_stream_video_track.cc',
+ 'renderer/media/media_stream_video_track.h',
'renderer/media/media_stream_center.cc',
'renderer/media/media_stream_dependency_factory.cc',
'renderer/media/media_stream_dispatcher.cc',
@@ -654,6 +659,8 @@
'renderer/media/webrtc_local_audio_track.h',
'renderer/media/webrtc_logging.cc',
'renderer/media/webrtc_logging.h',
+ 'renderer/media/webrtc/webrtc_video_sink_adapter.cc',
+ 'renderer/media/webrtc/webrtc_video_sink_adapter.h',
'renderer/p2p/host_address_request.cc',
'renderer/p2p/host_address_request.h',
'renderer/p2p/ipc_network_manager.cc',
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/perkj@chromium.org/83023005/270001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/perkj@chromium.org/83023005/290001
Message was sent while issue was closed.
Change committed as 238694 |
