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
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

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>