Last Comment Bug 759908 - MediaStreams need Notify callbacks
: MediaStreams need Notify callbacks
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: WebRTC (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks: 752353
  Show dependency treegraph
 
Reported: 2012-05-30 14:34 PDT by Anant Narayanan [:anant]
Modified: 2012-07-27 10:24 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (5.13 KB, patch)
2012-05-30 23:17 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
anant: feedback+
Details | Diff | Review

Description Anant Narayanan [:anant] 2012-05-30 14:34:56 PDT
When content assgins (or de-assigns) a stream on media elements like <video> and <audio>, the underlying media engine should be notified via "NotifyStartConsuming" and "NotifyStopConsuming" callbacks.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-30 23:17:14 PDT
Created attachment 628608 [details] [diff] [review]
fix

This is actually totally untested, but it's pretty simple so I expect it to work.
Comment 2 [:jesup] on pto until 2016/7/5 Randell Jesup 2012-05-30 23:29:11 PDT
Comment on attachment 628608 [details] [diff] [review]
fix

Looks good to me.   Pretty simple and direct.  r+ from me (though I wasn't asked) assuming it meets all of anant's needs (and I strongly suspect it will).
Comment 3 Anant Narayanan [:anant] 2012-05-31 14:41:39 PDT
Comment on attachment 628608 [details] [diff] [review]
fix

I updated the DOM patch to use this. Though it doesn't quite work yet (main thread weirdness), the API is great! I'll post on the bug again when I get it work for real.
Comment 4 Anant Narayanan [:anant] 2012-05-31 20:47:12 PDT
I just tested this out, and it works. Let's land it!
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-01 00:22:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2074a7b72bd1
Comment 6 Anant Narayanan [:anant] 2012-06-01 22:57:22 PDT
NotfyConsumptionChanged does not get called when a window with a live media stream is closed. Is that expected behaviour?
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-02 11:44:40 PDT
https://hg.mozilla.org/mozilla-central/rev/2074a7b72bd1
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-04 16:36:22 PDT
(In reply to Anant Narayanan [:anant] from comment #6)
> NotfyConsumptionChanged does not get called when a window with a live media
> stream is closed. Is that expected behaviour?

Yes.

If you want to stop producing video/audio immediately when your window is closed, you should track whether your document is active or not.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-04 16:39:03 PDT
We could change NotfyConsumptionChanged so that we say "no consumers" under more conditions, but it's not clear to me what the right definition would be. One issue is that it's not easy to distinguish between "page going into bfcache and may be revisited and resume playing" from "window closed". In fact it is possible to close a window containing a playing media element, but keep a reference to that element from another window, and then pull the media element out into a document that's still visible and resume it playing!
Comment 10 Anant Narayanan [:anant] 2012-06-04 17:08:18 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> We could change NotfyConsumptionChanged so that we say "no consumers" under
> more conditions, but it's not clear to me what the right definition would
> be. One issue is that it's not easy to distinguish between "page going into
> bfcache and may be revisited and resume playing" from "window closed". In
> fact it is possible to close a window containing a playing media element,
> but keep a reference to that element from another window, and then pull the
> media element out into a document that's still visible and resume it playing!

Ah, that's an interesting use-case. We might want to support resuming a video call from an accidentally closed tab or going back from accidental page navigation. On the other hand, there might be other security issues with this behaviour. Regardless, you're right that the current behaviour is more flexible, not tying a stream's lifetime to a window is the right thing to do.

I've changed the DOM bindings to watch for window navigation and closures at which point we release our reference to the stream. At a later point, we may want to keep the stream around to enable some interesting use cases, but this works for now.

Note You need to log in before you can comment on or make changes to this bug.