Bug 43239 (HTMLMediaCaptureAPI)

Summary: Support for HTML Media Capture API
Product: WebKit Reporter: jvn <jvnforums10>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: 709922234, agold, chrisjshull, commit-queue, eric.carlson, fred.wang, glenn, gruan, jay, jonlee, js45.yang, jvnforums10, mcatanzaro, mdaiter, mitz, rcombs, sam, skyul, syoichi, thorton, tonikitoo, tonyg, webkit-bug-importer, wonsuk11.lee
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.w3.org/TR/2011/WD-html-media-capture-20110414/
See Also: https://bugs.webkit.org/show_bug.cgi?id=158799
Bug Depends on:    
Bug Blocks: 78947    
Attachments:
Description Flags
Proposed patch.
none
Proposed patch V2
none
Proposed patc v3
none
Proposed patch v3
thorton: review-
Proposed Patch V4
none
Proposed Patch V4
none
Proposed Patch V4
none
Proposed Patch v5
none
Proposed Patch V6
none
Proposed Patch V7
none
Proposed Patch V8
none
Proposed Patch V9
sam: review-
Proposed Patch v10
none
Rebased patch.
thorton: review+
Fixed Patch.
none
Fixed Patch.
commit-queue: commit-queue-
Fixed Patch. none

Description jvn 2010-07-30 04:23:09 PDT
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/
Comment 1 Tony Gentilcore 2010-08-06 09:40:59 PDT
*** Bug 16474 has been marked as a duplicate of this bug. ***
Comment 2 Glenn Adams 2012-12-13 19:57:40 PST
*** Bug 58184 has been marked as a duplicate of this bug. ***
Comment 3 Glenn Adams 2012-12-13 19:59:22 PST
(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/
Comment 4 Jon Lee 2015-06-22 17:01:40 PDT
rdar://problem/9911008
Comment 5 Alexey Proskuryakov 2016-06-15 16:48:19 PDT
*** Bug 158800 has been marked as a duplicate of this bug. ***
Comment 6 Andrew Gold 2016-11-04 17:03:14 PDT
Created attachment 293962 [details]
Proposed patch.
Comment 7 Tim Horton 2016-11-07 14:35:33 PST
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?
Comment 8 Andrew Gold 2016-11-08 13:48:42 PST
(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.
Comment 9 Andrew Gold 2016-11-09 17:52:59 PST
Created attachment 294315 [details]
Proposed patch V2
Comment 10 WebKit Commit Bot 2016-11-10 17:12:10 PST
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.
Comment 11 Eric Carlson 2016-11-10 17:24:03 PST
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.
Comment 12 Andrew Gold 2016-11-11 14:02:39 PST
Created attachment 294531 [details]
Proposed patc v3
Comment 13 Andrew Gold 2016-11-11 14:03:40 PST
Created attachment 294532 [details]
Proposed patch v3
Comment 14 Tim Horton 2016-11-11 14:38:43 PST
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.
Comment 15 Tim Horton 2016-11-11 14:39:51 PST
You may want to have Sam or Anders sit down and chat about this before going crazy making those changes, they will have Opinions.
Comment 16 Tim Horton 2016-11-11 14:52:05 PST
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.
Comment 17 Andrew Gold 2016-11-14 10:48:51 PST
Created attachment 294713 [details]
Proposed Patch V4
Comment 18 Andrew Gold 2016-11-14 10:53:36 PST
Created attachment 294716 [details]
Proposed Patch V4
Comment 19 Andrew Gold 2016-11-14 12:09:33 PST
Hm my patch still seems to be failing when building for Mac. I'm going to make a few changes and try again.
Comment 20 Andrew Gold 2016-11-14 12:11:45 PST
Created attachment 294726 [details]
Proposed Patch V4
Comment 21 Andrew Gold 2016-11-14 14:20:59 PST
Created attachment 294743 [details]
Proposed Patch v5

Thanks to a little help from Eric, I think this one is going to be all green!
Comment 22 Andrew Gold 2016-11-14 15:03:36 PST
Created attachment 294754 [details]
Proposed Patch V6

Okay, let's try that again.
Comment 23 Andrew Gold 2016-11-14 15:34:04 PST
Created attachment 294760 [details]
Proposed Patch V7

Aaaaand again.
Comment 24 Andrew Gold 2016-11-14 17:07:00 PST
Created attachment 294777 [details]
Proposed Patch V8

Added the proper API annotations.
Comment 25 Tim Horton 2016-11-15 13:32:54 PST
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?
Comment 26 Andrew Gold 2016-11-15 13:51:49 PST
Created attachment 294874 [details]
Proposed Patch V9
Comment 27 Sam Weinig 2016-11-17 15:16:00 PST
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.
Comment 28 Andrew Gold 2016-11-17 15:25:31 PST
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.
Comment 29 Andrew Gold 2016-11-17 17:00:25 PST
Created attachment 295111 [details]
Proposed Patch v10
Comment 30 Andrew Gold 2016-11-17 17:11:14 PST
Forgot to rebase like an amateur. Fixed patch coming soon.
Comment 31 Eric Carlson 2016-11-18 06:45:58 PST
Created attachment 295147 [details]
Rebased patch.
Comment 32 Tim Horton 2016-11-18 14:08:23 PST
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.
Comment 33 Tim Horton 2016-11-18 14:08:48 PST
Also, tests?
Comment 34 Andrew Gold 2016-11-28 11:04:39 PST
Created attachment 295504 [details]
Fixed Patch.
Comment 35 WebKit Commit Bot 2016-11-28 11:10:56 PST
Comment on attachment 295504 [details]
Fixed Patch.

Rejecting attachment 295504 [details] from commit-queue.

agold@apple.com 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.
Comment 36 WebKit Commit Bot 2016-11-28 11:57:05 PST
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.
Comment 37 Andrew Gold 2016-11-28 12:01:33 PST
Created attachment 295508 [details]
Fixed Patch.
Comment 38 WebKit Commit Bot 2016-11-28 12:05:17 PST
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
Comment 39 Andrew Gold 2016-11-28 12:07:57 PST
Created attachment 295510 [details]
Fixed Patch.
Comment 40 Andrew Gold 2016-11-28 12:18:30 PST
(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
Comment 41 WebKit Commit Bot 2016-11-28 12:47:02 PST
Comment on attachment 295510 [details]
Fixed Patch.

Clearing flags on attachment: 295510

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