WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
227756
[Cocoa] Make Coordinator playback commands more precise
https://bugs.webkit.org/show_bug.cgi?id=227756
Summary
[Cocoa] Make Coordinator playback commands more precise
Jer Noble
Reported
2021-07-07 11:18:54 PDT
[Cocoa] Make Coordinator playback commands more precise
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 attachment
proposed patch, testcase, etc.
Jer Noble
Comment 1
2021-07-07 11:43:06 PDT
Created
attachment 433055
[details]
Patch
Jer Noble
Comment 2
2021-07-07 12:09:02 PDT
Created
attachment 433059
[details]
Patch
Jer Noble
Comment 3
2021-07-07 12:21:04 PDT
Created
attachment 433060
[details]
Patch
Eric Carlson
Comment 4
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?
Eric Carlson
Comment 5
2021-07-07 12:28:16 PDT
Comment on
attachment 433060
[details]
Patch r=me once the bots are happy
Jer Noble
Comment 6
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.
Jer Noble
Comment 7
2021-07-07 12:40:43 PDT
Created
attachment 433062
[details]
Patch for landing
Jer Noble
Comment 8
2021-07-07 12:58:30 PDT
Created
attachment 433063
[details]
Patch for landing
EWS
Comment 9
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]
.
Radar WebKit Bug Importer
Comment 10
2021-07-09 09:47:19 PDT
<
rdar://problem/80381143
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug