Bug 146862 – Adopt AVPlayerLayerView

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jeremy Jones]

Adopt AVPlayerLayerView

[Jeremy Jones]

Created attachment 256628 [details]
Patch

[Jeremy Jones]

Created attachment 256778 [details]
Patch

[Jeremy Jones]

Created attachment 256779 [details]
Patch

[Jeremy Jones]

rdar://problem/21661808

[Jeremy Jones]

Need to add derivation of AVPictureInPicturePlayerLayerView and implement -pictureInPicturePlayerLayerView.

[Jeremy Jones]

Created attachment 257085 [details]
Patch

[WebKit Commit Bot]

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.

[Jer Noble]

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.

[Jeremy Jones]

(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.

[Simon Fraser (smfr)]

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>&&?

[Jeremy Jones]

Created attachment 257120 [details]
Patch

[Jeremy Jones]

(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.

[Jeremy Jones]

Created attachment 257121 [details]
Patch for landing.

[Jeremy Jones]

Created attachment 257125 [details]
Patch for landing.

[Jeremy Jones]

Created attachment 257127 [details]
Patch for landing.

[WebKit Commit Bot]

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

[Jeremy Jones]

Created attachment 257131 [details]
Patch for landing.

[WebKit Commit Bot]

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

[Jeremy Jones]

Created attachment 257134 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 257134 [details]
Patch for landing.

Clearing flags on attachment: 257134

Committed r187044: <http://trac.webkit.org/changeset/187044>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.