Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
99113003:
Define the interface for MediaStreamVideoSource. (Closed)
|
Created:
7 years ago by Ronghua Wu (Left Chromium) Modified:
6 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, Peng Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionDefine the interface for MediaStreamVideoSource.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243692
Patch Set 1 #
Total comments: 14
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 22 (0 generated)
Joi- can you please take a look at this proposal and my comments? I think Ronghua wants to implement a NaCl MediaStreamVideoSource and therefore needs a first version of the base class. This needs some discussion. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... File content/public/renderer/media_stream_video_source.cc (right): https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.cc:14: const blink::WebMediaConstraints& constrains) { indentation https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... File content/public/renderer/media_stream_video_source.h (right): https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:6: #define CONTENT_PUBLIC_RENDERER_MEDIA_STREAM_VIDEO_SOURCE_H_ Why does the source have to be in content public? Put in renderer/media? https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:26: // MediaStreamVideoSink is an interface used for sending video frames to a update the comment. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:30: class CONTENT_EXPORT MediaStreamVideoSource : public MediaStreamSink { Should this inherit from MediaStreamSourceExtraData - same as MediaStreamVideoTrack? It should be owned by blink source extra data and by inheriting from MediaStreamSourceExtraData an implementation get access to the blink source object. Do you need to inherit from MediaStreamSink? I know that it says so in the design doc but I am not sure how we want to do this. Either we have multiple implementations of a MediaStreamVideoSource. 1. local capture, 1 Nacl, 1 remote tracks . Or we have one implementation of MediaStreamSource and each specialization is implemented by a delegate instead and the MediaStreamSource inherit from MediaStreamVideoSink or similar and owns the delegate. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:32: bool Register(MediaStreamVideoSink* track, One thing that came up yesterday was applyConstraints. Our idea was to handle that by Register/ Unregister with constraints to the source. But the source might have to do something async, ex restart the camera and only when that has happened would we know if Register succeeded. So the source might have to tell the track using a callback if Register with the new constraint succeeded or not. I don't think such a callback belong in MediaStreamVideoSink since things like a renderer don't need this. Would it therefore make sence to have Register take a pointer to a MediaStreamVideoTrack object instead? MediaStreamVideoTrack can maybe inherit from MediaStreamVideoSink + add new methods. btw- webrtc_audio_capturer (which is the audio source) call this AddTrack and RemoveTrack instead of Register and UnRegister. Maybe we should do the same. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:32: bool Register(MediaStreamVideoSink* track, Comment on thread requirements please. Here and the other methods. For the time being, all methods will be called on the main render thread. See comment in MediaStreamVideoSink.
I think we will need an interface/base class similar to this, although as Per points out we don't yet know the specifics of how we will specialize it for the different cases. It doesn't belong in content/public unless there are firm plans to use it from the embedder, which I doubt would be the case. Cheers, Jói https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... File content/public/renderer/media_stream_video_source.h (right): https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:30: class CONTENT_EXPORT MediaStreamVideoSource : public MediaStreamSink { On 2013/12/18 08:15:10, perkj wrote: > Should this inherit from MediaStreamSourceExtraData - same as > MediaStreamVideoTrack? > It should be owned by blink source extra data and by inheriting from > MediaStreamSourceExtraData an implementation get access to the blink source > object. > > Do you need to inherit from MediaStreamSink? I know that it says so in the > design doc but I am not sure how we want to do this. Either we have multiple > implementations of a MediaStreamVideoSource. 1. local capture, 1 Nacl, 1 remote > tracks . Or we have one implementation of MediaStreamSource and each > specialization is implemented by a delegate instead and the MediaStreamSource > inherit from MediaStreamVideoSink or similar and owns the delegate. I agree we might want to hold off on inheriting from MediaStreamSink until we're sure that's the appropriate interface for media providers that push data to the source to use. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:32: bool Register(MediaStreamVideoSink* track, On 2013/12/18 08:15:10, perkj wrote: > One thing that came up yesterday was applyConstraints. Our idea was to handle > that by Register/ Unregister with constraints to the source. But the source > might have to do something async, ex restart the camera and only when that has > happened would we know if Register succeeded. > So the source might have to tell the track using a callback if Register with the > new constraint succeeded or not. > I don't think such a callback belong in MediaStreamVideoSink since things like a > renderer don't need this. > Would it therefore make sence to have Register take a pointer to a > MediaStreamVideoTrack object instead? MediaStreamVideoTrack can maybe inherit > from MediaStreamVideoSink + add new methods. > > btw- webrtc_audio_capturer (which is the audio source) call this AddTrack and > RemoveTrack instead of Register and UnRegister. Maybe we should do the same. If MediaStreamVideoTrack has some methods that a source would need to use and that aren't appropriate for a sink interface, then I would tend to agree, this should register tracks rather than sinks.
Thanks for comments. PTAL https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... File content/public/renderer/media_stream_video_source.cc (right): https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.cc:14: const blink::WebMediaConstraints& constrains) { On 2013/12/18 08:15:10, perkj wrote: > indentation Done. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... File content/public/renderer/media_stream_video_source.h (right): https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:6: #define CONTENT_PUBLIC_RENDERER_MEDIA_STREAM_VIDEO_SOURCE_H_ On 2013/12/18 08:15:10, perkj wrote: > Why does the source have to be in content public? Put in renderer/media? I don't really know what is the content public about. Just thought to put this aside to video sink. Where do you think I should put this? content/renderer/media? https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:26: // MediaStreamVideoSink is an interface used for sending video frames to a On 2013/12/18 08:15:10, perkj wrote: > update the comment. Done. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:30: class CONTENT_EXPORT MediaStreamVideoSource : public MediaStreamSink { On 2013/12/18 13:47:07, Jói wrote: > On 2013/12/18 08:15:10, perkj wrote: > > Should this inherit from MediaStreamSourceExtraData - same as > > MediaStreamVideoTrack? > > It should be owned by blink source extra data and by inheriting from > > MediaStreamSourceExtraData an implementation get access to the blink source > > object. > > > > Do you need to inherit from MediaStreamSink? I know that it says so in the > > design doc but I am not sure how we want to do this. Either we have multiple > > implementations of a MediaStreamVideoSource. 1. local capture, 1 Nacl, 1 > remote > > tracks . Or we have one implementation of MediaStreamSource and each > > specialization is implemented by a delegate instead and the MediaStreamSource > > inherit from MediaStreamVideoSink or similar and owns the delegate. > > I agree we might want to hold off on inheriting from MediaStreamSink until we're > sure that's the appropriate interface for media providers that push data to the > source to use. Ok. Inheriting from MediaStreamSourceExtraData instread. Use PutVideoFrame to distinguish from MediaStreamVideoSink interface. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:32: bool Register(MediaStreamVideoSink* track, On 2013/12/18 08:15:10, perkj wrote: > Comment on thread requirements please. Here and the other methods. > For the time being, all methods will be called on the main render thread. See > comment in MediaStreamVideoSink. It's at the beginning of the class. https://codereview.chromium.org/99113003/diff/1/content/public/renderer/media... content/public/renderer/media_stream_video_source.h:32: bool Register(MediaStreamVideoSink* track, On 2013/12/18 13:47:07, Jói wrote: > On 2013/12/18 08:15:10, perkj wrote: > > One thing that came up yesterday was applyConstraints. Our idea was to handle > > that by Register/ Unregister with constraints to the source. But the source > > might have to do something async, ex restart the camera and only when that has > > happened would we know if Register succeeded. > > So the source might have to tell the track using a callback if Register with > the > > new constraint succeeded or not. > > I don't think such a callback belong in MediaStreamVideoSink since things like > a > > renderer don't need this. > > Would it therefore make sence to have Register take a pointer to a > > MediaStreamVideoTrack object instead? MediaStreamVideoTrack can maybe inherit > > from MediaStreamVideoSink + add new methods. > > > > btw- webrtc_audio_capturer (which is the audio source) call this AddTrack and > > RemoveTrack instead of Register and UnRegister. Maybe we should do the same. > > If MediaStreamVideoTrack has some methods that a source would need to use and > that aren't appropriate for a sink interface, then I would tend to agree, this > should register tracks rather than sinks. Ok. I thought we are not suppose to reopen the camera in any case. That's why I thought this can return synchronizly and don't need a callback, in which case MediaStreamVideoSink will be the only interface needed from the track. Change to MediaStreamVideoTrack SGTM regardless.
Yes, the new classes belong in content/renderer/media. (typed on tiny keyboard) On Dec 18, 2013 1:47 PM, <joi@chromium.org> wrote: > I think we will need an interface/base class similar to this, although as > Per > points out we don't yet know the specifics of how we will specialize it > for the > different cases. > > It doesn't belong in content/public unless there are firm plans to use it > from > the embedder, which I doubt would be the case. > > Cheers, > Jói > > > > https://codereview.chromium.org/99113003/diff/1/content/ > public/renderer/media_stream_video_source.h > File content/public/renderer/media_stream_video_source.h (right): > > https://codereview.chromium.org/99113003/diff/1/content/ > public/renderer/media_stream_video_source.h#newcode30 > content/public/renderer/media_stream_video_source.h:30: class > CONTENT_EXPORT MediaStreamVideoSource : public MediaStreamSink { > On 2013/12/18 08:15:10, perkj wrote: > >> Should this inherit from MediaStreamSourceExtraData - same as >> MediaStreamVideoTrack? >> It should be owned by blink source extra data and by inheriting from >> MediaStreamSourceExtraData an implementation get access to the blink >> > source > >> object. >> > > Do you need to inherit from MediaStreamSink? I know that it says so in >> > the > >> design doc but I am not sure how we want to do this. Either we have >> > multiple > >> implementations of a MediaStreamVideoSource. 1. local capture, 1 Nacl, >> > 1 remote > >> tracks . Or we have one implementation of MediaStreamSource and each >> specialization is implemented by a delegate instead and the >> > MediaStreamSource > >> inherit from MediaStreamVideoSink or similar and owns the delegate. >> > > I agree we might want to hold off on inheriting from MediaStreamSink > until we're sure that's the appropriate interface for media providers > that push data to the source to use. > > https://codereview.chromium.org/99113003/diff/1/content/ > public/renderer/media_stream_video_source.h#newcode32 > content/public/renderer/media_stream_video_source.h:32: bool > Register(MediaStreamVideoSink* track, > On 2013/12/18 08:15:10, perkj wrote: > >> One thing that came up yesterday was applyConstraints. Our idea was to >> > handle > >> that by Register/ Unregister with constraints to the source. But the >> > source > >> might have to do something async, ex restart the camera and only when >> > that has > >> happened would we know if Register succeeded. >> So the source might have to tell the track using a callback if >> > Register with the > >> new constraint succeeded or not. >> I don't think such a callback belong in MediaStreamVideoSink since >> > things like a > >> renderer don't need this. >> Would it therefore make sence to have Register take a pointer to a >> MediaStreamVideoTrack object instead? MediaStreamVideoTrack can maybe >> > inherit > >> from MediaStreamVideoSink + add new methods. >> > > btw- webrtc_audio_capturer (which is the audio source) call this >> > AddTrack and > >> RemoveTrack instead of Register and UnRegister. Maybe we should do the >> > same. > > If MediaStreamVideoTrack has some methods that a source would need to > use and that aren't appropriate for a sink interface, then I would tend > to agree, this should register tracks rather than sinks. > > https://codereview.chromium.org/99113003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Moved to content/renderer/media.
I think this looks good, just a few comments. Per, what do you think? https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... content/renderer/media/media_stream_video_source.cc:14: const blink::WebMediaConstraints& constrains) { constrains -> constraints https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... content/renderer/media/media_stream_video_source.cc:16: // deliver frames to it according to |constrains|. |constrains| -> |constraints| https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... File content/renderer/media/media_stream_video_source.h (right): https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... content/renderer/media/media_stream_video_source.h:39: nit: Just one blank line.
It looks good to me. But how will we handle specialization? Did we decide by delegation? Should the delegate be owned by MediaStreamImpl. RTCPeerConnectionHandler etc? That might make sence. I think we should also have a SetReadyState method that sets the blink objects ready state and notifies the ready state to all registered tracks. IE when the delegate is started- it sets the ready state to live. When the delegate is stopped it sets the ready state to ended.
I don't think we've really decided how to handle the specialization. It seems to me that the "ideal" approach would be to handle specialization by inheritance, since there is an is-a relationship, but whatever is practical seems fine to me. Cheers, Jói On Thu, Dec 19, 2013 at 2:41 PM, <perkj@chromium.org> wrote: > It looks good to me. > > But how will we handle specialization? Did we decide by delegation? > Should the delegate be owned by MediaStreamImpl. RTCPeerConnectionHandler > etc? > That might make sence. > > I think we should also have a SetReadyState method that sets the blink > objects > ready state and notifies the ready state to all registered tracks. > > IE when the delegate is started- it sets the ready state to live. > When the delegate is stopped it sets the ready state to ended. > > > https://codereview.chromium.org/99113003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Addressed the comments and added SetReadyState. In terms of specialization, Joe has a good point about the "is-a relationship". Now I'm actually disposed inheritance more. But I guess we can decide later when I start implementing this class. https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... content/renderer/media/media_stream_video_source.cc:14: const blink::WebMediaConstraints& constrains) { On 2013/12/19 12:06:23, Jói wrote: > constrains -> constraints Done. https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... content/renderer/media/media_stream_video_source.cc:16: // deliver frames to it according to |constrains|. On 2013/12/19 12:06:23, Jói wrote: > |constrains| -> |constraints| Done. https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... File content/renderer/media/media_stream_video_source.h (right): https://codereview.chromium.org/99113003/diff/40001/content/renderer/media/me... content/renderer/media/media_stream_video_source.h:39: On 2013/12/19 12:06:23, Jói wrote: > nit: Just one blank line. Done.
OK. For local video capture we need to implement media:video capture:Eventhandler somewhere. If we decide for delegation , the source can maybe inherit that and the delegate implement media:video capture. I don't know how suitable that is for the other specializations but it seems feasible to me. Den 19 dec 2013 21:04 skrev <ronghuawu@chromium.org>: > Addressed the comments and added SetReadyState. > > In terms of specialization, Joe has a good point about the "is-a > relationship". > Now I'm actually disposed inheritance more. But I guess we can decide > later when > I start implementing this class. > > > https://codereview.chromium.org/99113003/diff/40001/ > content/renderer/media/media_stream_video_source.cc > File content/renderer/media/media_stream_video_source.cc (right): > > https://codereview.chromium.org/99113003/diff/40001/ > content/renderer/media/media_stream_video_source.cc#newcode14 > content/renderer/media/media_stream_video_source.cc:14: const > blink::WebMediaConstraints& constrains) { > On 2013/12/19 12:06:23, Jói wrote: > >> constrains -> constraints >> > > Done. > > https://codereview.chromium.org/99113003/diff/40001/ > content/renderer/media/media_stream_video_source.cc#newcode16 > content/renderer/media/media_stream_video_source.cc:16: // deliver > frames to it according to |constrains|. > On 2013/12/19 12:06:23, Jói wrote: > >> |constrains| -> |constraints| >> > > Done. > > https://codereview.chromium.org/99113003/diff/40001/ > content/renderer/media/media_stream_video_source.h > File content/renderer/media/media_stream_video_source.h (right): > > https://codereview.chromium.org/99113003/diff/40001/ > content/renderer/media/media_stream_video_source.h#newcode39 > content/renderer/media/media_stream_video_source.h:39: > On 2013/12/19 12:06:23, Jói wrote: > >> nit: Just one blank line. >> > > Done. > > https://codereview.chromium.org/99113003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/99113003/diff/60001/content/renderer/media/me... File content/renderer/media/media_stream_video_source.h (right): https://codereview.chromium.org/99113003/diff/60001/content/renderer/media/me... content/renderer/media/media_stream_video_source.h:34: void SetReadyState(blink::WebMediaStreamSource::ReadyState state); Can SetReadyState and PutVideoFrame be protected if we decided to go with inheritance? And why not rename PutVideoFrame to DeliverVideoFrame?
PTAL https://codereview.chromium.org/99113003/diff/60001/content/renderer/media/me... File content/renderer/media/media_stream_video_source.h (right): https://codereview.chromium.org/99113003/diff/60001/content/renderer/media/me... content/renderer/media/media_stream_video_source.h:34: void SetReadyState(blink::WebMediaStreamSource::ReadyState state); On 2014/01/05 15:38:03, perkj__ooo_until_7ofJan wrote: > Can SetReadyState and PutVideoFrame be protected if we decided to go with > inheritance? > And why not rename PutVideoFrame to DeliverVideoFrame? Done.
lgtm. I downloaded this patch and started sketching on a local video capture source. Does it make sence to defer landing this until we have an implementation?
On 2014/01/07 06:25:12, perkj wrote: > lgtm. I downloaded this patch and started sketching on a local video capture > source. > > Does it make sence to defer landing this until we have an implementation? I do want to land this and continue to implement the MediaStreamVideoSource, see TODOs in media_stream_video_source.cc. These methods will be shared by all the implementations.
+jam for gyp files approval.
On 2014/01/07 17:12:26, Ronghua Wu wrote: > +jam for gyp files approval. lgtm, sorry for the delay. please feel free to tbr simple additions/removals from gypi files
owner stamp for media, lgtm.
Thanks all. Will commit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/99113003/440001
Message was sent while issue was closed.
Change committed as 243692 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
