<rdar://problem/36323219>

Created attachment 330566 [details] Patch

Created attachment 330570 [details] Patch

Created attachment 330576 [details] Patch

Comment on attachment 330576 [details] 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?

Comment on attachment 330576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330576&action=review >> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:103 >> + 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)? Both are good points, fixed. >> 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? We never remove a display from our list, but if it disappears it is marked as inactive. >> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:126 >> + for (auto &device : m_displaysInternal) { > > Nit auto& not auto & Fixed. >> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:52 >> +} > > I'll take your word for this :) :-) >> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:76 >> + displayCount = maxDisplays; > > Same comment here. I must be missing it. Fixed. >> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:83 >> + > > As discussed on IRC... find_first_of Actually I was wrong, it won't work because activeDisplays is a C-style array. >> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:143 >> + screenHeight = 480; > > Are these numbers defined somewhere? should you put a fixme in to identify them? I guess failing to get width or height should result in a failure. Changed. >> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:156 >> + int depth = 6; > > What is 6? The maximum number of frames to queue. Added a constant.

Created attachment 330592 [details] Patch

Comment on attachment 330592 [details] Patch Clearing flags on attachment: 330592 Committed r226468: <https://trac.webkit.org/changeset/226468>

All reviewed patches have been landed. Closing bug.

Comment on attachment 330592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330592&action=review > Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.h:72 > + class DisplaySurface { Can this just use our IOSurface class? > Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:152 > + static CGColorSpaceRef deviceRGBColorSpace = CGColorSpaceCreateDeviceRGB(); We're trying to avoid use of deviceRGB anywhere now, since it's effectively the same as sRGB. Can you change this to use sRGBColorSpaceRef() from GraphicsContextCG.h?