Bug 147189 – Fix fullscreen and PiP video animation and sizing regressions.

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

Fix fullscreen and PiP video animation and sizing regressions.

[Jeremy Jones]

rdar://problem/21930899

[Jeremy Jones]

Created attachment 257261 [details]
Patch

[Jer Noble]

Comment on attachment 257261 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:467
> -    RefPtr<WebVideoFullscreenControllerContext> strongThis(this);
> -    RetainPtr<CALayer> videoFullscreenLayer = [m_videoFullscreenView layer];
> -
> -    mach_port_name_t fencePort = [[videoFullscreenLayer context] createFencePort];
> -    
> -    WebThreadRun([strongThis, this, frame, fencePort, videoFullscreenLayer] {
> -        [CATransaction begin];
> -        [CATransaction setAnimationDuration:0];
> -        if (m_model)
> -            m_model->setVideoLayerFrame(frame);
> -        [[videoFullscreenLayer context] setFencePort:fencePort];
> -        mach_port_deallocate(mach_task_self(), fencePort);
> -        [CATransaction commit];
> -    });
> +    if (m_model)
> +        m_model->setVideoLayerFrame(frame);

Is there anything we do inside setVideoLayerFrame() which assumes it's being run on the web thread?

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:532
> +        FloatRect videoLayerFrame = FloatRect(0, 0, videoElementClientRect.width(), videoElementClientRect.height());

You can just do: 

FloatRect videoLayerFrame = FloatRect(FloatPoint(), videoElementClientRect.size());  

Or use constructor syntax rather than assignment. FloatSize is constructible from an IntSize (but not vice-versa).

[Jeremy Jones]

(In reply to comment #3)
> Comment on attachment 257261 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257261&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:467
> > -    RefPtr<WebVideoFullscreenControllerContext> strongThis(this);
> > -    RetainPtr<CALayer> videoFullscreenLayer = [m_videoFullscreenView layer];
> > -
> > -    mach_port_name_t fencePort = [[videoFullscreenLayer context] createFencePort];
> > -    
> > -    WebThreadRun([strongThis, this, frame, fencePort, videoFullscreenLayer] {
> > -        [CATransaction begin];
> > -        [CATransaction setAnimationDuration:0];
> > -        if (m_model)
> > -            m_model->setVideoLayerFrame(frame);
> > -        [[videoFullscreenLayer context] setFencePort:fencePort];
> > -        mach_port_deallocate(mach_task_self(), fencePort);
> > -        [CATransaction commit];
> > -    });
> > +    if (m_model)
> > +        m_model->setVideoLayerFrame(frame);
> 
> Is there anything we do inside setVideoLayerFrame() which assumes it's being
> run on the web thread?

Maybe. Just to be base I'll move the operations that need to be sync'd into the web thread instead of into the UIThread.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:532
> > +        FloatRect videoLayerFrame = FloatRect(0, 0, videoElementClientRect.width(), videoElementClientRect.height());
> 
> You can just do: 
> 
> FloatRect videoLayerFrame = FloatRect(FloatPoint(),
> videoElementClientRect.size());  
> 
> Or use constructor syntax rather than assignment. FloatSize is constructible
> from an IntSize (but not vice-versa).

I thought there had to be something more succinct.

[Jeremy Jones]

Created attachment 257290 [details]
Patch

[Jeremy Jones]

Created attachment 257293 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 257293 [details]
Patch for landing.

Clearing flags on attachment: 257293

Committed r187183: <http://trac.webkit.org/changeset/187183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Build fix: http://trac.webkit.org/changeset/187225