Bug 189958 – [MediaStream] Add Mac window capture source

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

Add Mac window capture source

[Radar WebKit Bug Importer]

<rdar://problem/44767616>

[Eric Carlson]

Created attachment 350774 [details]
Patch

[youenn fablet]

Comment on attachment 350774 [details]
Patch

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

> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:176
> +    if (m_lastSampleBuffer && m_lastFullSizedPixelBuffer && CFEqual(m_lastFullSizedPixelBuffer.get(), pixelBuffer.get())) {

If we set intrinsic size, we will not reuse m_lastSampleBuffer since it will be nullified.
Can we be in a case where m_initrinsicSize is not changed but this->size() is so that we should not reuse m_lastSampleBuffer?

> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:193
> +        if (m_pixelBufferResizer)

No need for this if check

> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:209
> +            if (m_pixelBufferConformer)

No need for this if check

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:129
> +        RetainPtr<CGDisplayModeRef> displayMode = adoptCF(CGDisplayCopyDisplayMode(m_displayID));

s/RetainPtr<CGDisplayModeRef>/auto ?

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:33
> +#include <wtf/Lock.h>

We probably do not need all these includes, lock does not seem used, DisplayCaptureSourceCocoa.h seems used.
PixelBufferConformerCV could be forward declared .

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:46
> +    WindowDisplayCaptureSourceMac(uint32_t, String&&);

Add name for uint32_t?
Maybe make the constructor private since there are static methods.

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:49
> +    static void windowCaptureDevices(Vector<CaptureDevice>&);

Might be easier to have something like Vector<CaptureDevice> windowCaptureDevices()?

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:58
> +    RetainPtr<CGImageRef> windowImage(bool);

bool is not easy to understand, maybe add the name here, use an enum or rename windowImage to windowImageWithResize?

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.mm:58
> +    CFIndex windowCount = CFArrayGetCount(windows.get());

s/CFIndex/auto/

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.mm:59
> +    for (CFIndex i = 0; i < windowCount; i++) {

Ditto here.

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.mm:155
> +    m_lastImage = image;

WTFMove() and use m_lastImage below, or maybe do not make m_lastImage not retained pointer?
It also seems like we are storing m_lastImage, m_lastFullSizedPixelBuffer and m_lastSampleBuffer for the same reason of optimizing.
Maybe there could be a simpler way?

> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.mm:188
> +        CVReturn status = CVPixelBufferPoolCreate(kCFAllocatorDefault, pixelBufferPoolOptions, sourcePixelBufferOptions, &bufferPool);

Maybe we should have a routine helper here that would take width, height and format type and return a RetainPtr<CVPixelBufferPoolRef>.

[Eric Carlson]

Comment on attachment 350774 [details]
Patch

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

>> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:176
>> +    if (m_lastSampleBuffer && m_lastFullSizedPixelBuffer && CFEqual(m_lastFullSizedPixelBuffer.get(), pixelBuffer.get())) {
> 
> If we set intrinsic size, we will not reuse m_lastSampleBuffer since it will be nullified.
> Can we be in a case where m_initrinsicSize is not changed but this->size() is so that we should not reuse m_lastSampleBuffer?

m_lastSampleBuffer is also nullified in settingsDidChange, which is called when size changes.

>> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:193
>> +        if (m_pixelBufferResizer)
> 
> No need for this if check

Fixed.

>> Source/WebCore/platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp:209
>> +            if (m_pixelBufferConformer)
> 
> No need for this if check

OK

>> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:129
>> +        RetainPtr<CGDisplayModeRef> displayMode = adoptCF(CGDisplayCopyDisplayMode(m_displayID));
> 
> s/RetainPtr<CGDisplayModeRef>/auto ?

Fixed.

>> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:33
>> +#include <wtf/Lock.h>
> 
> We probably do not need all these includes, lock does not seem used, DisplayCaptureSourceCocoa.h seems used.
> PixelBufferConformerCV could be forward declared .

Fixed.

>> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:46
>> +    WindowDisplayCaptureSourceMac(uint32_t, String&&);
> 
> Add name for uint32_t?
> Maybe make the constructor private since there are static methods.

Fixed.

>> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:49
>> +    static void windowCaptureDevices(Vector<CaptureDevice>&);
> 
> Might be easier to have something like Vector<CaptureDevice> windowCaptureDevices()?

It would look better, but they we will need to copy the vector it returns when building the list of window and screen "devices".

>> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.h:58
>> +    RetainPtr<CGImageRef> windowImage(bool);
> 
> bool is not easy to understand, maybe add the name here, use an enum or rename windowImage to windowImageWithResize?

We don't need the option yet, so I just removed it.

>> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.mm:58
>> +    CFIndex windowCount = CFArrayGetCount(windows.get());
> 
> s/CFIndex/auto/

Fixed.

>> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.mm:59
>> +    for (CFIndex i = 0; i < windowCount; i++) {
> 
> Ditto here.

Ditto.

>> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.mm:155
>> +    m_lastImage = image;
> 
> WTFMove() and use m_lastImage below, or maybe do not make m_lastImage not retained pointer?
> It also seems like we are storing m_lastImage, m_lastFullSizedPixelBuffer and m_lastSampleBuffer for the same reason of optimizing.
> Maybe there could be a simpler way?

m_lastImage is a CGImage, which is a retain counted object so we need a retain pointer for it.

>> Source/WebCore/platform/mediastream/mac/WindowDisplayCaptureSourceMac.mm:188
>> +        CVReturn status = CVPixelBufferPoolCreate(kCFAllocatorDefault, pixelBufferPoolOptions, sourcePixelBufferOptions, &bufferPool);
> 
> Maybe we should have a routine helper here that would take width, height and format type and return a RetainPtr<CVPixelBufferPoolRef>.

Good idea, we do this in a number of places. I will do this in a follow up patch.

[Eric Carlson]

Created attachment 350823 [details]
Patch for landing

[EWS Watchlist]

Comment on attachment 350774 [details]
Patch

Attachment 350774 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9350472

New failing tests:
fast/frames/lots-of-objects.html
imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html

[EWS Watchlist]

Created attachment 350827 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Eric Carlson]

(In reply to Build Bot from comment #6)
> Comment on attachment 350774 [details]
> Patch
> 
> Attachment 350774 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/9350472
> 
> New failing tests:
> fast/frames/lots-of-objects.html
> imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-
> normalization.html

This failure is unrelated to the changes in this patch.

[WebKit Commit Bot]

Comment on attachment 350823 [details]
Patch for landing

Clearing flags on attachment: 350823

Committed r236494: <https://trac.webkit.org/changeset/236494>