Bug 134919 - Decrease flicker when enter and exit fullscreen.
Summary: Decrease flicker when enter and exit fullscreen.
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: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-14 23:38 PDT by Jeremy Jones
Modified: 2014-07-30 07:43 PDT (History)
11 users (show)

See Also:


Attachments
Patch (15.73 KB, patch)
2014-07-15 00:09 PDT, Jeremy Jones
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch for landing. (15.30 KB, patch)
2014-07-20 16:33 PDT, Jeremy Jones
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for landing. (14.94 KB, patch)
2014-07-20 18:51 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 2014-07-14 23:38:48 PDT
Decrease flicker when enter and exit fullscreen.
Comment 1 Jeremy Jones 2014-07-14 23:39:42 PDT
<rdar://problem/17351830>
Comment 2 Jeremy Jones 2014-07-15 00:09:07 PDT
Created attachment 234908 [details]
Patch
Comment 3 Eric Carlson 2014-07-15 07:29:52 PDT
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.
Comment 4 Simon Fraser (smfr) 2014-07-15 12:10:42 PDT
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?
Comment 5 Jeremy Jones 2014-07-20 15:48:54 PDT
(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()
Comment 6 Jeremy Jones 2014-07-20 15:56:57 PDT
(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.
Comment 7 Jeremy Jones 2014-07-20 16:33:41 PDT
Created attachment 235192 [details]
Patch for landing.
Comment 8 WebKit Commit Bot 2014-07-20 17:51:48 PDT
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
Comment 9 Jeremy Jones 2014-07-20 18:51:45 PDT
Created attachment 235198 [details]
Patch for landing.
Comment 10 WebKit Commit Bot 2014-07-20 19:30:45 PDT
Comment on attachment 235198 [details]
Patch for landing.

Clearing flags on attachment: 235198

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