[webkit-reviews] review granted: [Bug 180156] [MediaStream] Clean up audio and video capture factories : [Attachment 327884] Proposed patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 180156: [MediaStream] Clean up audio and video capture factories
https://bugs.webkit.org/show_bug.cgi?id=180156

Attachment 327884: Proposed patch

https://bugs.webkit.org/attachment.cgi?id=327884&action=review




--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 327884
  --> https://bugs.webkit.org/attachment.cgi?id=327884
Proposed patch

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

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:137
> +   
RealtimeMediaSourceCenter::singleton().videoFactory().unsetActiveSource(*this);

Maybe we can do without this change (see other lower comments) or find a way to
grab the factory that created the source.

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:-64
> -    m_supportedConstraints.setSupportsGroupId(true);

We are going from default supportsGroupId being true to being false.
Is that what we want?

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.h:53
> +    void unsetAudioFactory(RealtimeMediaSource::AudioCaptureFactory&)
override { m_audioFactoryOverride = nullptr; }

should be final.

> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:97
> +   
RealtimeMediaSourceCenter::singleton().videoFactory().unsetActiveSource(*this);

Should be audioFactory probably.
But do we need that change though?

Previously, we had a tight coupling between the factory and the source set as
active/inactive.
Now, we could set an active mock video source to a non-mock video factory if we
are switching dynamically the source center audio factory.
There might be an issue here although probably restricted to people switching
from/to mock while capturing audio?

> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:125
> +   
RealtimeMediaSourceCenter::singleton().videoFactory().setActiveSource(*this);

audioFactory as well if we need that change.