Summary: | [Cocoa] rVFC() isn't called for initial video load | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jean-yves.avenard, peng.liu6, philipj, sergio, simon.fraser, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 235072, 237286 | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Jer Noble
2022-01-08 11:00:29 PST
Created attachment 448676 [details]
Patch
Created attachment 448678 [details]
Patch
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/ (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. (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. Created attachment 448682 [details]
Patch for landing
Created attachment 448684 [details]
Patch for landing
Created attachment 448716 [details]
Patch for landing
Created attachment 448720 [details]
Patch for landing
Created attachment 448721 [details]
Patch for landing
Created attachment 448724 [details]
Patch for landing
Created attachment 448758 [details]
Patch for landing
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. Created attachment 448851 [details]
Patch for landing
Created attachment 448863 [details]
Patch for landing
Created attachment 448865 [details]
Patch for landing
Created attachment 448874 [details]
Patch for landing
Created attachment 448995 [details]
Patch for landing
Created attachment 449010 [details]
Patch for landing
Created attachment 449075 [details]
Patch for landing
Created attachment 449085 [details]
Patch for landing
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. 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. 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]. |