Temasys / AdapterJS

AdapterJS Javascript Polyfill and Tools for WebRTC - Skylink WebRTC
http://skylink.io/web
Other
432 stars 100 forks source link

Allow attachMediaStream() to detach stream by passing null #50

Closed ekzobrain closed 9 years ago

ekzobrain commented 9 years ago

It is often needed to not only attach, but also detach media stream from video/audio element. Detaching code looks almost like attaching one, and logically it might be a good decision to use attachMediaStream() for it like this: attachMediaStream(element, null)

The only thing that needs to be changed in attachMediaStream() is line "element.src = URL.createObjectURL(stream);" to "element.src = (stream === null ? "" : URL.createObjectURL(stream));", and also disable video.play() in Firefox-specific attachMediaStream() version. It may also be done via a separate function, but it will be just code duplicate.

serrynaimo commented 9 years ago

Cool. Sounds like a good idea. Something to put into our next release.

ekzobrain commented 9 years ago

Hm, I also notices, that this trick will not work in Safari/IE when using plugin. In general - plugin replaces original video tag with an object tag. I suppose, that new implementation should also return an original video tag back to it's initial state (need to fix plugin-related version of attachMediaStream())

johache commented 9 years ago

Pull request sent: https://github.com/Temasys/AdapterJS/pull/53

ekzobrain commented 9 years ago

Ok, I see your point about element restore, but why dont't you want to go a simpler way? It is general practice to hide the orignal element with style="display:none", not remove completely. Then you could restore it easily, and all of it's properties (even changed while working with plugin) would be preserved. Currently, yes, we have such a use case: by default video tag is opaque and user sees a picture behind it. When a call starts - peer's stream it attached to this video element. When a call ends video tag should becomes opaque again (that's why we need to detach media stream from it). And between calls some video files may be played in this video tag. Of course we could just replace video tag with a new one every time, but i think it is a bad approach. Please, think about my suggestion about hiding original video tag, and then restoring it back to visible state. But if you reject it either - let me know how to correctly remove an object tag, created by your plugin. I think, that some cleanup shoul be done on it prior to removing an element itself? In there any "destruct" method available?

ekzobrain commented 9 years ago

Also, this approach might be helpful in listening to video element events. For example, you hide video element and listen to it's "volumechange" event to enable/disable stream audio tracks. That would solve #34. So generally, user interracts with original video element, and you just proxy it's events to your plugin in some way. Of couse this approach needs investigation and testing, but i find it possible and perspective.

johache commented 9 years ago

@serrynaimo @ncurrier Setting style="display:none" instead of removing the video element. What do you guys think of that. I think it might be a good idea. I read it should work fine on older version of IE: http://stackoverflow.com/questions/17462209/style-displaynone-not-working-in-ie8-ie9-ie10-compatibility-view

My only concern is about the element ID management. We are copying the