Bug 140897 - Prevent flicker when exiting fullscreen by synchronizing transactions.
Summary: Prevent flicker when exiting fullscreen by synchronizing transactions.
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:
Depends on:
Blocks:
 
Reported: 2015-01-26 12:51 PST by Jeremy Jones
Modified: 2015-02-03 15:41 PST (History)
3 users (show)

See Also:


Attachments
Patch (4.64 KB, patch)
2015-01-26 12:57 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (4.84 KB, patch)
2015-01-29 12:59 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2015-02-02 11:57 PST, 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 2015-01-26 12:51:35 PST
Prevent flicker when exiting fullscreen by synchronizing transactions.
Comment 1 Jeremy Jones 2015-01-26 12:57:55 PST
Created attachment 245368 [details]
Patch
Comment 2 Jeremy Jones 2015-01-29 12:59:36 PST
Created attachment 245642 [details]
Patch
Comment 3 Tim Horton 2015-01-30 15:01:39 PST
Comment on attachment 245642 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Synchronize across CAContexts when moving the video layer between layer hierarcies.

spelling "hierarchies"

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1063
> +    for (CAContext* context in [CAContext allContexts]) {

star's on the wrong side

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1064
> +        if (!fencePort)

I'm not sure it's appropriate to fence all CAContexts, is it? We really just want to hit WebKit's and the remote layer's, right?
Also, is the UIProcess' context really accessible from here? That seems vaguely surprising.

> Source/WebCore/platform/spi/cocoa/QuartzCoreSPI.h:60
> ++ (NSArray*)allContexts;

star's on the wrong side
Comment 4 Tim Horton 2015-01-30 15:02:10 PST
Oh, maybe you're synchronizing between two WebProcess-owned video-hosting CAContexts? In that case, can't you just hit both of them?
Comment 5 Jeremy Jones 2015-02-02 11:52:57 PST
(In reply to comment #3)
> Comment on attachment 245642 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245642&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Synchronize across CAContexts when moving the video layer between layer hierarcies.
> 
> spelling "hierarchies"

Fixed.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1063
> > +    for (CAContext* context in [CAContext allContexts]) {
> 
> star's on the wrong side

Fixed.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1064
> > +        if (!fencePort)
> 
> I'm not sure it's appropriate to fence all CAContexts, is it? We really just
> want to hit WebKit's and the remote layer's, right?
> Also, is the UIProcess' context really accessible from here? That seems
> vaguely surprising.

Now finds the root layer and and compares that against the CAContext's layer to make sure it fences necessary CAContexts.

> 
> > Source/WebCore/platform/spi/cocoa/QuartzCoreSPI.h:60
> > ++ (NSArray*)allContexts;
> 
> star's on the wrong side

Fixed.
Comment 6 Jeremy Jones 2015-02-02 11:54:00 PST
(In reply to comment #4)
> Oh, maybe you're synchronizing between two WebProcess-owned video-hosting
> CAContexts? In that case, can't you just hit both of them?

Exactly. There isn't a good way to go from CALayer to CAContext, so it now tries to match the root layer against the CAContext's layer.
Comment 7 Jeremy Jones 2015-02-02 11:57:13 PST
Created attachment 245891 [details]
Patch
Comment 8 Tim Horton 2015-02-02 12:23:40 PST
Comment on attachment 245891 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1066
> +        newRootLayer = m_videoInlineLayer.get();

It's too bad you don't have the PlatformCALayer(Remote) here and can't acquire the contexts directly from those. Is it possible to make that happen? It would be much prettier than this.
Comment 9 Jeremy Jones 2015-02-03 14:58:18 PST
(In reply to comment #8)
> Comment on attachment 245891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245891&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1066
> > +        newRootLayer = m_videoInlineLayer.get();
> 
> It's too bad you don't have the PlatformCALayer(Remote) here and can't
> acquire the contexts directly from those. Is it possible to make that
> happen? It would be much prettier than this.

Agree, but I can't think of an elegant way to get the appropriate contexts down to this layer. I'll file another bug to investigate that.
Comment 10 WebKit Commit Bot 2015-02-03 15:41:53 PST
Comment on attachment 245891 [details]
Patch

Clearing flags on attachment: 245891

Committed r179574: <http://trac.webkit.org/changeset/179574>
Comment 11 WebKit Commit Bot 2015-02-03 15:41:56 PST
All reviewed patches have been landed.  Closing bug.