Summary: | [Cocoa] Make Coordinator playback commands more precise | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||
Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, calvaris, cdumez, changseok, cmarcelo, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, philipj, sergio, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Jer Noble
2021-07-07 11:18:54 PDT
Created attachment 433055 [details]
Patch
Created attachment 433059 [details]
Patch
Created attachment 433060 [details]
Patch
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 on attachment 433060 [details]
Patch
r=me once the bots are happy
(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. Created attachment 433062 [details]
Patch for landing
Created attachment 433063 [details]
Patch for landing
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]. |