Bug 146862 - Adopt AVPlayerLayerView
Summary: Adopt AVPlayerLayerView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords:
Depends on:
Blocks: 171514
  Show dependency treegraph
 
Reported: 2015-07-10 17:04 PDT by Jeremy Jones
Modified: 2017-05-01 15:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (17.94 KB, patch)
2015-07-10 17:22 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (18.08 KB, patch)
2015-07-14 10:53 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (18.06 KB, patch)
2015-07-14 11:18 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (60.10 KB, patch)
2015-07-20 03:24 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (61.78 KB, patch)
2015-07-20 13:18 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (61.77 KB, patch)
2015-07-20 13:22 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (61.77 KB, patch)
2015-07-20 14:00 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (62.57 KB, patch)
2015-07-20 14:35 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (62.52 KB, patch)
2015-07-20 15:22 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (62.69 KB, patch)
2015-07-20 15:34 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2015-07-10 17:04:54 PDT
Adopt AVPlayerLayerView
Comment 1 Jeremy Jones 2015-07-10 17:22:31 PDT
Created attachment 256628 [details]
Patch
Comment 2 Jeremy Jones 2015-07-14 10:53:59 PDT
Created attachment 256778 [details]
Patch
Comment 3 Jeremy Jones 2015-07-14 11:18:56 PDT
Created attachment 256779 [details]
Patch
Comment 4 Jeremy Jones 2015-07-14 14:33:00 PDT
rdar://problem/21661808
Comment 5 Jeremy Jones 2015-07-14 15:13:29 PDT
Need to add derivation of AVPictureInPicturePlayerLayerView and implement -pictureInPicturePlayerLayerView.
Comment 6 Jeremy Jones 2015-07-20 03:24:54 PDT
Created attachment 257085 [details]
Patch
Comment 7 WebKit Commit Bot 2015-07-20 03:26:25 PDT
Attachment 257085 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:580:  Should not have spaces around = in property attributes.  [whitespace/property] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:589:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Jer Noble 2015-07-20 11:02:01 PDT
Comment on attachment 257085 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:583
> +@property (nonatomic, copy, nullable) NSDictionary *pixelBufferAttributes;

Is this necessary? I don't see where it's used in this patch.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:750
> +    static Class theClass = nil;
> +    if (!theClass) {
> +        theClass = objc_allocateClassPair(getAVPictureInPicturePlayerLayerViewClass(), "WebAVPictureInPicturePlayerLayerView", 0);

If this method is ever called simultaneously on different threads (i.e., web thread vs. main thread), there's a possibility of a race condition. Perhaps dispatch_once() here instead?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:848
> +    static Class classWebAVPlayerLayerView = nil;
> +    if (!classWebAVPlayerLayerView) {
> +        classWebAVPlayerLayerView = objc_allocateClassPair(getAVPlayerLayerViewClass(), "WebAVPlayerLayerView", 0);

Ditto.
Comment 9 Jeremy Jones 2015-07-20 12:47:31 PDT
(In reply to comment #8)
> Comment on attachment 257085 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257085&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:583
> > +@property (nonatomic, copy, nullable) NSDictionary *pixelBufferAttributes;
> 
> Is this necessary? I don't see where it's used in this patch.

It is necessary to look like an AVPlayerLayer, it is accessed by AVPlayerLayerView in AVKit.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:750
> > +    static Class theClass = nil;
> > +    if (!theClass) {
> > +        theClass = objc_allocateClassPair(getAVPictureInPicturePlayerLayerViewClass(), "WebAVPictureInPicturePlayerLayerView", 0);
> 
> If this method is ever called simultaneously on different threads (i.e., web
> thread vs. main thread), there's a possibility of a race condition. Perhaps
> dispatch_once() here instead?

WebVideoFullscreenInterfaceAVKit is only ever accessed from the main thread. All the jumping between threads happens in WebVideoFullscreenControllerAVKit.

Yes, dispatch_once is the right way to initialize these. I'll change that.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:848
> > +    static Class classWebAVPlayerLayerView = nil;
> > +    if (!classWebAVPlayerLayerView) {
> > +        classWebAVPlayerLayerView = objc_allocateClassPair(getAVPlayerLayerViewClass(), "WebAVPlayerLayerView", 0);
> 
> Ditto.

Ditto.
Comment 10 Simon Fraser (smfr) 2015-07-20 13:18:14 PDT
Comment on attachment 257085 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:328
> +    model->setLayerHostView(view.get());

Why the .get(), since setLayerHostView takes a RetainPtr<WebLayerHostView>&&?
Comment 11 Jeremy Jones 2015-07-20 13:18:31 PDT
Created attachment 257120 [details]
Patch
Comment 12 Jeremy Jones 2015-07-20 13:19:24 PDT
(In reply to comment #10)
> Comment on attachment 257085 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257085&action=review
> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:328
> > +    model->setLayerHostView(view.get());
> 
> Why the .get(), since setLayerHostView takes a RetainPtr<WebLayerHostView>&&?

I'll remove this.
Comment 13 Jeremy Jones 2015-07-20 13:22:18 PDT
Created attachment 257121 [details]
Patch for landing.
Comment 14 Jeremy Jones 2015-07-20 14:00:44 PDT
Created attachment 257125 [details]
Patch for landing.
Comment 15 Jeremy Jones 2015-07-20 14:35:17 PDT
Created attachment 257127 [details]
Patch for landing.
Comment 16 WebKit Commit Bot 2015-07-20 15:07:24 PDT
Comment on attachment 257127 [details]
Patch for landing.

Rejecting attachment 257127 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 257127, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
rce/WebCore/platform/spi/cocoa/AVKitSPI.h
patching file Source/WebCore/platform/spi/cocoa/QuartzCoreSPI.h
Hunk #2 succeeded at 135 (offset 9 lines).
patching file Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h
patching file Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm
patching file Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/5350159965749248
Comment 17 Jeremy Jones 2015-07-20 15:22:59 PDT
Created attachment 257131 [details]
Patch for landing.
Comment 18 WebKit Commit Bot 2015-07-20 15:25:14 PDT
Comment on attachment 257131 [details]
Patch for landing.

Rejecting attachment 257131 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 257131, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
creenModelVideoElement.mm
patching file Source/WebCore/platform/spi/cocoa/AVKitSPI.h
patching file Source/WebCore/platform/spi/cocoa/QuartzCoreSPI.h
patching file Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h
patching file Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm
patching file Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/5774197791916032
Comment 19 Jeremy Jones 2015-07-20 15:34:46 PDT
Created attachment 257134 [details]
Patch for landing.
Comment 20 WebKit Commit Bot 2015-07-20 16:29:08 PDT
Comment on attachment 257134 [details]
Patch for landing.

Clearing flags on attachment: 257134

Committed r187044: <http://trac.webkit.org/changeset/187044>
Comment 21 WebKit Commit Bot 2015-07-20 16:29:12 PDT
All reviewed patches have been landed.  Closing bug.