Summary: | [Cocoa] Remove support for AVAssetImageGenerator | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | calvaris, cdumez, changseok, clopez, commit-queue, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, philipj, sergio, tsavell, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=228714 | ||||||||||||||||||||||||||||||||
Bug Depends on: | 228620, 228677 | ||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Jer Noble
2021-07-28 11:30:18 PDT
Created attachment 434458 [details]
Patch
Created attachment 434460 [details]
Patch
Created attachment 434467 [details]
Patch
Comment on attachment 434467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434467&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2387 > + if ([m_videoOutput respondsToSelector:@selector(requestNotificationOfMediaDataChangeAsSoonAsPossible)]) > + [m_videoOutput requestNotificationOfMediaDataChangeAsSoonAsPossible]; As discussed, it might be worth logging an error if it doesn't support this selector Created attachment 434471 [details]
Patch for landing
Created attachment 434498 [details]
Patch for landing
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess Created attachment 434499 [details]
Patch for landing
The mac-AS-debug-wk2 test failure will be fixed by bug #228620. ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!. ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!. Created attachment 434605 [details]
Patch for landing
After running tests on a mac-AS-debug-wk2 and catching some of the timeouts & using the Web Inspector, I'm confident that the test failures are unrelated to my patch. The failures, when caught, appear to be a result of `testRunner.notifyDone()` not causing the test contents to be dumped. In all the results I've caught, the DOM includes `"TEST COMPLETE"`, but WKTR is still waiting for results to be dumped. I have caught other flaky tests that are stuck in AVPlayerItem -seekToTime:toleranceBefore:toleranceAfter:completionHandler: being stuck without the completionHandler being called. Calling -cancelPendingSeeks through lldb causes the test to "unstick" and run to completion. Also unrelated to this patch. Committed r280488 (240120@main): <https://commits.webkit.org/240120@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434605 [details]. It looks like the changes in https://trac.webkit.org/changeset/280488/webkit broke 10 media/modern-media-controls/ tests causing them to time out tracking in https://bugs.webkit.org/show_bug.cgi?id=228660 Re-opened since this is blocked by bug 228677 Created attachment 434709 [details]
Patch
Created attachment 434710 [details]
Patch for landing
Created attachment 434711 [details]
Patch for landing
ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!. Created attachment 434751 [details]
Patch for landing
Committed r280531 (240163@main): <https://commits.webkit.org/240163@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434751 [details]. (In reply to EWS from comment #23) > Committed r280531 (240163@main): <https://commits.webkit.org/240163@main> It appears that we are seeing modern-media-controls layout test timeouts again: https://build.webkit.org/results/Apple-BigSur-Release-AppleSilicon-WK2-Tests/r280531%20(2978)/results.html Reverted r280531 and r280589 for reason: caused Committed r280624 (240237@main): <https://commits.webkit.org/240237@main> Created attachment 434926 [details]
Patch
Comment on attachment 434926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434926&action=review > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:-132 > - tearDownVideoRendering(); Last changed in 2011! > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2429 > m_videoOutputDelegate = adoptNS([[WebCoreAVFPullDelegate alloc] init]); > + [m_videoOutputDelegate setParent:*this]; Nit: since you pass a reference, and so never need to clear the parent, you could add an initializer and pass it there. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2513 > + updateLastImage(UpdateType::UpdateSynchronously); Is it OK to ignore the old warning about not doing this synchronously? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2566 > + bool satisfied = timeoutTimer.isActive(); Isn't there is a (very) small chance that this will be wrong because you check the timer after the logging above? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2568 > + INFO_LOG(LOGIDENTIFIER, "timed out"); Why not log this as an error so it always shows up in logs? (In reply to Eric Carlson from comment #27) > Comment on attachment 434926 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434926&action=review > > > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:-132 > > - tearDownVideoRendering(); > > Last changed in 2011! :-D > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2429 > > m_videoOutputDelegate = adoptNS([[WebCoreAVFPullDelegate alloc] init]); > > + [m_videoOutputDelegate setParent:*this]; > > Nit: since you pass a reference, and so never need to clear the parent, you > could add an initializer and pass it there. Good idea. I'll make it the same signature as WebCoreAVFLoaderDelegate. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2513 > > + updateLastImage(UpdateType::UpdateSynchronously); > > Is it OK to ignore the old warning about not doing this synchronously? Lol yes. Because later on in updateLastImage(), it used to call into AVAssetImageGenerator, which is super synchronous. Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2566 > > + bool satisfied = timeoutTimer.isActive(); > > Isn't there is a (very) small chance that this will be wrong because you > check the timer after the logging above? I don't think so? The timer's time might be up, but it hasn't fired yet (because it has to run on the main run-loop). > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2568 > > + INFO_LOG(LOGIDENTIFIER, "timed out"); > > Why not log this as an error so it always shows up in logs? Will do. Created attachment 434955 [details]
Patch for landing
Created attachment 435016 [details]
Patch for landing
Looks like the EWS failures are due to https://trac.webkit.org/changeset/280702/webkit. Landing. Committed r280723 (240314@main): <https://commits.webkit.org/240314@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435016 [details]. |