[webkit-reviews] review granted: [Bug 226960] Video poster disappears prematurely on play, leaving transparent video element. : [Attachment 454626] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 226960: Video poster disappears prematurely on play, leaving transparent
video element.
https://bugs.webkit.org/show_bug.cgi?id=226960

Attachment 454626: Patch

https://bugs.webkit.org/attachment.cgi?id=454626&action=review




--- Comment #6 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 454626
  --> https://bugs.webkit.org/attachment.cgi?id=454626
Patch

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

r=me with nits:

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:543
> +	   bool hasAvailableVideoFrame = this->hasAvailableVideoFrame();
> +	   if (!m_haveReportedFirstVideoFrame && m_cachedHasVideo &&
hasAvailableVideoFrame) {
> +	       haveFirstVideoFrameChanged = true;
> +	       m_haveReportedFirstVideoFrame = true;
> +	   } else if (!hasAvailableVideoFrame)
> +	       m_haveReportedFirstVideoFrame = false;

This section of code confused me. What about this instead?:

	if (!hasAvailableVideoFrame())
	    m_haveReportedFirstVideoFrame = false
	else if (!m_haveReportedFirstVideoFrame && m_cachedHasVideo) {
	    m_haveReportedFirstVideoFrame = true;
	    haveFirstVideoFrameChanged = true;
	}

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:561
> -		   if (m_readyState != MediaPlayer::ReadyState::HaveEnoughData
&& maxTimeLoaded() > currentMediaTime())
> +		   if (m_readyState != MediaPlayer::ReadyState::HaveEnoughData
&& (!m_cachedHasVideo || m_haveReportedFirstVideoFrame) && maxTimeLoaded() >
currentMediaTime())

I wonder if we should have a `shouldBlockAdvancingToFutureData` variable
(initialized to the same as above) here.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:582
> -    if (!m_haveReportedFirstVideoFrame && m_cachedHasVideo &&
hasAvailableVideoFrame()) {
> +    if (haveFirstVideoFrameChanged) {

`haveFirstVideoFrameChanged` is slightly weird (because it's really saying the
media player has a video frame available for the first time). Maybe
`videoFrameBecomeAvailable`?