Bug 134919 – Decrease flicker when enter and exit fullscreen.

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

[Jeremy Jones]

Decrease flicker when enter and exit fullscreen.

[Jeremy Jones]

<rdar://problem/17351830>

[Jeremy Jones]

Created attachment 234908 [details]
Patch

[Eric Carlson]

Comment on attachment 234908 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:964
>      if (m_videoFullscreenLayer && m_videoLayer) {
> -        CGRect frame = CGRectMake(0, 0, m_videoFullscreenFrame.width(), m_videoFullscreenFrame.height());
> -        [m_videoLayer setFrame:frame];
> +        [CATransaction begin];
> +        [CATransaction setDisableActions:YES];
> +        [m_videoLayer setFrame:[m_videoFullscreenLayer bounds]];
> +        [m_videoLayer removeFromSuperlayer];
>          [m_videoFullscreenLayer insertSublayer:m_videoLayer.get() atIndex:0];
> -    }
> +        [CATransaction commit];
> +    } else if (m_videoInlineLayer && m_videoLayer) {
> +        [CATransaction begin];
> +        [CATransaction setDisableActions:YES];
> +        [m_videoLayer setFrame:[m_videoInlineLayer bounds]];
> +        [m_videoLayer removeFromSuperlayer];
> +        [m_videoInlineLayer insertSublayer:m_videoLayer.get() atIndex:0];
> +        [CATransaction commit];
> +    } else if (m_videoLayer)
> +        [m_videoLayer removeFromSuperlayer];

Nit: You could just wrap this block with [CATransaction begin] ... [CATransaction commit]

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:179
> +// HELP! Where should I put this? It is also declared in MediaPlayerPrivateAVFoundationObjC.mm

Oops, this shouldn't be here.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:185
> +@interface WebVideoContainerLayer : CALayer
> +@end
> +
>  PlatformCALayer::LayerType PlatformCALayerMac::layerTypeForPlatformLayer(PlatformLayer* layer)
>  {
> -    if ([layer isKindOfClass:getAVPlayerLayerClass()])
> +    if ([layer isKindOfClass:getAVPlayerLayerClass()] || [layer isKindOfClass:[WebVideoContainerLayer class]])

This is ugly... object_getClassName() isn't a lot better, but at least you don't have to declare the interface here.

[Simon Fraser (smfr)]

Comment on attachment 234908 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        is as easy as adding and removig it from a containter layer; no need to do a layout.

removig

> Source/WebCore/ChangeLog:11
> +        Make sure fullscreen layers are transpreant before moving moving the AVPlayerLayer

transpreant

> Source/WebKit2/ChangeLog:8
> +        Change the sequence of teartdown and use transparency to prevent flicker when entering and exiting fullscreen.

teartdown

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:89
> +@interface WebVideoContainerLayer : CALayer { }

I don't think you need the { }

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:93
> +-(void)setBounds:(CGRect)bounds {

Space after the @implementation line. { on a new line.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:96
> +    for (CALayer* l in self.sublayers)
> +        l.frame = bounds;

Please don't abbreviate layer to l.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:624
>      m_videoLayer = 0;
> +    m_videoInlineLayer = 0;

These should be nil

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:181
> +@interface WebVideoContainerLayer : CALayer
> +@end

Nor should this.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:36
> +#import <WebCore/WAKWindow.h>

Why this?

[Jeremy Jones]

(In reply to comment #3)
> (From update of attachment 234908 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234908&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:964
> >      if (m_videoFullscreenLayer && m_videoLayer) {
> > -        CGRect frame = CGRectMake(0, 0, m_videoFullscreenFrame.width(), m_videoFullscreenFrame.height());
> > -        [m_videoLayer setFrame:frame];
> > +        [CATransaction begin];
> > +        [CATransaction setDisableActions:YES];
> > +        [m_videoLayer setFrame:[m_videoFullscreenLayer bounds]];
> > +        [m_videoLayer removeFromSuperlayer];
> >          [m_videoFullscreenLayer insertSublayer:m_videoLayer.get() atIndex:0];
> > -    }
> > +        [CATransaction commit];
> > +    } else if (m_videoInlineLayer && m_videoLayer) {
> > +        [CATransaction begin];
> > +        [CATransaction setDisableActions:YES];
> > +        [m_videoLayer setFrame:[m_videoInlineLayer bounds]];
> > +        [m_videoLayer removeFromSuperlayer];
> > +        [m_videoInlineLayer insertSublayer:m_videoLayer.get() atIndex:0];
> > +        [CATransaction commit];
> > +    } else if (m_videoLayer)
> > +        [m_videoLayer removeFromSuperlayer];
> 
> Nit: You could just wrap this block with [CATransaction begin] ... [CATransaction commit]

Done.

> 
> > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:179
> > +// HELP! Where should I put this? It is also declared in MediaPlayerPrivateAVFoundationObjC.mm
> 
> Oops, this shouldn't be here.

Removed.

> 
> > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:185
> > +@interface WebVideoContainerLayer : CALayer
> > +@end
> > +
> >  PlatformCALayer::LayerType PlatformCALayerMac::layerTypeForPlatformLayer(PlatformLayer* layer)
> >  {
> > -    if ([layer isKindOfClass:getAVPlayerLayerClass()])
> > +    if ([layer isKindOfClass:getAVPlayerLayerClass()] || [layer isKindOfClass:[WebVideoContainerLayer class]])
> 
> This is ugly... object_getClassName() isn't a lot better, but at least you don't have to declare the interface here.

Ok, removed declaration and switched to objc_getClass()

[Jeremy Jones]

(In reply to comment #4)
> (From update of attachment 234908 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234908&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        is as easy as adding and removig it from a containter layer; no need to do a layout.
> 
> removig

-> removing

> 
> > Source/WebCore/ChangeLog:11
> > +        Make sure fullscreen layers are transpreant before moving moving the AVPlayerLayer
> 
> transpreant

-> transparent

> 
> > Source/WebKit2/ChangeLog:8
> > +        Change the sequence of teartdown and use transparency to prevent flicker when entering and exiting fullscreen.
> 
> teartdown

-> tear down

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:89
> > +@interface WebVideoContainerLayer : CALayer { }
> 
> I don't think you need the { }

Removed.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:93
> > +-(void)setBounds:(CGRect)bounds {
> 
> Space after the @implementation line. { on a new line.

New lines added.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:96
> > +    for (CALayer* l in self.sublayers)
> > +        l.frame = bounds;
> 
> Please don't abbreviate layer to l.

l -> layer

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:624
> >      m_videoLayer = 0;
> > +    m_videoInlineLayer = 0;
> 
> These should be nil

Changed to nil.

> 
> > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:181
> > +@interface WebVideoContainerLayer : CALayer
> > +@end
> 
> Nor should this.

Deleted.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:36
> > +#import <WebCore/WAKWindow.h>
> 
> Why this?

Deleted.

[Jeremy Jones]

Created attachment 235192 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 235192 [details]
Patch for landing.

Rejecting attachment 235192 [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', 'validate-changelog', '--check-oops', '--non-interactive', 235192, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

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

Full output: http://webkit-queues.appspot.com/results/5222228933214208

[Jeremy Jones]

Created attachment 235198 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 235198 [details]
Patch for landing.

Clearing flags on attachment: 235198

Committed r171286: <http://trac.webkit.org/changeset/171286>