Bug 158148 – Decrease flicker when changing video presentation mode.

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

Decrease flicker when changing video presentation mode.

[Jeremy Jones]

rdar://problem/24476949

[Jeremy Jones]

Created attachment 279949 [details]
Patch

[Jon Lee]

Created attachment 279963 [details]
Patch

Rebaselined Jeremy's patch for EWS to chew on

[Jon Lee]

Comment on attachment 279963 [details]
Patch

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

Needs cleanup.

> Source/WebCore/ChangeLog:24
> +        (Controller.prototype.handlePresentationModeChange): Wait to show placeholder, and notify when placehodler is removed.

Placeholder*

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:841
> +        if (this.presentationMode() == 'picture-in-picture') {

Nit: reverse the logic and return early to reduce indenting.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:842
> +            if (!this.host.isVideoLayerInline) {

Nit: No brace needed.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:844
> +            } else {

Nit: No brace needed.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:845
> +                setTimeout(this.showInlinePlaybackPlaceholderWhenSafe.bind(this), 33);

Is this a standard value to wait before retrying?

> Source/WebCore/html/HTMLMediaElement.cpp:5475
> +{

All of this needs to be wrapped up in #if.

> Source/WebCore/html/HTMLMediaElement.cpp:5478
> +        m_perpareForInlineCompletionHandler();

Spelling error.

Could we name it similarly to the bool, like m_preparedForInlineCompletionHandler? I was initially confused by the difference in tenses.

> Source/WebCore/html/HTMLMediaElement.h:127
> +    bool isVideoLayerInline() { return m_videoFullscreenLayer == nil; };

Causing build errors: needs to be moved to .cpp with alternate path for other platforms.

> Source/WebCore/html/HTMLMediaElement.h:129
> +    bool m_preparedForInline;

Should be moved down with other variables.

> Source/WebCore/html/HTMLMediaElement.h:141
> +    std::function<void()> m_perpareForInlineCompletionHandler;

Spelling error. Should be moved down with other variables.

> Source/WebCore/platform/graphics/avfoundation/objc/VideoFullscreenLayerManager.mm:154
> +    } else

Since it's multiple lines, I'd wrap with braces.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:263
> +        m_model->setVideoFullscreenLayer(nil, [protectdThis, this] {

Misspelled protectedThis!

[Jeremy Jones]

Comment on attachment 279963 [details]
Patch

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

>> Source/WebCore/ChangeLog:24
>> +        (Controller.prototype.handlePresentationModeChange): Wait to show placeholder, and notify when placehodler is removed.
> 
> Placeholder*

Fixed 2x

>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:841
>> +        if (this.presentationMode() == 'picture-in-picture') {
> 
> Nit: reverse the logic and return early to reduce indenting.

Reversed..

>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:842
>> +            if (!this.host.isVideoLayerInline) {
> 
> Nit: No brace needed.

Removed.

>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:844
>> +            } else {
> 
> Nit: No brace needed.

Removed.

>> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:845
>> +                setTimeout(this.showInlinePlaybackPlaceholderWhenSafe.bind(this), 33);
> 
> Is this a standard value to wait before retrying?

This is polling 30x a second :-( . I can make a constant, but is there a way to have HTMLMediaElement call a JS function to update this value instead of having JS poll for it? I don't think I can add an event that is only exposed to the shadow DOM.

>> Source/WebCore/html/HTMLMediaElement.cpp:5475
>> +{
> 
> All of this needs to be wrapped up in #if.

I think this can work without the #if. Platforms that don't need it don't have to use it, but there is nothing platform specific about the implementation.

>> Source/WebCore/html/HTMLMediaElement.cpp:5478
>> +        m_perpareForInlineCompletionHandler();
> 
> Spelling error.
> 
> Could we name it similarly to the bool, like m_preparedForInlineCompletionHandler? I was initially confused by the difference in tenses.

Changed to m_preparedForInlineCompletionHandler

Should I also rename 
   void prepareForInline(std::function<void()> completionHandler = [] { }); 
to
  whenPreparedForInline(...
I'm not sure if we have a pattern for this.

>> Source/WebCore/html/HTMLMediaElement.h:127
>> +    bool isVideoLayerInline() { return m_videoFullscreenLayer == nil; };
> 
> Causing build errors: needs to be moved to .cpp with alternate path for other platforms.

Moved. Added alternate implementation that always returns true.

>> Source/WebCore/html/HTMLMediaElement.h:129
>> +    bool m_preparedForInline;
> 
> Should be moved down with other variables.

Moved down to near m_videoFullscreenLayer.

>> Source/WebCore/html/HTMLMediaElement.h:141
>> +    std::function<void()> m_perpareForInlineCompletionHandler;
> 
> Spelling error. Should be moved down with other variables.

Renamed. Moved down to near m_videoFullscreenLayer.

>> Source/WebCore/platform/graphics/avfoundation/objc/VideoFullscreenLayerManager.mm:154
>> +    } else
> 
> Since it's multiple lines, I'd wrap with braces.

Braces added.

>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:263
>> +        m_model->setVideoFullscreenLayer(nil, [protectdThis, this] {
> 
> Misspelled protectedThis!

Fixed.

[Jeremy Jones]

Created attachment 279982 [details]
Patch

[Jon Lee]

Comment on attachment 279982 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5483
> +void HTMLMediaElement::prepareForInline(std::function<void()> completionHandler)

In a previous patch you asked about renaming this to whenPreparedForInline().

Maybe we could model it like a promise, since it's waiting for something to be resolved (a flag to be set) before invoking the completion handler. So maybe: waitForPreparedForInlineThen(completionHandler)?

[Jeremy Jones]

Created attachment 279988 [details]
Patch

[Jeremy Jones]

Created attachment 279989 [details]
Patch

[Jeremy Jones]

(In reply to comment #7)
> Comment on attachment 279982 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279982&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5483
> > +void HTMLMediaElement::prepareForInline(std::function<void()> completionHandler)
> 
> In a previous patch you asked about renaming this to whenPreparedForInline().
> 
> Maybe we could model it like a promise, since it's waiting for something to
> be resolved (a flag to be set) before invoking the completion handler. So
> maybe: waitForPreparedForInlineThen(completionHandler)?

renamed.

[Jeremy Jones]

Also s/nil/nullptr/ to fix gtk and elf.

[Tim Horton]

Comment on attachment 279989 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:847
> +            setTimeout(this.showInlinePlaybackPlaceholderWhenSafe.bind(this), 33);

33!?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:771
> +    [m_secondaryVideoLayer.get() setPlayer:nil];

No .get() here (or the others).

[Jer Noble]

Comment on attachment 279989 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:847
> +            setTimeout(this.showInlinePlaybackPlaceholderWhenSafe.bind(this), 33);

Magic #?

> Source/WebCore/html/HTMLMediaElement.cpp:5498
> +    return m_videoFullscreenLayer == nil;

!m_videoFullscreenLayer

> Source/WebCore/html/HTMLMediaElement.cpp:5534
> +    return true;

Is this true for ports which don't support the full screen API?

Apart from these nits /questions, lgtm, but this needs a WK2 reviewer.

[Jeremy Jones]

(In reply to comment #12)
> Comment on attachment 279989 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279989&action=review
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:847
> > +            setTimeout(this.showInlinePlaybackPlaceholderWhenSafe.bind(this), 33);
> 
> 33!?

I'll make a constant for it. I have a follow-up change to remove this polling.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:771
> > +    [m_secondaryVideoLayer.get() setPlayer:nil];
> 
> No .get() here (or the others).

Fixed.

[Tim Horton]

(In reply to comment #14)
> (In reply to comment #12)
> > Comment on attachment 279989 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=279989&action=review
> > 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:847
> > > +            setTimeout(this.showInlinePlaybackPlaceholderWhenSafe.bind(this), 33);
> > 
> > 33!?
> 
> I'll make a constant for it. I have a follow-up change to remove this
> polling.

> Apart from these nits /questions, lgtm, but this needs a WK2 reviewer.

As long as ^^ this happens, this is fine with me.

[Jeremy Jones]

(In reply to comment #13)
> Comment on attachment 279989 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279989&action=review
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:847
> > +            setTimeout(this.showInlinePlaybackPlaceholderWhenSafe.bind(this), 33);
> 
> Magic #?

I'll add a constant. There is a follow-up change to remove the polling.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5498
> > +    return m_videoFullscreenLayer == nil;
> 
> !m_videoFullscreenLayer

Done.
 
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5534
> > +    return true;
> 
> Is this true for ports which don't support the full screen API?

Video layer is always inline if there is no fullscreen. Right now, is only used by mediaControlsApple.js And it is only needed if a platform moves the video layer around outside the DOM and coordinate those changes in the DOM.

> 
> Apart from these nits /questions, lgtm, but this needs a WK2 reviewer.

[Jeremy Jones]

Created attachment 280005 [details]
Patch for landing.

[Jeremy Jones]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > Comment on attachment 279989 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=279989&action=review
> > > 
> > > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:847
> > > > +            setTimeout(this.showInlinePlaybackPlaceholderWhenSafe.bind(this), 33);
> > > 
> > > 33!?
> > 
> > I'll make a constant for it. I have a follow-up change to remove this
> > polling.
> 
> > Apart from these nits /questions, lgtm, but this needs a WK2 reviewer.
> 
> As long as ^^ this happens, this is fine with me.

Added. I'm assuming your "TIFWM" is an R+.

[WebKit Commit Bot]

Comment on attachment 280005 [details]
Patch for landing.

Clearing flags on attachment: 280005

Committed r201474: <http://trac.webkit.org/changeset/201474>

[Brent Fulgham]

Comment on attachment 279989 [details]
Patch

Clearing flags now that this bug is landed and closed.