Bug 235006 - [Cocoa] rVFC() isn't called for initial video load
Summary: [Cocoa] rVFC() isn't called for initial video load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks: 235072
  Show dependency treegraph
 
Reported: 2022-01-08 11:00 PST by Jer Noble
Modified: 2022-01-14 14:59 PST (History)
14 users (show)

See Also:


Attachments
Patch (53.77 KB, patch)
2022-01-08 12:09 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (47.85 KB, patch)
2022-01-08 12:16 PST, Jer Noble
eric.carlson: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (47.94 KB, patch)
2022-01-08 14:18 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (47.94 KB, patch)
2022-01-08 14:48 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (43.37 KB, patch)
2022-01-09 11:38 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (43.36 KB, patch)
2022-01-09 14:29 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (43.59 KB, patch)
2022-01-09 17:29 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (46.73 KB, patch)
2022-01-10 00:03 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (46.77 KB, patch)
2022-01-10 09:05 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (47.51 KB, patch)
2022-01-11 09:12 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (47.22 KB, patch)
2022-01-11 11:21 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (47.41 KB, patch)
2022-01-11 11:35 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (47.45 KB, patch)
2022-01-11 13:31 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (49.93 KB, patch)
2022-01-12 14:25 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (49.93 KB, patch)
2022-01-12 15:53 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (50.51 KB, patch)
2022-01-13 09:56 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (50.51 KB, patch)
2022-01-13 10:46 PST, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2022-01-08 11:00:29 PST
[Cocoa] rVFC() isn't called for initial video load
Comment 1 Jer Noble 2022-01-08 12:09:51 PST
Created attachment 448676 [details]
Patch
Comment 2 Jer Noble 2022-01-08 12:16:59 PST
Created attachment 448678 [details]
Patch
Comment 3 Eric Carlson 2022-01-08 13:09:43 PST
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/
Comment 4 Jer Noble 2022-01-08 14:09:56 PST
(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.
Comment 5 Jer Noble 2022-01-08 14:14:46 PST
(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.
Comment 6 Jer Noble 2022-01-08 14:18:13 PST
Created attachment 448682 [details]
Patch for landing
Comment 7 Jer Noble 2022-01-08 14:48:25 PST
Created attachment 448684 [details]
Patch for landing
Comment 8 Jer Noble 2022-01-09 11:38:38 PST
Created attachment 448716 [details]
Patch for landing
Comment 9 Jer Noble 2022-01-09 14:29:32 PST
Created attachment 448720 [details]
Patch for landing
Comment 10 Jer Noble 2022-01-09 17:29:25 PST
Created attachment 448721 [details]
Patch for landing
Comment 11 Jer Noble 2022-01-10 00:03:24 PST
Created attachment 448724 [details]
Patch for landing
Comment 12 Jer Noble 2022-01-10 09:05:40 PST
Created attachment 448758 [details]
Patch for landing
Comment 13 Jer Noble 2022-01-11 08:18:22 PST
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.
Comment 14 Jer Noble 2022-01-11 09:12:10 PST
Created attachment 448851 [details]
Patch for landing
Comment 15 Jer Noble 2022-01-11 11:21:27 PST
Created attachment 448863 [details]
Patch for landing
Comment 16 Jer Noble 2022-01-11 11:35:30 PST
Created attachment 448865 [details]
Patch for landing
Comment 17 Jer Noble 2022-01-11 13:31:24 PST
Created attachment 448874 [details]
Patch for landing
Comment 18 Jer Noble 2022-01-12 14:25:08 PST
Created attachment 448995 [details]
Patch for landing
Comment 19 Jer Noble 2022-01-12 15:53:10 PST
Created attachment 449010 [details]
Patch for landing
Comment 20 Jer Noble 2022-01-13 09:56:53 PST
Created attachment 449075 [details]
Patch for landing
Comment 21 Jer Noble 2022-01-13 10:46:49 PST
Created attachment 449085 [details]
Patch for landing
Comment 22 Simon Fraser (smfr) 2022-01-13 12:11:06 PST
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.
Comment 23 Jer Noble 2022-01-14 11:30:09 PST
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.
Comment 24 EWS 2022-01-14 13:59:54 PST
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].
Comment 25 Radar WebKit Bug Importer 2022-01-14 14:02:34 PST
<rdar://problem/87617561>