Bug 192379 – [MediaStream] Cleanup up Mac screen capture class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

Minor cleanup.

[Radar WebKit Bug Importer]

<rdar://problem/46465458>

[Eric Carlson]

Created attachment 356532 [details]
Patch

[youenn fablet]

Comment on attachment 356532 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        (WebCore::DisplayCaptureManagerCocoa::captureDevices): Initialize Display devices first.

Maybe document why we need to do this

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:156
>              weakThis->frameAvailable(status, displayTime, frameSurface, updateRef);

This is existing code but the weakThis check is not thread safe here. weakThis may be dead between if and next call.

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:-167
> -        });

Do we need the call to frameAvailable to be in a background thread?

[Eric Carlson]

Created attachment 356632 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 356632 [details]
Patch for landing.

Clearing flags on attachment: 356632

Committed r238904: <https://trac.webkit.org/changeset/238904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 356632 [details]
Patch for landing.

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

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:144
> +        CFDictionaryRef streamOptions = (__bridge CFDictionaryRef) @{

Doing the cast here may be incompatible with ARC. Instead the local variable should be an NSDictionary * and the cast to CFDictionaryRef should go below at the call site. Or we can use two local variables.

> Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:148
> +            (__bridge NSString *)kCGDisplayStreamMinimumFrameTime :@(1 / frameRate()),
> +            (__bridge NSString *)kCGDisplayStreamQueueDepth:@(screenQueueMaximumLength),
> +            (__bridge NSString *)kCGDisplayStreamColorSpace:(__bridge id)sRGBColorSpaceRef(),
> +            (__bridge NSString *)kCGDisplayStreamShowCursor : @(YES),

Formatting here is a little messy, inconsistent about whether there is a space before and after the colon.

[Eric Carlson]

(In reply to Darin Adler from comment #7)
> Comment on attachment 356632 [details]
> Patch for landing.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356632&action=review
> 
> > Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:144
> > +        CFDictionaryRef streamOptions = (__bridge CFDictionaryRef) @{
> 
> Doing the cast here may be incompatible with ARC. Instead the local variable
> should be an NSDictionary * and the cast to CFDictionaryRef should go below
> at the call site. Or we can use two local variables.
> 
> > Source/WebCore/platform/mediastream/mac/ScreenDisplayCaptureSourceMac.mm:148
> > +            (__bridge NSString *)kCGDisplayStreamMinimumFrameTime :@(1 / frameRate()),
> > +            (__bridge NSString *)kCGDisplayStreamQueueDepth:@(screenQueueMaximumLength),
> > +            (__bridge NSString *)kCGDisplayStreamColorSpace:(__bridge id)sRGBColorSpaceRef(),
> > +            (__bridge NSString *)kCGDisplayStreamShowCursor : @(YES),
> 
> Formatting here is a little messy, inconsistent about whether there is a
> space before and after the colon.

Thanks, I will fix these in bug 192514.