Bug 237524 – Add a preference to mute video capture in case audio capture gets interrupted

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Add a preference to mute video capture in case audio capture gets interrupted

[youenn fablet]

<rdar://78421282>

[youenn fablet]

Created attachment 453967 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/89902939>

[youenn fablet]

Created attachment 453983 [details]
Patch

[youenn fablet]

Created attachment 453985 [details]
Patch

[youenn fablet]

Created attachment 453987 [details]
Patch

[youenn fablet]

@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?

[Philippe Normand]

(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?

[Philippe Normand]

We do implement MockRealtimeAudioSourceGStreamer::setInterruptedForTesting() though.

[Philippe Normand]

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

[youenn fablet]

> 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?

[Philippe Normand]

(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.

[Eric Carlson]

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?

[youenn fablet]

(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.

[youenn fablet]

Given gtk failures, I'll add back the internals API, we should remove it as a follow-up

[youenn fablet]

Created attachment 454081 [details]
Patch

[youenn fablet]

Created attachment 454092 [details]
Patch for landing

[EWS]

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].

[Geoffrey Garen]

> > 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.