Bug 235006 – [Cocoa] rVFC() isn't called for initial video load

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

[Cocoa] rVFC() isn't called for initial video load

[Jer Noble]

Created attachment 448676 [details]
Patch

[Jer Noble]

Created attachment 448678 [details]
Patch

[Eric Carlson]

Comment on attachment 448678 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.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/MediaPlayerPrivateAVFoundationObjC.mm:1440
> +        m_currentImageChangedObserver = WTF::make_unique<Observer<void()>>([this] {

ASSERT(!m_currentImageChangedObserver)

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

Ditto the comment above about using m_cachedCurrentTime

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.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/

[Jer Noble]

(In reply to Eric Carlson from comment #3)
> Comment on attachment 448678 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448678&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.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].

Good idea.

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

Ok.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2537
> > +    auto currentTime = PAL::toMediaTime([m_avPlayerItem currentTime]);
> 
> Ditto the comment above about using m_cachedCurrentTime

Ok.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.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?

Actually, takeVideoFrameEntryForTime() already returns something nullable; so I think we can just use it in this method alone.

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

Unfortunately no, because of the static, templated findImageForTime() method in the implementation file.

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

Ok.

[Jer Noble]

(In reply to Eric Carlson from comment #3)
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1440
> > +        m_currentImageChangedObserver = WTF::make_unique<Observer<void()>>([this] {
> 
> ASSERT(!m_currentImageChangedObserver)

Actually, this isn't really needed. If the current observer is destroyed, it won't be called by the QueuedVideoOutput, since it's held as a WeakPtr.

[Jer Noble]

Created attachment 448682 [details]
Patch for landing

[Jer Noble]

Created attachment 448684 [details]
Patch for landing

[Jer Noble]

Created attachment 448716 [details]
Patch for landing

[Jer Noble]

Created attachment 448720 [details]
Patch for landing

[Jer Noble]

Created attachment 448721 [details]
Patch for landing

[Jer Noble]

Created attachment 448724 [details]
Patch for landing

[Jer Noble]

Created attachment 448758 [details]
Patch for landing

[Jer Noble]

The remaining test failures on wk1 are in imported/w3c/web-platform-tests/video-rvfc/request-video-frame-callback-repeating.html, and the failure is caused by the AVPlayerLayer causing decoders to get reconfigured and a "new" frame for time 0 is sent. The request-video-frame-callback-repeating.html test fails because rVFC() returns VideoFrameMetadata with mediaTime of 0 twice in a row.  However, given that the underlying cause is that a new decoded frame is submitted for composition, this appears to just be an implementation quirk instead of a true failure.

I'll file a new bug to track re-enabling this test, but for now, my plan is to skip it on wk1.

[Jer Noble]

Created attachment 448851 [details]
Patch for landing

[Jer Noble]

Created attachment 448863 [details]
Patch for landing

[Jer Noble]

Created attachment 448865 [details]
Patch for landing

[Jer Noble]

Created attachment 448874 [details]
Patch for landing

[Jer Noble]

Created attachment 448995 [details]
Patch for landing

[Jer Noble]

Created attachment 449010 [details]
Patch for landing

[Jer Noble]

Created attachment 449075 [details]
Patch for landing

[Jer Noble]

Created attachment 449085 [details]
Patch for landing

[Simon Fraser (smfr)]

Comment on attachment 449085 [details]
Patch for landing

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

> Source/WebCore/html/HTMLVideoElement.cpp:609
> +    // Search first the requests currently being serviced, and mark them as cancelled if found.
> +    auto index = m_servicedVideoFrameRequests.findMatching([identifier](auto& request) { return request->identifier == identifier; });
> +    if (index != notFound) {
> +        m_servicedVideoFrameRequests[index]->cancelled = true;

It would be nice if we could share this cancel/service logic with that for requestAnimationFrame in ScriptedAnimationController. Also, WebXRSession has another copy of the same logic.

[Jer Noble]

I'm reasonably confident there are no Mac-wk1 test regressions; running the following:

run-webkit-tests -1 -f --iterations=8 webgl/2.0.0/conformance2/

Results in zero test failures and a number of progressions.

[EWS]

Committed r288031 (246057@main): <https://commits.webkit.org/246057@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449085 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87617561>