MediaStreamVideoSource should adopt the safer object reference scheme used by MSAudioSource |
||||
Issue descriptionWhile reviewing https://codereview.chromium.org/1834323002/, I discussed with perkj@ the reasons why MSAudioSource just provides a simple "ConnectToTrack" method instead of separate AddTrack()/RemoveTrack() methods like in MSVideoSources. We decided that MSVideoSources should adopt the simpler "ConnectToTrack" scheme. I've pasted the relevant discussion below to explain further. It's also worth noting that, if MSVideoSources uses the same scheme, we may also just want the parent class, MediaStreamSource to implement the scheme and then have MSAudioSources and MSVideoSources inherit that. --------------------------------------snip------------------------------------ On 2016/04/08 14:05:41, perkj_chrome wrote: > For video- the tracks register and deregister with the source instead using a > ConnectToTrack. Would it make sence for audio to work the same way for > consistency? I looked into this a bit. IMO, it would be better for MediaStreamVideoSource to instead change to the scheme used here since it makes the client code much simpler. Some fine points: 1. MSAudioSources sometimes need to provide extended MSAudioTrack implementations. It's more appropriate for the source to create the MSAudioTrack rather than the client code (e.g., media_stream_center.cc) since the client code does not (and probably should not) know the implementation details of the source. 2. Client code must be sure to call MSVideoSource::RemoveTrack() at the right time and get it right for all possible tear-down sequences and code paths. With this scheme, MSAudioSources and MSAudioTracks are guaranteed to be safely shutdown and destroyed whenever their blink owners are destroyed, with no additional client code written to ensure this. 3. Safety/Robustness: There are comments in MSVideoSource about it's blink owner having to outlive all tracks. To me, that feels a bit brittle, as future changes to blink code could break this assumption. If the blink code happens to be changed in some way that causes a MSAudioSource to tear-down before its tracks, the weak references will make sure nothing bad happens.
,
May 14 2016
I started looking a bit at this with one eye and on Eurovision song contest with the other. I remembered why AddTrack/RemoveTrack was used in the first place. The idea is that it should be easy to apply constraints http://w3c.github.io/mediacapture-main/#widl-MediaStreamTrack-applyConstraints-Promise-void--MediaTrackConstraints-constraints The idea was that applyConstraints can be implemented similar to as how the the video track is created. (Call VideoSource::RemoveTrack and Video::Source::AddTrack with the new constraints + handle the fact that a source with no tracks are stopped + handle the case when the constraints can not be applied. That feels a bit dum and I think it would be better if the existing MediaStreamVideoSource::AddTrack( MediaStreamVideoTrack* track, const VideoCaptureDeliverFrameCB& frame_callback, const blink::WebMediaConstraints& constraints, const ConstraintsCallback& callback) is changed to VideoSource::ApplyConstraints MediaStreamVideoTrack* track, const blink::WebMediaConstraints& constraints, const ConstraintsCallback& callback) VideoSource::RemoveTrack removed. and sources created from gUM require that ApplyConstraints have been called before it is started since the constraints are needed to know resolution etc. With that, I think we can change video tracks to be created like the audio tracks.
,
Jul 11 2016
,
Sep 14 2016
Reassigning to Guido since I will start on another project. I uploaded what I started hacking on here, not sure if that is any help but I am very willing to discuss. https://codereview.chromium.org/2339193002
,
Sep 19 2016
Support for everything
,
Sep 29 2016
|
||||
►
Sign in to add a comment |
||||
Comment 1 by nabilamn...@gmail.com
, Apr 25 2016