Bug 234131 – [Cocoa] -[AVPlayerItem liveUpdateTime] can hang the main thread for ~60ms

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

[Cocoa] -[AVPlayerItem liveUpdateThread] can hang the main thread for ~60ms

[Jer Noble]

<rdar://86307593>

[Jer Noble]

Created attachment 446682 [details]
Patch

[Eric Carlson]

Comment on attachment 446682 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [Cocoa] -[AVPlayerItem liveUpdateThread] can hang the main thread for ~60ms

Do you mean -[AVPlayerItem liveUpdateInterval]?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3839
> +    auto queueTaskOnEventLoopWithPlayer = [self, strongSelf = retainPtr(self)] (Function<void(MediaPlayerPrivateAVFoundationObjC&)>&& function) mutable {

s/strongSelf = retainPtr(self)/strongSelf = RetainPtr { self }/

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3856
> +        auto seekableTimeRanges = retainPtr((NSArray*)newValue);

s/ = retainPtr( ... )/ = RetainPtr { ... }/

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3858
> +        auto playerItem = retainPtr((AVPlayerItem*)object);

The player doesn't need to be kept alive so you could avoid the refcount churn by just casting to an AVPlayerItem

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3866
> +    queueTaskOnEventLoopWithPlayer([keyPath = retainPtr(keyPath), change = retainPtr(change), object = retainPtr(object), context] (auto& player) mutable {

s/ = retainPtr( ... )/ = RetainPtr { ... }/

[Jer Noble]

(In reply to Eric Carlson from comment #3)
> Comment on attachment 446682 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446682&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [Cocoa] -[AVPlayerItem liveUpdateThread] can hang the main thread for ~60ms
> 
> Do you mean -[AVPlayerItem liveUpdateInterval]?

Whoops, yes I do.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3839
> > +    auto queueTaskOnEventLoopWithPlayer = [self, strongSelf = retainPtr(self)] (Function<void(MediaPlayerPrivateAVFoundationObjC&)>&& function) mutable {
> 
> s/strongSelf = retainPtr(self)/strongSelf = RetainPtr { self }/

Ok.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3856
> > +        auto seekableTimeRanges = retainPtr((NSArray*)newValue);
> 
> s/ = retainPtr( ... )/ = RetainPtr { ... }/
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3858
> > +        auto playerItem = retainPtr((AVPlayerItem*)object);
> 
> The player doesn't need to be kept alive so you could avoid the refcount
> churn by just casting to an AVPlayerItem

So, I think I'm going to have to modify the patch to explicitly do this query on a background queue. The KVO will happen on the main thread most of the time, in which case, I need to keep this a RetainPtr so I can pass it through to a background queue.

[Jer Noble]

Created attachment 446753 [details]
Patch for landing

[EWS]

Committed r286907 (245133@main): <https://commits.webkit.org/245133@main>

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