Clean up audio and video capture factories
<rdar://problem/35753435>
Created attachment 327884 [details] Proposed patch
Attachment 327884 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:49: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 327884 [details] 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.
(In reply to youenn fablet from comment #4) > Comment on attachment 327884 [details] > 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); If we expose "static VideoCaptureSourceFactoryMac& videoCaptureSourceFactory()" as a static method of RealtimeMediaSourceCenterMac, we could probably write RealtimeMediaSourceCenterMac::videoCaptureSourceFactory().unsetActiveSource(*this);
Comment on attachment 327884 [details] 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. > > If we expose "static VideoCaptureSourceFactoryMac& videoCaptureSourceFactory()" as a static method of RealtimeMediaSourceCenterMac, we could probably write RealtimeMediaSourceCenterMac::videoCaptureSourceFactory().unsetActiveSource(*this); Good idea, fixed. >> 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? Yes, the we do not support it. >> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.h:53 >> + void unsetAudioFactory(RealtimeMediaSource::AudioCaptureFactory&) override { m_audioFactoryOverride = nullptr; } > > should be final. Fixed. >> 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? Oops, fixed. >> Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:125 >> + RealtimeMediaSourceCenter::singleton().videoFactory().setActiveSource(*this); > > audioFactory as well if we need that change. Fixed.
Created attachment 327912 [details] Patch for landing.
Attachment 327912 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:49: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:48: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 327922 [details] Patch for landing.
Attachment 327922 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:49: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:48: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 327943 [details] Patch for landing.
Attachment 327943 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:49: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:48: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 327943 [details] Patch for landing. Clearing flags on attachment: 327943 Committed r225313: <https://trac.webkit.org/changeset/225313>