Summary: | Decrease flicker when enter and exit fullscreen. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Jones <jeremyj-wk> | ||||||||
Component: | Media | Assignee: | Jeremy Jones <jeremyj-wk> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, mitz, philipj, sergio, simon.fraser | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | iPhone / iPad | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Jeremy Jones
2014-07-14 23:38:48 PDT
Created attachment 234908 [details]
Patch
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 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? (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() (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. Created attachment 235192 [details]
Patch for landing.
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 Created attachment 235198 [details]
Patch for landing.
Comment on attachment 235198 [details] Patch for landing. Clearing flags on attachment: 235198 Committed r171286: <http://trac.webkit.org/changeset/171286> |