Bug 227756 - [Cocoa] Make Coordinator playback commands more precise
Summary: [Cocoa] Make Coordinator playback commands more precise
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-07 11:18 PDT by Jer Noble
Modified: 2021-07-09 09:47 PDT (History)
13 users (show)

See Also:


Attachments
Patch (37.24 KB, patch)
2021-07-07 11:43 PDT, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (38.41 KB, patch)
2021-07-07 12:09 PDT, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (38.51 KB, patch)
2021-07-07 12:21 PDT, Jer Noble
eric.carlson: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (38.69 KB, patch)
2021-07-07 12:40 PDT, Jer Noble
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (38.68 KB, patch)
2021-07-07 12:58 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2021-07-07 11:18:54 PDT
[Cocoa] Make Coordinator playback commands more precise
Comment 1 Jer Noble 2021-07-07 11:43:06 PDT
Created attachment 433055 [details]
Patch
Comment 2 Jer Noble 2021-07-07 12:09:02 PDT
Created attachment 433059 [details]
Patch
Comment 3 Jer Noble 2021-07-07 12:21:04 PDT
Created attachment 433060 [details]
Patch
Comment 4 Eric Carlson 2021-07-07 12:27:58 PDT
Comment on attachment 433059 [details]
Patch

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

> Source/WTF/wtf/CurrentTime.cpp:267
> +MonotonicTime MonotonicTime::fromMachAbsoluteTime(uint64_t machAbsoluteTime)

Can this (and fromMachAbsoluteTime) be const?

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:77
> +    const std::optional<PlaySessionCommand>& currentPlaySessionCommand() const { return m_currentPlaySessionCommand; }

It looks like this isn't used.

> Source/WebCore/html/HTMLMediaElement.cpp:5430
> +        if (!currentPlaySessionCommand->hostTime)
> +            break;

Is it ever valid for the coordinator to have a current play session command without a host time? If not, should this assert?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1573
> +        [m_avPlayer setAutomaticallyWaitsToMinimizeStalling:shouldAutomaticallyWait];
> +        m_automaticallyWaitsToMinimizeStalling = shouldAutomaticallyWait;

Should we restore this after the setRate completes?
Comment 5 Eric Carlson 2021-07-07 12:28:16 PDT
Comment on attachment 433060 [details]
Patch

r=me once the bots are happy
Comment 6 Jer Noble 2021-07-07 12:33:50 PDT
(In reply to Eric Carlson from comment #4)
> Comment on attachment 433059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433059&action=review
> 
> > Source/WTF/wtf/CurrentTime.cpp:267
> > +MonotonicTime MonotonicTime::fromMachAbsoluteTime(uint64_t machAbsoluteTime)
> 
> Can this (and fromMachAbsoluteTime) be const?

They can't be because they're already `static`.

> > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:77
> > +    const std::optional<PlaySessionCommand>& currentPlaySessionCommand() const { return m_currentPlaySessionCommand; }
> 
> It looks like this isn't used.

True; it's just there for completeness and as a convenience. I'll remove it.

> > Source/WebCore/html/HTMLMediaElement.cpp:5430
> > +        if (!currentPlaySessionCommand->hostTime)
> > +            break;
> 
> Is it ever valid for the coordinator to have a current play session command
> without a host time?

Yes, there's no guarantee at the WebCore level that a valid hostTime is present; that's a detail of the MediaSessionCoordinatorPrivate.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1573
> > +        [m_avPlayer setAutomaticallyWaitsToMinimizeStalling:shouldAutomaticallyWait];
> > +        m_automaticallyWaitsToMinimizeStalling = shouldAutomaticallyWait;
> 
> Should we restore this after the setRate completes?

Interesting question, but it seems improbable. We only call setRate: from this method, so I'm pretty sure it wouldn't have any effect otherwise.
Comment 7 Jer Noble 2021-07-07 12:40:43 PDT
Created attachment 433062 [details]
Patch for landing
Comment 8 Jer Noble 2021-07-07 12:58:30 PDT
Created attachment 433063 [details]
Patch for landing
Comment 9 EWS 2021-07-09 09:46:12 PDT
Committed r279786 (239553@main): <https://commits.webkit.org/239553@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433063 [details].
Comment 10 Radar WebKit Bug Importer 2021-07-09 09:47:19 PDT
<rdar://problem/80381143>