Adopt attribution AVCaptureSession SPI for GPU process

• Component: Media
• Status: RESOLVED FIXED

[Kate Cheney]

Adopt attribution AVCaptureSession SPI for GPU process

[Kate Cheney]

Created attachment 440988 [details]
Patch

[Kate Cheney]

rdar://80748535

[Kate Cheney]

Created attachment 440994 [details]
Patch

[Kate Cheney]

Created attachment 441019 [details]
Patch

[Kate Cheney]

Created attachment 441090 [details]
Patch

[Eric Carlson]

Comment on attachment 441090 [details]
Patch

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:50
> +#if USE(APPLE_INTERNAL_SDK)
> +#include <TCC/TCC.h>
> +#endif

#include "TCCSPI.h"

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:51
> +#if USE(APPLE_INTERNAL_SDK)
> +#import <AVFoundation/AVCaptureSession_Private.h>
> +#endif

This should go in AVFoundationSPI.h

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:60
> +@interface AVCaptureSession (AVCaptureSessionPrivate)
> +- (instancetype)initWithAssumedIdentity:(tcc_identity_t)tccIdentity SPI_AVAILABLE(ios(15.0)) API_UNAVAILABLE(macos, macCatalyst, watchos, tvos);
> +@end
> +

Ditto

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:34
> +#include "TCCSoftLink.h"

It looks like this isn't needed.

[Kate Cheney]

