[webkit-reviews] review granted: [Bug 235006] [Cocoa] rVFC() isn't called for initial video load : [Attachment 448678] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 235006: [Cocoa] rVFC() isn't called for initial video load
https://bugs.webkit.org/show_bug.cgi?id=235006

Attachment 448678: Patch

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




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 448678
  --> https://bugs.webkit.org/attachment.cgi?id=448678
Patch

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

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:693
> +    if (m_videoOutput->hasImageForTime(PAL::toMediaTime([m_avPlayerItem
currentTime])))

This isn’t new, but we might want to use m_cachedCurrentTime so we don’t block
in [AVPlayerItem currentTime].

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1440
> +	   m_currentImageChangedObserver =
WTF::make_unique<Observer<void()>>([this] {

ASSERT(!m_currentImageChangedObserver)

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:2537
> +    auto currentTime = PAL::toMediaTime([m_avPlayerItem currentTime]);

Ditto the comment above about using m_cachedCurrentTime

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:2542
> +    auto entry = m_videoOutput->takeVideoFrameEntryForTime(currentTime);

`hasImageForTime` and `takeVideoFrameEntryForTime` both search the hashmap. Can
you avoid doing this twice by having `takeVideoFrameEntryForTime` return a
std::optional?

> Source/WebCore/platform/graphics/avfoundation/objc/QueuedVideoOutput.h:68
> +    using ImageMap = std::map<MediaTime, RetainPtr<CVPixelBufferRef>>;

Can this be private?

> LayoutTests/media/request-video-frame-seek.html:4
> +    <title>request-video-frame-loadstart</title>

s/loadstart/seek/