Bug 169227 – Refactor: Allow WebKit2 to override the creation of RealtimeMediaSources

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Refactor: Allow WebKit2 to override the creation of RealtimeMediaSources

[Jer Noble]

Created attachment 303574 [details]
Patch

[Eric Carlson]

Comment on attachment 303574 [details]
Patch

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:77
> +    if (m_audioFactory == &factory)
> +        m_audioFactory = nullptr;

Nit: is it ever legal for factory to not be m_audioFactory? If not, ASSERT(m_audioFactory == &factory) would be helpful

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:88
> +    if (m_videoFactory == &factory)
> +        m_videoFactory = nullptr;

Ditto.

> Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:84
> +        if (device && captureDevice.type() == CaptureDevice::DeviceType::Audio)

It should not be possible to get a non-null device of the wrong type, so ASSERT(!device || (device  && captureDevice.type() == CaptureDevice::DeviceType::Audio))

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:112
> +        AVCaptureDeviceTypedef *device = [getAVCaptureDeviceClass() deviceWithUniqueID:captureDevice.persistentId()];
> +        if (device && captureDevice.type() == CaptureDevice::DeviceType::Video)

Ditto.

[youenn fablet]

Comment on attachment 303574 [details]
Patch

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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:86
> +    class Factory {

This does not apply to all realtime sources but only to capture.
How about renaming it to CaptureFactory?

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:69
> +    virtual RealtimeMediaSource::Factory& defaultVideoFactory() = 0;

Is it needed to have two factories, one for audio and another for video?
Can't we have a single capture factory that would handle audio and video?
The factory class could have two methods instead of one.
It may also contain the default implementation and WebKit2 would override it if needed.

> Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:86
> +        return nullptr;

If you are touching this again, I would try to "return nullptr" first.

[Jer Noble]

(In reply to comment #2)
> Comment on attachment 303574 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303574&action=review
> 
> > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:77
> > +    if (m_audioFactory == &factory)
> > +        m_audioFactory = nullptr;
> 
> Nit: is it ever legal for factory to not be m_audioFactory? If not,
> ASSERT(m_audioFactory == &factory) would be helpful

Imagine a scenario where one client sets a factory to Foo, and another client sets it to Bar. Then the first client tries to unset the factory (which they believe is Foo, but is really Bar). We should probably only unset the factory if it hasn't changed in the meanwhile, which is the intention of this patch.

> > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:88
> > +    if (m_videoFactory == &factory)
> > +        m_videoFactory = nullptr;
> 
> Ditto.
> 
> > Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:84
> > +        if (device && captureDevice.type() == CaptureDevice::DeviceType::Audio)
> 
> It should not be possible to get a non-null device of the wrong type, so
> ASSERT(!device || (device  && captureDevice.type() ==
> CaptureDevice::DeviceType::Audio))

Ok.

> > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:112
> > +        AVCaptureDeviceTypedef *device = [getAVCaptureDeviceClass() deviceWithUniqueID:captureDevice.persistentId()];
> > +        if (device && captureDevice.type() == CaptureDevice::DeviceType::Video)
> 
> Ditto.

[Jer Noble]

(In reply to comment #3)
> Comment on attachment 303574 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303574&action=review
> 
> > Source/WebCore/platform/mediastream/RealtimeMediaSource.h:86
> > +    class Factory {
> 
> This does not apply to all realtime sources but only to capture.
> How about renaming it to CaptureFactory?
> 
> > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:69
> > +    virtual RealtimeMediaSource::Factory& defaultVideoFactory() = 0;
> 
> Is it needed to have two factories, one for audio and another for video?
> Can't we have a single capture factory that would handle audio and video?
> The factory class could have two methods instead of one.

Then overriding only creating of audio or video streams (which is the primary use case) becomes much more complicated.

> It may also contain the default implementation and WebKit2 would override it
> if needed.

The default behavior won't never be in the base class, since it's platform dependent.

> > Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:86
> > +        return nullptr;
> 
> If you are touching this again, I would try to "return nullptr" first.

[Jer Noble]

Created attachment 304358 [details]
Patch for landing

One more revision to get the GTK EWS bot happy.

[Jer Noble]

Created attachment 304393 [details]
Patch for landing

[Jer Noble]

Created attachment 304398 [details]
Patch for landing

[Jer Noble]

Created attachment 304402 [details]
Patch for landing

[Jer Noble]

Created attachment 304406 [details]
Patch for landing

[Build Bot]

Comment on attachment 304406 [details]
Patch for landing

Attachment 304406 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3323337

New failing tests:
imported/w3c/web-platform-tests/cors/status.htm

[Build Bot]

Created attachment 304416 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jer Noble]

Committed r213941: <http://trac.webkit.org/changeset/213941>