Decrease flicker when changing video presentation mode.
rdar://problem/24476949
Created attachment 279949 [details] Patch
Created attachment 279963 [details] Patch Rebaselined Jeremy's patch for EWS to chew on
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 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.
Created attachment 279982 [details] Patch
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)?
Created attachment 279988 [details] Patch
Created attachment 279989 [details] Patch
(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.
Also s/nil/nullptr/ to fix gtk and elf.
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 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.
(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.
(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.
(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.
Created attachment 280005 [details] Patch for landing.
(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 on attachment 280005 [details] Patch for landing. Clearing flags on attachment: 280005 Committed r201474: <http://trac.webkit.org/changeset/201474>
Comment on attachment 279989 [details] Patch Clearing flags now that this bug is landed and closed.