Bug 43239 – Support for HTML Media Capture API

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[jvn]

Add The Capture API interface to WebKit API.
This API provides access to the audio, image and video capture capabilities of the device.
http://www.w3.org/TR/2010/WD-capture-api-20100401/

[Tony Gentilcore]

*** Bug 16474 has been marked as a duplicate of this bug. ***

[Glenn Adams]

*** Bug 58184 has been marked as a duplicate of this bug. ***

[Glenn Adams]

(In reply to comment #0)
> Add The Capture API interface to WebKit API.
> This API provides access to the audio, image and video capture capabilities of the device.
> http://www.w3.org/TR/2010/WD-capture-api-20100401/

The most recent working draft is located at [1], dated December 2012.

[1] http://www.w3.org/TR/html-media-capture/

[Jon Lee]

rdar://problem/9911008

[Alexey Proskuryakov]

*** Bug 158800 has been marked as a duplicate of this bug. ***

[Andrew Gold]

Created attachment 293962 [details]
Proposed patch.

[Tim Horton]

Comment on attachment 293962 [details]
Proposed patch.

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:67
> +- (void)_webView:(WKWebView *)webVIew checkUserMediaPermissionForURL:(NSURL *)url mainFrameURL:(NSURL *)mainFrameURL frameIdentifier:(NSUInteger)frameIdentifier decisionHandler:(void (^)(NSString *salt, BOOL authorized))decisionHandler WK_API_AVAILABLE(ios(10.3));

s/VI/Vi

> Source/WebKit2/UIProcess/WKUserMediaProviderIOS.h:45
> +@interface WKUserMediaProviderIOS : NSObject

Do you think it would be reasonable to have the same interface on Mac, and call this WKUserMediaProvider, and have it implemented perhaps in platform specific ways (or perhaps shared!).

> Source/WebKit2/UIProcess/WKUserMediaProviderIOS.mm:56
> +-(void)decidePolicyForUserMediaRequestFromOrigin:(SecurityOrigin&)userMediaOrigin topLevelOrigin:(SecurityOrigin&)topLevelOrigin frame:(WebFrameProxy&)frame request:(UserMediaPermissionRequestProxy&)permissionRequest view:(WKWebView*)view {

Space after the -, and before the star in WKWebView*

> Source/WebKit2/UIProcess/WebPageProxy.h:368
> +    PageClient& pageClient() { return m_pageClient; }

This is surprising. The fact that it wasn't here before suggests that maybe it shouldn't be?

[Andrew Gold]

(In reply to comment #7)
> Comment on attachment 293962 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293962&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:67
> > +- (void)_webView:(WKWebView *)webVIew checkUserMediaPermissionForURL:(NSURL *)url mainFrameURL:(NSURL *)mainFrameURL frameIdentifier:(NSUInteger)frameIdentifier decisionHandler:(void (^)(NSString *salt, BOOL authorized))decisionHandler WK_API_AVAILABLE(ios(10.3));
> 
> s/VI/Vi

Done.

> 
> > Source/WebKit2/UIProcess/WKUserMediaProviderIOS.h:45
> > +@interface WKUserMediaProviderIOS : NSObject
> 
> Do you think it would be reasonable to have the same interface on Mac, and
> call this WKUserMediaProvider, and have it implemented perhaps in platform
> specific ways (or perhaps shared!).

Yes! It will have to be an interface and implemented differently on Mac and iOS because the AVFoundation APIs used in the class are platform specific, but I will make this change and implement it for iOS.

> 
> > Source/WebKit2/UIProcess/WKUserMediaProviderIOS.mm:56
> > +-(void)decidePolicyForUserMediaRequestFromOrigin:(SecurityOrigin&)userMediaOrigin topLevelOrigin:(SecurityOrigin&)topLevelOrigin frame:(WebFrameProxy&)frame request:(UserMediaPermissionRequestProxy&)permissionRequest view:(WKWebView*)view {
> 
> Space after the -, and before the star in WKWebView*

Done.

> 
> > Source/WebKit2/UIProcess/WebPageProxy.h:368
> > +    PageClient& pageClient() { return m_pageClient; }
> 
> This is surprising. The fact that it wasn't here before suggests that maybe
> it shouldn't be?

Hm maybe not. I exposed it because the UserMediaPermissionRequestManagerProxy is calling decidePolicyForUserMediaPermissionRequest on the page proxy's uiClient for the C API so I wanted to do something similar for the WebKit2 API, but perhaps it would make more sense to have UserMediaPermissionRequestManagerProxy call decidePolicyForUserMediaPermissionRequest on the page proxy directly and the page proxy can call the appropriate methods on the respective clients. 

Thanks for the review! V2 coming shortly.

[Andrew Gold]

Created attachment 294315 [details]
Proposed patch V2

[WebKit Commit Bot]

Attachment 294315 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKUserMediaCaptureDeviceAuthorizerIOS.mm:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebKit2/UIProcess/ios/WKUserMediaCaptureDeviceAuthorizerIOS.mm:11:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 294315 [details]
Proposed patch V2

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

> Source/WebKit2/UIProcess/PageClient.h:142
> +    virtual WKUserMediaProvider *userMediaProvider() = 0;

You should define WKUserMediaProvider as an abstract C++ API, in a non-ObjC header,  so other ports can implement it.

[Andrew Gold]

Created attachment 294531 [details]
Proposed patc v3

[Andrew Gold]

Created attachment 294532 [details]
Proposed patch v3

[Tim Horton]

Comment on attachment 294532 [details]
Proposed patch v3

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

> Source/WebKit2/UIProcess/Cocoa/WKUserMediaProviderCocoa.mm:67
> +    void (^uiDelegateAuthorizationBlock)(void) = ^ {

Much of this should be in UIDelegate

> Source/WebKit2/UIProcess/Cocoa/WKUserMediaProviderCocoa.mm:73
> +            if ((requiresAudio!= authorizedMicrophone) || (requiresVideo != authorizedCamera)) {

space before the !=

> Source/WebKit2/UIProcess/WKUserMediaProvider.h:40
> +class WKUserMediaProvider {

Now that this is a C++ class in the WebKit namespace you can (should!) drop the prefix.

> Source/WebKit2/UIProcess/WKUserMediaProvider.h:45
> +    virtual void mediaStateDidChange(WebCore::MediaProducer::MediaStateFlags, WKWebView*) = 0;

Very surprised to see WKWebView here.

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:231
> +WKUserMediaProvider& PageClientImpl::userMediaProvider()

This all seems wrong. It should be owned by WebPageProxy, know nothing about WKWebView, and communicate with WKWebView and friends when needed via the PageClient/UIClient/etc.

[Tim Horton]

You may want to have Sam or Anders sit down and chat about this before going crazy making those changes, they will have Opinions.

[Tim Horton]

Comment on attachment 294532 [details]
Proposed patch v3

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

> Source/WebKit2/UIProcess/Cocoa/WKUserMediaProviderCocoa.h:42
> +class WKUserMediaProviderCocoa final : public WKUserMediaProvider {

No WK prefix here either.

> Source/WebKit2/UIProcess/Cocoa/WKUserMediaProviderCocoa.mm:86
> +    void (^cameraAuthorizationBlock)(void) = ^ {
> +        if (requiresVideo) {
> +            [captureDeviceAuthorizer() requestCameraAuthorization:^(BOOL authorized) {

I think instead of this mechanism, this should be the default implementation of the UIDelegate, perhaps.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:3265
> -		7CB16FE41724B9B5007A0A95 /* com.apple.WebKit.plugin-common.sb.in */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = "com.apple.WebKit.plugin-common.sb.in"; sourceTree = "<group>"; };
> +		7CB16FE41724B9B5007A0A95 /* com.apple.WebKit.plugin-common.sb.in */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = "com.apple.WebKit.plugin-common.sb.in"; sourceTree = "<group>"; };

This shouldn't be in this patch.

[Andrew Gold]

Created attachment 294713 [details]
Proposed Patch V4

[Andrew Gold]

Created attachment 294716 [details]
Proposed Patch V4

[Andrew Gold]

Hm my patch still seems to be failing when building for Mac. I'm going to make a few changes and try again.

[Andrew Gold]

Created attachment 294726 [details]
Proposed Patch V4

[Andrew Gold]

Created attachment 294743 [details]
Proposed Patch v5

Thanks to a little help from Eric, I think this one is going to be all green!

[Andrew Gold]

Created attachment 294754 [details]
Proposed Patch V6

Okay, let's try that again.

[Andrew Gold]

Created attachment 294760 [details]
Proposed Patch V7

Aaaaand again.

[Andrew Gold]

Created attachment 294777 [details]
Proposed Patch V8

Added the proper API annotations.

[Tim Horton]

Comment on attachment 294777 [details]
Proposed Patch V8

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

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:325
> +    if (!delegate || !m_uiDelegate.m_delegateMethods.webViewRequestUserMediaAuthorizationForMicrophoneCameraUrlMainFrameURLDecisionHandler) {

URL or Url?

[Andrew Gold]

Created attachment 294874 [details]
Proposed Patch V9

[Sam Weinig]

Comment on attachment 294874 [details]
Proposed Patch V9

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

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:118
> +#if PLATFORM(IOS)
> +        WKUserMediaCaptureDeviceAuthorizerIOS *captureDeviceAuthorizer();
> +        RetainPtr<WKUserMediaCaptureDeviceAuthorizerIOS> m_captureDeviceAuthorizer;
> +#endif

This class really shouldn't have any data members.  Can this be merged into WebPageProxy.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:119
> +        WebCore::MediaProducer::MediaStateFlags m_mediaState { WebCore::MediaProducer::IsNotPlaying };

Ditto.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:375
> +    void (^cameraAuthorizationBlock)(void) = ^ {
> +        if (requiresVideo) {
> +            [captureDeviceAuthorizer() requestCameraAuthorization:^(BOOL authorized) {
> +                if (!authorized)
> +                    request.deny(UserMediaPermissionRequestProxy::UserMediaAccessDenialReason::PermissionDenied);
> +                else
> +                    uiDelegateAuthorizationBlock();
> +            }];
> +        } else
> +            uiDelegateAuthorizationBlock();
> +    };
> +
> +    if (requiresAudio) {
> +        [captureDeviceAuthorizer() requestMicrophoneAuthorization:^(BOOL authorized) {
> +            if (!authorized)
> +                request.deny(UserMediaPermissionRequestProxy::UserMediaAccessDenialReason::PermissionDenied);
> +            else
> +                cameraAuthorizationBlock();
> +        }];
> +    } else
> +        cameraAuthorizationBlock();

Why does this need to be iOS specific?

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:395
> +    [(id <WKUIDelegatePrivate>)delegate _webView:webView checkUserMediaPermissionForURL:requestFrameURL mainFrameURL:mainFrameURL frameIdentifier:frame.frameID() decisionHandler:^(NSString *salt, BOOL authorized) {

It's a bit odd that you pass SecurityOrigin's to this level, but then don't use them, but rather get URLs.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:402
> +void UIDelegate::UIClient::isPlayingAudioDidChange(WebKit::WebPageProxy& page)

It is really weird that isPlayingAudioDidChange is what triggers _webViewDidBeginCaptureSession/_webViewDidEndCaptureSession.  This should be re-worked to be more direct.

> Source/WebKit2/UIProcess/ios/WKUserMediaCaptureDeviceAuthorizerIOS.h:28
> +@interface WKUserMediaCaptureDeviceAuthorizerIOS : NSObject

As this class is not AP/SPII, it should not have the WK prefix.  I am generally confused about why this class is necessary, as it seems to store no state.

[Andrew Gold]

Comment on attachment 294874 [details]
Proposed Patch V9

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

Thanks for the review!

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:118
>> +#endif
> 
> This class really shouldn't have any data members.  Can this be merged into WebPageProxy.

Will move these.

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:119
>> +        WebCore::MediaProducer::MediaStateFlags m_mediaState { WebCore::MediaProducer::IsNotPlaying };
> 
> Ditto.

Ditto.

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:375
>> +        cameraAuthorizationBlock();
> 
> Why does this need to be iOS specific?

This is iOS specific because Mac does not require apps to request permission to use media capture devices.

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:395
>> +    [(id <WKUIDelegatePrivate>)delegate _webView:webView checkUserMediaPermissionForURL:requestFrameURL mainFrameURL:mainFrameURL frameIdentifier:frame.frameID() decisionHandler:^(NSString *salt, BOOL authorized) {
> 
> It's a bit odd that you pass SecurityOrigin's to this level, but then don't use them, but rather get URLs.

Will refactor.

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:402
>> +void UIDelegate::UIClient::isPlayingAudioDidChange(WebKit::WebPageProxy& page)
> 
> It is really weird that isPlayingAudioDidChange is what triggers _webViewDidBeginCaptureSession/_webViewDidEndCaptureSession.  This should be re-worked to be more direct.

This is what is currently being used on Mac to notify the client of a capture session beginning/ending. Should I write a new method and only call that one and let the Mac team know to change their implementation, or should I continue to call this method to let clients using the C API know of a change in state, and also call a new method to notify WK2 clients?

>> Source/WebKit2/UIProcess/ios/WKUserMediaCaptureDeviceAuthorizerIOS.h:28
>> +@interface WKUserMediaCaptureDeviceAuthorizerIOS : NSObject
> 
> As this class is not AP/SPII, it should not have the WK prefix.  I am generally confused about why this class is necessary, as it seems to store no state.

I will remove this class and folds its logic into UIDelegate.

[Andrew Gold]

Created attachment 295111 [details]
Proposed Patch v10

[Andrew Gold]

Forgot to rebase like an amateur. Fixed patch coming soon.

[Eric Carlson]

Created attachment 295147 [details]
Rebased patch.

[Tim Horton]

Comment on attachment 295147 [details]
Rebased patch.

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

So much progress!

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:62
> +- (void)_webView:(WKWebView *)webView checkUserMediaPermissionForURL:(NSURL *)url mainFrameURL:(NSURL *)mainFrameURL frameIdentifier:(NSUInteger)frameIdentifier decisionHandler:(void (^)(NSString *salt, BOOL authorized))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

What's up with the salt parameter?

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:351
> +#if PLATFORM(IOS)

Likely we should have a function that does this work and implement it separately in e.g. new UIDelegateIOS and UIDelegateMac files, doing the different things, to satisfy Sam's comment above. There are also other things around that should be (separately) moved into something like that.

[Tim Horton]

Also, tests?

[Andrew Gold]

Created attachment 295504 [details]
Fixed Patch.

[WebKit Commit Bot]

Comment on attachment 295504 [details]
Fixed Patch.

Rejecting attachment 295504 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Attachment 295504 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:45:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.cpp:6413:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrew Gold]

Created attachment 295508 [details]
Fixed Patch.

[WebKit Commit Bot]

Comment on attachment 295508 [details]
Fixed Patch.

Rejecting attachment 295508 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 295508, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit2/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2584191

[Andrew Gold]

Created attachment 295510 [details]
Fixed Patch.

[Andrew Gold]

(In reply to comment #32)
> Comment on attachment 295147 [details]
> Rebased patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295147&action=review
> 
> So much progress!
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:62
> > +- (void)_webView:(WKWebView *)webView checkUserMediaPermissionForURL:(NSURL *)url mainFrameURL:(NSURL *)mainFrameURL frameIdentifier:(NSUInteger)frameIdentifier decisionHandler:(void (^)(NSString *salt, BOOL authorized))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> What's up with the salt parameter?

It is the string we use to make sure capture device IDs are unique per domain.

> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:351
> > +#if PLATFORM(IOS)
> 
> Likely we should have a function that does this work and implement it
> separately in e.g. new UIDelegateIOS and UIDelegateMac files, doing the
> different things, to satisfy Sam's comment above. There are also other
> things around that should be (separately) moved into something like that.

Filed: https://bugs.webkit.org/show_bug.cgi?id=165101

[WebKit Commit Bot]

Comment on attachment 295510 [details]
Fixed Patch.

Clearing flags on attachment: 295510

Committed r209010: <http://trac.webkit.org/changeset/209010>