886891 - Fake audio still shows devices being shared

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Eric Rescorla (:ekr)]

Attachment: STR: load this file

If you do fake audio/video with gUM you still get the indicator in the URL bar that you are
sharing the camera and microphone. This isn't great, in part because datachannels used to need a fake audio stream to piggyback on, and this means that old datachannels demos look like they are sharing the mic.

Comment 1

[Suhas]

Attachment: Don't show recording indicator for data-channel

Comment 2

[Suhas]

Comment on attachment 771126 [details] [diff] [review]
Don't show recording indicator for data-channel

WIP patch

Comment 3

[Suhas]

Attachment: Don't show recording indicator when using fake sources

Comment 4

[Suhas]

Comment on attachment 771136 [details] [diff] [review]
Don't show recording indicator when using fake sources

Disable recording-events notifications to observer if fake source(s) are used.
This will be useful in the cases of data channel use-cases when fake audio is used.
Same shall also apply for those use-cases that just use fake media.
In both the cases above, recording indicator will not be shown in the Chrome.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 771136 [details] [diff] [review]
Don't show recording indicator when using fake sources

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

r- because the real problem here isn't the event, it's the definition of CapturingVideo()/Audio in MediaManager.h

I think if you simply modify that to check if the Sources are fake (perhaps adding an "IsFake() to Sources), instead of the suppression of the event, it will work much better.

Comments on the code style/comments included anyways, though likely most of this code will go away.

::: dom/media/MediaManager.cpp
@@ +427,5 @@
>      // Dispatch to the media thread to ask it to start the sources,
>      // because that can take a while.
>      // Pass ownership of trackunion to the MediaOperationRunnable
> +    // to ensure it's kept alive until the MediaOperationRunnable runs(at least)
> +    // Bug 886891: Pass on fake preferece to supress recording-event propogation

preference, suppress, and it's not propagation, it's notification.
but really it's an attribute

@@ +471,5 @@
>  
>  private:
>    nsRefPtr<nsIDOMGetUserMediaSuccessCallback> mSuccess;
>    nsRefPtr<nsIDOMGetUserMediaErrorCallback> mError;
> +  bool mFake; // Indicates the existence of fake source.

comment not needed

@@ +542,5 @@
>  
>    /**
>     * The caller can also choose to provide their own backend instead of
>     * using the one provided by MediaManager::GetBackend.
> +   * Bug886891: For DefaultBackend we pass in the Fake source indicator.

We don't need to tag state in the final code with references to what bug changed them, unless it's still important in some way.

@@ +795,5 @@
>  
>  private:
>    bool mAudio;
>    bool mVideo;
> +  bool mFake; // Indicates the existence of fake source(s).

comment not needed

@@ +1087,5 @@
>    // XXX take options from constraints instead of prefs
>    if (fake) {
>      // Fake stream from default backend.
> +    // Bug886891: Pass fake source indicator along the chain to eventualy
> +    // suppress the recording-indicator events.

no need to repeat this all over the code.  Say it once, without the bug number.
"eventually"

@@ +1092,2 @@
>      gUMRunnable = new GetUserMediaRunnable(
> +      audio, video, fake, onSuccess.forget(), onError.forget(), windowID, listener, mPrefs,

split the argument list (line length)

::: dom/media/MediaManager.h
@@ +171,5 @@
>  private:
>    // Set at construction
>    nsCOMPtr<nsIThread> mMediaThread;
>    uint64_t mWindowID;
> +  bool mFake; // Indicates if we have to supress recording-events indicator.

suppress

@@ +242,5 @@
>            }
>            break;
>        }
> +      // Bug886891: if we have a fake source, supress recording events
> +      if(!mFake) {

Space after if (in this code at least):
if (!mFake) {

@@ +384,5 @@
>  private:
>    MediaOperation mType;
>    nsRefPtr<DOMMediaStream> mStream;
>    nsAutoPtr<DOMMediaStream::OnTracksAvailableCallback> mOnTracksAvailableCallback;
> +  bool mFake; // Indicates if there is a fake source.

comment isn't needed here

Comment 6

[Suhas]

Attachment: Don't show recording indicator when using fake sources

Comment 7

[Suhas]

Agree Randell. I will go ahead with add a function to identify the source type. That seems to be more clean logic

Comment 8

[Suhas]

Attachment: Don't show recording indicator when using fake sources

Comment 9

[Suhas]

Attachment: Don't show recording indicator when using fake sources

Comment 10

[Suhas]

Attachment: Don't show recording indicator when using fake sources

Comment 11

[Suhas]

Attachment: Don't show recording indicator when using fake sources

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67b5b54a57cd

We can consider uplifting this to Aurora.  Not critical.  Very simple patch though

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67b5b54a57cd