Bug 158148 - Decrease flicker when changing video presentation mode.
Summary: Decrease flicker when changing video presentation mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-27 01:28 PDT by Jeremy Jones
Modified: 2016-07-07 12:22 PDT (History)
9 users (show)

See Also:


Attachments
Patch (43.42 KB, patch)
2016-05-27 02:02 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (46.18 KB, patch)
2016-05-27 09:01 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (43.58 KB, patch)
2016-05-27 12:19 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (43.58 KB, patch)
2016-05-27 13:45 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (43.70 KB, patch)
2016-05-27 13:49 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (44.22 KB, patch)
2016-05-27 15:59 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 2016-05-27 01:28:04 PDT
Decrease flicker when changing video presentation mode.
Comment 1 Jeremy Jones 2016-05-27 01:29:32 PDT
rdar://problem/24476949
Comment 2 Jeremy Jones 2016-05-27 02:02:10 PDT
Created attachment 279949 [details]
Patch
Comment 3 Jon Lee 2016-05-27 09:01:46 PDT
Created attachment 279963 [details]
Patch

Rebaselined Jeremy's patch for EWS to chew on
Comment 4 Jon Lee 2016-05-27 10:00:14 PDT
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!
Comment 5 Jeremy Jones 2016-05-27 12:07:58 PDT
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.
Comment 6 Jeremy Jones 2016-05-27 12:19:31 PDT
Created attachment 279982 [details]
Patch
Comment 7 Jon Lee 2016-05-27 12:34:47 PDT
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)?
Comment 8 Jeremy Jones 2016-05-27 13:45:38 PDT
Created attachment 279988 [details]
Patch
Comment 9 Jeremy Jones 2016-05-27 13:49:06 PDT
Created attachment 279989 [details]
Patch
Comment 10 Jeremy Jones 2016-05-27 13:49:39 PDT
(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.
Comment 11 Jeremy Jones 2016-05-27 13:50:49 PDT
Also s/nil/nullptr/ to fix gtk and elf.
Comment 12 Tim Horton 2016-05-27 14:59:51 PDT
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).
Comment 13 Jer Noble 2016-05-27 15:06:11 PDT
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.
Comment 14 Jeremy Jones 2016-05-27 15:21:19 PDT
(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.
Comment 15 Tim Horton 2016-05-27 15:25:28 PDT
(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.
Comment 16 Jeremy Jones 2016-05-27 15:27:12 PDT
(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.
Comment 17 Jeremy Jones 2016-05-27 15:59:06 PDT
Created attachment 280005 [details]
Patch for landing.
Comment 18 Jeremy Jones 2016-05-27 16:26:39 PDT
(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+.
Comment 19 WebKit Commit Bot 2016-05-27 16:57:16 PDT
Comment on attachment 280005 [details]
Patch for landing.

Clearing flags on attachment: 280005

Committed r201474: <http://trac.webkit.org/changeset/201474>
Comment 20 Brent Fulgham 2016-07-07 12:22:19 PDT
Comment on attachment 279989 [details]
Patch

Clearing flags now that this bug is landed and closed.