Fake audio still shows devices being shared

RESOLVED FIXED in mozilla25

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ekr, Assigned: Suhas)

Tracking

unspecified
mozilla25
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia] [blocking-gum-])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 767304 [details]
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.
(Assignee)

Comment 1

5 years ago
Created attachment 771126 [details] [diff] [review]
Don't show recording indicator for data-channel
(Assignee)

Comment 2

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

WIP patch
(Assignee)

Updated

5 years ago
Assignee: nobody → snandaku
(Assignee)

Comment 3

5 years ago
Created attachment 771136 [details] [diff] [review]
Don't show recording indicator when using fake sources
(Assignee)

Updated

5 years ago
Attachment #771126 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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.
Attachment #771136 - Flags: review?(rjesup)
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
Attachment #771136 - Flags: review?(rjesup) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 771533 [details] [diff] [review]
Don't show recording indicator when using fake sources
(Assignee)

Updated

5 years ago
Attachment #771136 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Agree Randell. I will go ahead with add a function to identify the source type. That seems to be more clean logic
(Assignee)

Comment 8

5 years ago
Created attachment 771538 [details] [diff] [review]
Don't show recording indicator when using fake sources
(Assignee)

Updated

5 years ago
Attachment #771533 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #771538 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 771539 [details] [diff] [review]
Don't show recording indicator when using fake sources
(Assignee)

Updated

5 years ago
Attachment #771539 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 771540 [details] [diff] [review]
Don't show recording indicator when using fake sources
(Assignee)

Updated

5 years ago
Attachment #771540 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 771552 [details] [diff] [review]
Don't show recording indicator when using fake sources
(Assignee)

Updated

5 years ago
Attachment #771552 - Flags: review?(rjesup)

Updated

5 years ago
Attachment #771552 - Flags: review?(rjesup) → review+
(Assignee)

Updated

5 years ago
Attachment #771552 - Flags: checkin?(rjesup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b5b54a57cd

We can consider uplifting this to Aurora.  Not critical.  Very simple patch though
Whiteboard: [getUserMedia] [blocking-gum-]
Target Milestone: --- → mozilla25

Updated

5 years ago
Attachment #771552 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/67b5b54a57cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.