[webkit-reviews] review granted: [Bug 181333] [MediaStream] Add Mac screen capture source : [Attachment 330576] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 181333: [MediaStream] Add Mac screen capture source
https://bugs.webkit.org/show_bug.cgi?id=181333

Attachment 330576: Patch

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




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 330576
  --> https://bugs.webkit.org/attachment.cgi?id=330576
Patch

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

> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:103
> +    auto maxDisplays = displayCount;
> +    CGDirectDisplayID activeDisplays[maxDisplays];
> +    err = CGGetActiveDisplayList(maxDisplays, &(activeDisplays[0]),
&displayCount);
> +    if (err) {
> +	   RELEASE_LOG(Media, "CGGetActiveDisplayList() returned error %d when
trying to get the active display list", (int)err);
> +	   return;
> +    }
> +    if (displayCount > maxDisplays)
> +	   displayCount = maxDisplays;

How could this last test happen? Why would the display count change within this
function?

Also, if displayCount has increased in size, isn't activeDisplays the wrong
length (too short)?

> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:110
> +	   m_displaysInternal.append({ displayID,
CGDisplayIDToOpenGLDisplayMask(displayID) });

Do you ever need to remove a display? Where does m_displaysInternal get reset?

> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:126
> +    for (auto &device : m_displaysInternal) {

Nit auto& not auto &

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.h:95
> +    class DisplaySurface {
> +    public:
> +	   DisplaySurface() = default;
> +	   ~DisplaySurface()
> +	   {
> +	       if (m_surface)
> +		   IOSurfaceDecrementUseCount(m_surface.get());
> +	   }
> +
> +	   DisplaySurface& operator=(IOSurfaceRef surface)
> +	   {
> +	       if (m_surface)
> +		   IOSurfaceDecrementUseCount(m_surface.get());
> +	       if (surface)
> +		   IOSurfaceIncrementUseCount(surface);
> +	       m_surface = surface;
> +	       return *this;
> +	   }
> +
> +	   IOSurfaceRef ioSurface() const { return m_surface.get(); }
> +
> +    private:
> +	   RetainPtr<IOSurfaceRef> m_surface;
> +    };

I was going to ask why you don't use platform/graphics/cocoa/IOSurface but I
don't think you'd get any benefit. Also, it seems those are intended to own a
surface, whereas you're just borrowing one for a while.

The name DisplaySurface through me off a bit though.

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:52
> +static int32_t roundUpToMacroblockMultiple(int32_t size)
> +{
> +    return (size + 15) & ~15;
> +}

I'll take your word for this :)

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:76
> +    if (displayCount > maxDisplays)
> +	   displayCount = maxDisplays;

Same comment here. I must be missing it.

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:83
> +    for (auto display : activeDisplays) {
> +	   if (displayMask == CGDisplayIDToOpenGLDisplayMask(display))
> +	       return display;
> +    }
> +

As discussed on IRC... find_first_of

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:143
> +		   screenWidth = 640;
> +		   screenHeight = 480;

Are these numbers defined somewhere? should you put a fixme in to identify
them?

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:156
> +	   int depth = 6;

What is 6?