Bug 228560 – [Cocoa] Remove support for AVAssetImageGenerator

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[Cocoa] Remove support for AVAssetImageGenerator

[Jer Noble]

Created attachment 434458 [details]
Patch

[Jer Noble]

Created attachment 434460 [details]
Patch

[Jer Noble]

Created attachment 434467 [details]
Patch

[Eric Carlson]

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

[Jer Noble]

Created attachment 434471 [details]
Patch for landing

[Jer Noble]

Created attachment 434498 [details]
Patch for landing

[EWS Watchlist]

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

[Jer Noble]

Created attachment 434499 [details]
Patch for landing

[Jer Noble]

The mac-AS-debug-wk2 test failure will be fixed by bug #228620.

[EWS]

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

[EWS]

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

[Jer Noble]

Created attachment 434605 [details]
Patch for landing

[Jer Noble]

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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/81336280>

[Truitt Savell]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 228677

[Jer Noble]

Created attachment 434709 [details]
Patch

[Jer Noble]

Created attachment 434710 [details]
Patch for landing

[Jer Noble]

Created attachment 434711 [details]
Patch for landing

[EWS]

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

[Jer Noble]

Created attachment 434751 [details]
Patch for landing

[EWS]

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].

[Ryan Haddad]

(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

[Ryan Haddad]

Reverted r280531 and r280589 for reason:

caused

Committed r280624 (240237@main): <https://commits.webkit.org/240237@main>

[Jer Noble]

Created attachment 434926 [details]
Patch

[Eric Carlson]

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?

[Jer Noble]

(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.

[Jer Noble]

Created attachment 434955 [details]
Patch for landing

[Jer Noble]

Created attachment 435016 [details]
Patch for landing

[Jer Noble]

<rdar://81542859>

[Jer Noble]

Looks like the EWS failures are due to https://trac.webkit.org/changeset/280702/webkit. Landing.

[EWS]

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].