Bug 237524 - Add a preference to mute video capture in case audio capture gets interrupted
Summary: Add a preference to mute video capture in case audio capture gets interrupted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-07 05:30 PST by youenn fablet
Modified: 2022-04-30 04:54 PDT (History)
12 users (show)

See Also:


Attachments
Patch (43.26 KB, patch)
2022-03-07 05:46 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (53.76 KB, patch)
2022-03-07 07:53 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (53.76 KB, patch)
2022-03-07 07:59 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (57.44 KB, patch)
2022-03-07 08:13 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (49.91 KB, patch)
2022-03-08 00:57 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (49.93 KB, patch)
2022-03-08 03:07 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-03-07 05:30:40 PST
Add a preference to mute video capture in case audio capture gets interrupted
Comment 1 youenn fablet 2022-03-07 05:31:06 PST
<rdar://78421282>
Comment 2 youenn fablet 2022-03-07 05:46:59 PST
Created attachment 453967 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2022-03-07 06:25:01 PST
<rdar://problem/89902939>
Comment 4 youenn fablet 2022-03-07 07:53:54 PST
Created attachment 453983 [details]
Patch
Comment 5 youenn fablet 2022-03-07 07:59:39 PST
Created attachment 453985 [details]
Patch
Comment 6 youenn fablet 2022-03-07 08:13:28 PST
Created attachment 453987 [details]
Patch
Comment 7 youenn fablet 2022-03-07 09:21:14 PST
@philn, I am trying to remove the internals API to interrupt source and use testRunner interruption API which should be closer to how this actually works.

The test currently fails in GTK, probably due to MockRealtimeAudioSource::setIsInterrupted being implemented for Cocoa right now.
Any idea how this could be implemented for GTK?
Comment 8 Philippe Normand 2022-03-07 09:31:08 PST
(In reply to youenn fablet from comment #7)
> @philn, I am trying to remove the internals API to interrupt source and use
> testRunner interruption API which should be closer to how this actually
> works.
> 
> The test currently fails in GTK, probably due to
> MockRealtimeAudioSource::setIsInterrupted being implemented for Cocoa right
> now.
> Any idea how this could be implemented for GTK?

Humm maybe make it so that MockRealtimeAudioSourceGStreamer::render returns early without notifying samples?
Comment 9 Philippe Normand 2022-03-07 09:31:58 PST
We do implement MockRealtimeAudioSourceGStreamer::setInterruptedForTesting() though.
Comment 10 Philippe Normand 2022-03-07 09:34:22 PST
Comment on attachment 453987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453987&action=review

> Source/WebCore/platform/mediastream/gstreamer/MockRealtimeAudioSourceGStreamer.cpp:-143
> -void MockRealtimeAudioSourceGStreamer::setInterruptedForTesting(bool isInterrupted)

Sorry, didn't read the patch. :D
Comment 11 youenn fablet 2022-03-07 09:41:44 PST
> Humm maybe make it so that MockRealtimeAudioSourceGStreamer::render returns
> early without notifying samples?

The main difference is that we are going from an instance method to a static method.
Maybe a static variable that MockRealtimeAudioSourceGStreamer::render would use might do the trick?
Comment 12 Philippe Normand 2022-03-07 09:47:33 PST
(In reply to youenn fablet from comment #11)
> > Humm maybe make it so that MockRealtimeAudioSourceGStreamer::render returns
> > early without notifying samples?
> 
> The main difference is that we are going from an instance method to a static
> method.
> Maybe a static variable that MockRealtimeAudioSourceGStreamer::render would
> use might do the trick?

Yes maybe. Otherwise mark the test as failing and I'll try to fix it later.
Comment 13 Eric Carlson 2022-03-07 11:13:08 PST
Comment on attachment 453987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453987&action=review

r=me once the bots are happy

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:521
> +        if (!source.isEnded() && source.type() == RealtimeMediaSource::Type::Video)
> +            source.setMuted(isMicrophoneInterrupted);

Shouldn't we also mute screen and window capture during and interruption?

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:643
> +    if (wasInterrupted != m_isInterrupted && m_private->source().type() == RealtimeMediaSource::Type::Audio && document->settings().muteCameraOnMicrophoneInterruptionEnabled())
> +        updateVideoCaptureAccordingMicrophoneInterruption(*document, m_isInterrupted);

Won't this run multiple times if we have more than one audio track?
Comment 14 youenn fablet 2022-03-07 11:40:27 PST
(In reply to Eric Carlson from comment #13)
> Comment on attachment 453987 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453987&action=review
> 
> r=me once the bots are happy
> 
> > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:521
> > +        if (!source.isEnded() && source.type() == RealtimeMediaSource::Type::Video)
> > +            source.setMuted(isMicrophoneInterrupted);
> 
> Shouldn't we also mute screen and window capture during and interruption?

I was not sure about that.
Let's continue the discussion on this one.

> > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:643
> > +    if (wasInterrupted != m_isInterrupted && m_private->source().type() == RealtimeMediaSource::Type::Audio && document->settings().muteCameraOnMicrophoneInterruptionEnabled())
> > +        updateVideoCaptureAccordingMicrophoneInterruption(*document, m_isInterrupted);
> 
> Won't this run multiple times if we have more than one audio track?

Yes, it will run once per live microphone track attached to the source.
In general, probably only once but it could be more with cloning.
That is the downside of using tracks but should have no functional impact visible to users.
Comment 15 youenn fablet 2022-03-07 23:39:45 PST
Given gtk failures, I'll add back the internals API, we should remove it as a follow-up
Comment 16 youenn fablet 2022-03-08 00:57:54 PST
Created attachment 454081 [details]
Patch
Comment 17 youenn fablet 2022-03-08 03:07:47 PST
Created attachment 454092 [details]
Patch for landing
Comment 18 EWS 2022-03-08 04:25:37 PST
Committed r290985 (248163@main): <https://commits.webkit.org/248163@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454092 [details].
Comment 19 Geoffrey Garen 2022-03-08 09:49:11 PST
> > Shouldn't we also mute screen and window capture during and interruption?


> I was not sure about that.
> Let's continue the discussion on this one.

FWIW, I tend to agree with the general direction that, if we're going to interrupt, we should interrupt everything. I think that's a simpler mental model for the user, and it matches most UI's I've seen best.