Bug 136870 – Improve fullscreen video rotation animation.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Improve fullscreen video rotation animation.

[Jeremy Jones]

Created attachment 238213 [details]
Patch

[Jeremy Jones]

rdar://problem/17950149

[Simon Fraser (smfr)]

Comment on attachment 238213 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:486
> +    RetainPtr<CALayer> _videoSublayer;

WebAVVideoLayer has a videoSublayer? Needs some comments so we know which layers are which.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:538
> +    [CATransaction setAnimationDuration:[self animationForKey:@"bounds"].duration ?:0.0001];

?:0.0001 is weird, both the magic number and the ?: This needs an explanatory comment.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:539
> +    [CATransaction setAnimationTimingFunction:[CAMediaTimingFunction functionWithName:kCAMediaTimingFunctionEaseInEaseOut]];

Do you want to explicitly end the transaction after setting the properties?

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:116
> +    WKCAContextSetFencePort(m_context.get(), fencePort);

I don't see WKCAContextSetFencePort anywhere. I assume this depends on other changes?

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:43
> +SOFT_LINK_FRAMEWORK(UIKit)
> +SOFT_LINK_CLASS(UIKit, UIWindow)

WebKit2 doesn't need to soft link with UIKit.

[Jeremy Jones]

Created attachment 238263 [details]
Patch

[Jeremy Jones]

(In reply to comment #3)
> (From update of attachment 238213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238213&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:486
> > +    RetainPtr<CALayer> _videoSublayer;
> 
> WebAVVideoLayer has a videoSublayer? Needs some comments so we know which layers are which.

Not sure what to say here. This just formalized the relationship that already existed. See the change from addSublayer: to setVideoSublayer:

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:538
> > +    [CATransaction setAnimationDuration:[self animationForKey:@"bounds"].duration ?:0.0001];
> 
> ?:0.0001 is weird, both the magic number and the ?: This needs an explanatory comment.
> 

Comment added.

> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:539
> > +    [CATransaction setAnimationTimingFunction:[CAMediaTimingFunction functionWithName:kCAMediaTimingFunctionEaseInEaseOut]];
> 
> Do you want to explicitly end the transaction after setting the properties?

Added transaction begin/commit.

> 
> > Source/WebKit2/Platform/mac/LayerHostingContext.mm:116
> > +    WKCAContextSetFencePort(m_context.get(), fencePort);
> 
> I don't see WKCAContextSetFencePort anywhere. I assume this depends on other changes?

I removed this and moved LayerHostingContext off of WKCAContext per AVSystemInterface guidelines.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:43
> > +SOFT_LINK_FRAMEWORK(UIKit)
> > +SOFT_LINK_CLASS(UIKit, UIWindow)
> 
> WebKit2 doesn't need to soft link with UIKit.

Removed.

[Simon Fraser (smfr)]

Comment on attachment 238263 [details]
Patch

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

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:51
> +    layerHostingContext->m_context = (CAContext*)WKCAContextMakeRemoteWithServerPort(serverPort);

CAContext *

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:86
> +    return [m_context setLayer:rootLayer];

no return.

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:102
> +    return [m_context invalidate];

no return

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:107
> +    return [m_context setColorSpace:colorSpace];

no return

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:37
> +#include <UIKit/UIWindow_Private.h>

This will annoy dbates who is trying to make us build with the public SDK. You should do the platform/spi thing.

[Jeremy Jones]

Created attachment 238283 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 238283 [details]
Patch

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

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:41
> +@interface CAContext : NSObject (Details)
> +- (void)invalidate;
> +@property (readonly) uint32_t contextId;
> +@property (strong) CALayer *layer;
> +@property CGColorSpaceRef colorSpace;
> +- (void)setFencePort:(mach_port_t)port;
>  + (CAContext *)remoteContextWithOptions:(NSDictionary *)dict;
>  @end

This should really go into its own spi header, but that can happen in a later commit.

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:88
> +    return [m_context setLayer:rootLayer];

No return.

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:104
> +    return [m_context invalidate];

No return.

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:109
> +    return [m_context setColorSpace:colorSpace];

No return.

[Jeremy Jones]

Created attachment 238284 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 238284 [details]
Patch

Patch doesn't apply to TOT!

[Jeremy Jones]

(In reply to comment #10)
> (From update of attachment 238284 [details])
> Patch doesn't apply to TOT!

dbates backed out his changes for platform/spi. So I'm removing mine also.

[Jeremy Jones]

Created attachment 238285 [details]
Patch

[Jeremy Jones]

Created attachment 238318 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 238318 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:540
> +    if (animationDuration == 0.0) // a duration of 0 for CA means 0.25. This is a way to approximate 0.

We normally write this as if (!animationDuration)

[Jeremy Jones]

Created attachment 238330 [details]
Patch for landing.

[Jeremy Jones]

(In reply to comment #14)
> (From update of attachment 238318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238318&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:540
> > +    if (animationDuration == 0.0) // a duration of 0 for CA means 0.25. This is a way to approximate 0.
> 
> We normally write this as if (!animationDuration)

Fixed.

[WebKit Commit Bot]

Comment on attachment 238330 [details]
Patch for landing.

Clearing flags on attachment: 238330

Committed r173741: <http://trac.webkit.org/changeset/173741>

[Chris Dumez]

I get the following build error after this change (but the bots seem fine):
Source/WebKit2/Platform/mac/LayerHostingContext.mm:121:16: error: instance method '-setFencePort:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access]
    [m_context setFencePort:fencePort];
               ^~~~~~~~~~~~

Anyone knows what I am missing?

[Simon Fraser (smfr)]

Mac build fixed in https://trac.webkit.org/r173762.

[Chris Dumez]

(In reply to comment #19)
> Mac build fixed in https://trac.webkit.org/r173762.

You rock Simon :) Thanks.