774581 - Unable to know the status of audio/video recording

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/mozilla-b2g/gaia/issues/2336

The lastest status bar spec specified a "audio/video recording" that need to be shown when there is an app is currently using the mic or camera.

Need a solution in Gecko for this.

Comment 1

[Paul Adenot (:padenot)]

I've got a patch that works for the gUM part, checked at the moment with Vivien. I need to add similar code in the code path we use for the camera app and we will be good to go.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dom/media part

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: b2g/ part

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 652215 [details] [diff] [review]
dom/media part

anant, could you look at the gUM part here?

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 652215 [details] [diff] [review]
dom/media part

Review of attachment 652215 [details] [diff] [review]:
-----------------------------------------------------------------

Fix up those nits and you're good to go from my end.

::: dom/camera/CameraControl.cpp
@@ +377,5 @@
>    mCameraThread->Dispatch(startRecordingTask, NS_DISPATCH_NORMAL);
>  
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (!obs) {
> +    NS_WARNING("Could not get the Observer service for GetUserMedia (startup).");

"GetUserMedia" --> "CameraControl::StartRecording"

@@ +397,5 @@
>    mCameraThread->Dispatch(stopRecordingTask, NS_DISPATCH_NORMAL);
>  
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (!obs) {
> +    NS_WARNING("Could not get the Observer service for GetUserMedia (shutdown).");

"GetUserMedia (shutdown)" --> "CameraControl::StopRecording"

::: dom/media/MediaManager.h
@@ +18,5 @@
>  
> +class GetUserMediaNotificationEvent: public nsRunnable
> +{
> +  public:
> +    GetUserMediaNotificationEvent(bool aState)

Ugh, boolean used for something other than on/off.  How about replacing this with an enum { STARTING, STOPPING }; ?

@@ +24,5 @@
> +
> +    NS_IMETHOD Run() {
> +      nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +      if (!obs) {
> +        NS_WARNING("Could not get the Observer service for GetUserMedia (startup).");

This isn't always on "(startup)", is it?

Comment 6

[Anant Narayanan [:anant]]

Comment on attachment 652215 [details] [diff] [review]
dom/media part

Review of attachment 652215 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure sending an event on the Start() of every MediaStream is what you want here, Could you explain how this works? What happens if two pages obtain a MediaStream each, both are started, and one of them is stopped. Does the notification in the status bar go away? This might work on B2G because of e10s but it won't on Desktop.

::: dom/media/MediaManager.h
@@ +19,5 @@
> +class GetUserMediaNotificationEvent: public nsRunnable
> +{
> +  public:
> +    GetUserMediaNotificationEvent(bool aState)
> +    :mState(aState) {}

Nit: Space after ":"

@@ +21,5 @@
> +  public:
> +    GetUserMediaNotificationEvent(bool aState)
> +    :mState(aState) {}
> +
> +    NS_IMETHOD Run() {

Nit: Run() on next line, { on the line after that

@@ +84,5 @@
>        mSource->Start(stream, mID);
> +      nsCOMPtr<GetUserMediaNotificationEvent> event =
> +        new GetUserMediaNotificationEvent(true);
> +
> +      NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

Are you sure this is what you want? You're sending an event when any MediaStream on any page starts, without a reference to it. Someone observing the event won't know what triggered it, it might be more useful if we included a pointer to the MediaStream which triggered it, or some other information that gives context to the observer.

A better solution would be to simply have a function on the MediaManager like "IsRecordingActive". Since the MediaManager is a singleton, it can keep track of all MediaStreams across all pages, and return true if any of them are active, false if none of them are.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Gaia part is done.

https://github.com/mozilla-b2g/gaia/pull/3511

Comment 8

[Paul Adenot (:padenot)]

> What happens if two pages obtain a MediaStream each, both are started, and one of them is
> stopped. Does the notification in the status bar go away? This might work on B2G because of 
> e10s but it won't on Desktop.

I would expect the front-end to have a counter to be able to tell how many streams are active, but it appears that the pull request in comment 7 does not implement this. I'll implement the counting on our side.

If you feel that we should pass-in the pointer, so be it. It probably won't be used, though.

> A better solution would be to simply have a function on the MediaManager like 
> "IsRecordingActive". Since the MediaManager is a singleton, it can keep track of all 
> MediaStreams across all pages, and return true if any of them are active, false if none of 
> them are.

This kind of API is not adequate for this use case. The gaia folks specifically said they are using the observer service.

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Paul ADENOT (:padenot) from comment #8)
> > What happens if two pages obtain a MediaStream each, both are started, and one of them is
> > stopped. Does the notification in the status bar go away? This might work on B2G because of 
> > e10s but it won't on Desktop.
> 
> I would expect the front-end to have a counter to be able to tell how many
> streams are active, but it appears that the pull request in comment 7 does
> not implement this. I'll implement the counting on our side.

That is done in shell.js, nothing to worry about.

> > A better solution would be to simply have a function on the MediaManager like 
> > "IsRecordingActive". Since the MediaManager is a singleton, it can keep track of all 
> > MediaStreams across all pages, and return true if any of them are active, false if none of 
> > them are.
> 
> This kind of API is not adequate for this use case. The gaia folks
> specifically said they are using the observer service.

Indeed, we don't want to poll.

Comment 10

[Anant Narayanan [:anant]]

(In reply to Paul ADENOT (:padenot) from comment #8)
> I would expect the front-end to have a counter to be able to tell how many
> streams are active, but it appears that the pull request in comment 7 does
> not implement this. I'll implement the counting on our side.
> 
> If you feel that we should pass-in the pointer, so be it. It probably won't
> be used, though.

Rather than implementing the counter in every frontend, it seems best to be do it in one place. Why not do the counting in MediaManager and only send events when recording is on or off, instead of with every MediaStream? This will simplify the frontend code for Gaia.

> This kind of API is not adequate for this use case. The gaia folks
> specifically said they are using the observer service.

Makes sense.

Comment 11

[Paul Adenot (:padenot)]

> Rather than implementing the counter in every frontend, it seems best to be do it in one 
> place. Why not do the counting in MediaManager and only send events when recording is on or 
> off, instead of with every MediaStream? This will simplify the frontend code for Gaia.

This does not work. We would need to have some kind of shared state between the gUM code and the camera API code, to be able to fire the notification when we go from 'nothing is recording' to 'at least one thing is recording' and the inverse, and not when each module starts or stops recording. In my opinion, it make sense to fire an event when something starts or stops recording, and let the front-end aggregate the data to react, instead of sharing state all over.

I still don't know what would be the use for the pointer (that would be received on the wrong thread anyway for the gUM part) considering it might not be valid when we want to use it.

Comment 12

[Anant Narayanan [:anant]]

(In reply to Paul ADENOT (:padenot) from comment #11)
> > Rather than implementing the counter in every frontend, it seems best to be do it in one 
> > place. Why not do the counting in MediaManager and only send events when recording is on or 
> > off, instead of with every MediaStream? This will simplify the frontend code for Gaia.
> 
> This does not work. We would need to have some kind of shared state between
> the gUM code and the camera API code, to be able to fire the notification
> when we go from 'nothing is recording' to 'at least one thing is recording'
> and the inverse, and not when each module starts or stops recording. In my
> opinion, it make sense to fire an event when something starts or stops
> recording, and let the front-end aggregate the data to react, instead of
> sharing state all over.

I see the problem, the reason you need shared state is because CameraControl and MediaManager are separate, when in fact they shouldn't be. However, until we port the features the Gaia Camera app needs to gUM, I guess we're stuck with this. I think it's pretty ugly to implement the counting in every front-end, the back-end is after all *the* authoritative source for whether recording is occurring or not and we really only need a single global notification. Also, keep in mind that just because a MediaStream starts doesn't mean recording is happening. A MediaEngine can be backed by a file, a stream doesn't always come from hardware.

But, I don't want to block B2G on this, we can refactor this code when we merge CameraControl into the MediaManager. This'll do for now. I filed bug 783472 so we can followup later (also add a reference to that bug in the MediaManager code).

r+ with nits fixed, don't worry about the MediaStream pointer in the notification for now. You're right about the problem with it not being on the correct thread.

Comment 13

[Paul Adenot (:padenot)]

Attachment: Send event when starting/stopping audio or video recording.

Thanks for the review, we surely need to refactor that code at some point.

Comment 14

[Anant Narayanan [:anant]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88d36d74c414
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0b0ff6e6908

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/88d36d74c414
https://hg.mozilla.org/mozilla-central/rev/c0b0ff6e6908

Comment 16

[Marco Chen [:mchen]]

May I know any one verified this on B2G build?
Due to another bug, I tried to hook recording status from shell.js but I can't get any messages about status change via observer.
Thanks.