(In reply to Eric Carlson from comment #6)
> Comment on attachment 441090 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441090&action=review
> 
> > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:50
> > +#if USE(APPLE_INTERNAL_SDK)
> > +#include <TCC/TCC.h>
> > +#endif
> 
> #include "TCCSPI.h"
> 

TCCSPI.h is in WebKit, I don't think it can be included from WebCore.

> > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:51
> > +#if USE(APPLE_INTERNAL_SDK)
> > +#import <AVFoundation/AVCaptureSession_Private.h>
> > +#endif
> 
> This should go in AVFoundationSPI.h
> 
> > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:60
> > +@interface AVCaptureSession (AVCaptureSessionPrivate)
> > +- (instancetype)initWithAssumedIdentity:(tcc_identity_t)tccIdentity SPI_AVAILABLE(ios(15.0)) API_UNAVAILABLE(macos, macCatalyst, watchos, tvos);
> > +@end
> > +
> 
> Ditto

Ok. Was getting errors trying to include AVFoundationSPI.h in this file. I will try again.

> 
> > Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:34
> > +#include "TCCSoftLink.h"
> 
> It looks like this isn't needed.

Oops, leftover from a previous version. Will remove.

[Eric Carlson]

Comment on attachment 441090 [details]
Patch

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

>>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:50
>>> +#endif
>> 
>> #include "TCCSPI.h"
> 
> TCCSPI.h is in WebKit, I don't think it can be included from WebCore.

You can move it to PAL. `:#if USE(APPLE_INTERNAL_SDK)` should generally only be used in *SPI and *SoftLink files

[Kate Cheney]

Created attachment 441129 [details]
Patch

[Kate Cheney]

Created attachment 441133 [details]
Patch

[Kate Cheney]

Created attachment 441135 [details]
Patch

[Kate Cheney]

Created attachment 441136 [details]
Patch

[Kate Cheney]

Created attachment 441137 [details]
Patch

[Eric Carlson]

Comment on attachment 441137 [details]
Patch

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

Does this work when we capture in the web process?

> Source/WebKit/GPUProcess/cocoa/GPUConnectionToWebProcessCocoa.mm:88
> +std::optional<tcc_identity_t> GPUConnectionToWebProcess::setTCCIdentityInGPUProcess()
> +{
> +    auto auditToken = gpuProcess().parentProcessConnection()->getAuditToken();
> +    if (!auditToken)
> +        return std::nullopt;
> +
> +    NSError *error = nil;
> +    auto bundleProxy = [LSBundleProxy bundleProxyWithAuditToken:*auditToken error:&error];
> +    if (error)
> +        return std::nullopt;
> +
> +    tcc_identity_t identity = nil;
> +    identity = tcc_identity_create(TCC_IDENTITY_CODE_BUNDLE_ID, [bundleProxy.bundleIdentifier UTF8String]);
> +    if (!identity)
> +        return std::nullopt;
> +
> +    WebCore::RealtimeMediaSourceCenter::singleton().setIdentity(identity);
> +
> +    return identity;
> +}

This name is wrong. It is a method on GPUConnectionToWebProcess, so "InGPUProcess" is redundant, and it returns a value so the "set" prefix is confusing. Does it need to return the identity token?

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:353
> +        WebCore::RealtimeMediaSourceCenter::singleton().setIdentity(*identity);

Doesn't setTCCIdentityInGPUProcess already do this?

[Kate Cheney]

Created attachment 441264 [details]
Patch

[Kate Cheney]

(In reply to Eric Carlson from comment #14)
> Comment on attachment 441137 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441137&action=review
> 
> Does this work when we capture in the web process?
> 

When I disable GPU Process for video capture in settings I am unable to use any video capture at all so I am not able to test. This is not related to my patch as it also reproduces without it. I can add a speculative implementation but it might be better to investigate that separately and post a fix after, especially since capture in GPU Process seems to be on by default.

[Eric Carlson]

(In reply to Kate Cheney from comment #16)
> 
> When I disable GPU Process for video capture in settings I am unable to use
> any video capture at all so I am not able to test. This is not related to my
> patch as it also reproduces without it. I can add a speculative
> implementation but it might be better to investigate that separately and
> post a fix after, especially since capture in GPU Process seems to be on by
> default.
>
It is on by default, but the experimental setting is exposed so in theory it should be switchable. 

Please add a FIXME in the place you think the fix should go (UserMediaCaptureManager::setupCaptureProcesses?).

[Eric Carlson]

Comment on attachment 441264 [details]
Patch

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

Thanks!

> Source/WebKit/ChangeLog:14
> +        (WebKit::GPUConnectionToWebProcess::setTCCIdentityInGPUProcess):

This is now incorrect.

> Source/WebKit/ChangeLog:20
> +        (WebKit::UserMediaCaptureManagerProxy::ConnectionProxy::setTCCIdentityInGPUProcess):

Ditto

[Kate Cheney]

Created attachment 441298 [details]
Patch for landing

[Kate Cheney]

(In reply to Eric Carlson from comment #17)
> (In reply to Kate Cheney from comment #16)
> > 
> > When I disable GPU Process for video capture in settings I am unable to use
> > any video capture at all so I am not able to test. This is not related to my
> > patch as it also reproduces without it. I can add a speculative
> > implementation but it might be better to investigate that separately and
> > post a fix after, especially since capture in GPU Process seems to be on by
> > default.
> >
> It is on by default, but the experimental setting is exposed so in theory it
> should be switchable. 
> 
> Please add a FIXME in the place you think the fix should go
> (UserMediaCaptureManager::setupCaptureProcesses?).

Added.

[Kate Cheney]

(In reply to Eric Carlson from comment #18)
> Comment on attachment 441264 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441264&action=review
> 
> Thanks!
> 
> > Source/WebKit/ChangeLog:14
> > +        (WebKit::GPUConnectionToWebProcess::setTCCIdentityInGPUProcess):
> 
> This is now incorrect.
> 
> > Source/WebKit/ChangeLog:20
> > +        (WebKit::UserMediaCaptureManagerProxy::ConnectionProxy::setTCCIdentityInGPUProcess):
> 
> Ditto

Fixed both, thanks for the review.

[EWS]

Committed r284220 (243029@main): <https://commits.webkit.org/243029@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441298 [details].

[Alex Christensen]

r284222

[Alex Christensen]

Comment on attachment 441298 [details]
Patch for landing

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

> Source/WebKit/GPUProcess/cocoa/GPUConnectionToWebProcessCocoa.mm:81
> +    identity = tcc_identity_create(TCC_IDENTITY_CODE_BUNDLE_ID, [bundleProxy.bundleIdentifier UTF8String]);

tcc_identity_create is annotated with OS_OBJECT_RETURNS_RETAINED, which means this is probably a memory leak.  It should probably be auto identity = adoptOSObject(tcc_identity_create(...)); which means my build fix of calling tcc_identity_t will probably also need to be changed to OS_OBJECT_DECL if OS_OBJECT_USE_OBJC is defined.

[David Kilzer (:ddkilzer)]

(In reply to EWS from comment #22)
> Committed r284220 (243029@main): <https://commits.webkit.org/243029@main>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 441298 [details].

(In reply to Alex Christensen from comment #23)
> r284222

Second follow-up fix by Kate for MacCatalyst:

r284251

[David Kilzer (:ddkilzer)]

Reopening to attach new patch.

[David Kilzer (:ddkilzer)]

Created attachment 441549 [details]
Patch for build fix #3

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #27)
> Created attachment 441549 [details]
> Patch for build fix #3

macOS 10.15 Catalina builds are still failing internally (inlining WebKit.framework).

This attempts to fix them.

[David Kilzer (:ddkilzer)]

Comment on attachment 441549 [details]
Patch for build fix #3

Typo: #endf

[David Kilzer (:ddkilzer)]

Created attachment 441550 [details]
Patch for build fix #3 v2

[David Kilzer (:ddkilzer)]

(In reply to Alex Christensen from comment #24)
> Comment on attachment 441298 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441298&action=review
> 
> > Source/WebKit/GPUProcess/cocoa/GPUConnectionToWebProcessCocoa.mm:81
> > +    identity = tcc_identity_create(TCC_IDENTITY_CODE_BUNDLE_ID, [bundleProxy.bundleIdentifier UTF8String]);
> 
> tcc_identity_create is annotated with OS_OBJECT_RETURNS_RETAINED, which
> means this is probably a memory leak.  It should probably be auto identity =
> adoptOSObject(tcc_identity_create(...)); which means my build fix of calling
> tcc_identity_t will probably also need to be changed to OS_OBJECT_DECL if
> OS_OBJECT_USE_OBJC is defined.

Bug 231882: WebKit::GPUConnectionToWebProcess::setTCCIdentity() leaks a tcc_identity_t

[David Kilzer (:ddkilzer)]

Comment on attachment 441550 [details]
Patch for build fix #3 v2

Marking cq+ as WebKit project built on Apple Cocoa platforms.

[EWS]

Committed r284344 (243138@main): <https://commits.webkit.org/243138@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441550 [details].