[webkit-reviews] review granted: [Bug 189958] [MediaStream] Add Mac window capture source : [Attachment 350774] 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 189958: [MediaStream] Add Mac window capture source
https://bugs.webkit.org/show_bug.cgi?id=189958

Attachment 350774: Patch

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 350774
  --> https://bugs.webkit.org/attachment.cgi?id=350774
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>.