-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SourceBufferPrivateClient/MediaSourcePrivateClient should use NativePromise #20513
Conversation
EWS run on previous version of this PR (hash 47d30c7) |
unrelated build failure ```
|
, completionHandler(WTFMove(completionHandler)) | ||
{ | ||
ASSERT(this->completionHandler); | ||
Ref<MediaTimePromise> promise = SourceBuffer::ComputeSeekPromise::all(RunLoop::main(), WTF::map(*m_activeSourceBuffers, [&](auto&& sourceBuffer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually only use RunLoop::main() on WK2 code since there is the web thread on WK1.
RunLoop::current() might be better.
Ditto elsewhere probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The aim here will be to set the thread where we will run as a class member. Right now it's the main thread. MaybE I should use WorkQueue::main() instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: RunLoop::main()
will be the main thread, even on iOS WK1. WorkQueue::main()
will also be the main thread. callOnMainThread()
will run on the Web Thread (not the main thread, or the UI thread).
m_seekCompletedHandler(MediaTime::invalidTime()); | ||
if (m_seekTargetPromise) | ||
m_seekTargetPromise->reject(-1); | ||
m_seekTargetPromise.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could reset in the if block.
@@ -697,12 +696,10 @@ void SourceBuffer::setActive(bool active) | |||
m_source->sourceBufferDidChangeActiveState(*this, active); | |||
} | |||
|
|||
void SourceBuffer::sourceBufferPrivateDidReceiveInitializationSegment(InitializationSegment&& segment, CompletionHandler<void(ReceiveResult)>&& completionHandler) | |||
auto SourceBuffer::sourceBufferPrivateDidReceiveInitializationSegment(InitializationSegment&& segment) -> Ref<ReceiveResultPromise> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why auto for sourceBufferPrivateDidReceiveInitializationSegment and not for computeSeekTime (or the reverse)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it makes for a shorter writing :)
Otherwise I have to write the fully qualified type name
return; | ||
} | ||
if (isClosed()) | ||
return MediaTimePromise::createAndReject(-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we define a constant instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm replacing this with a constant in part 4 with a PlatformMediaError
if (!track || error) | ||
auto promise = parser->appendData(WTFMove(segment)); | ||
ASSERT(promise->isSettled()); | ||
if (!track || !promise->isResolved()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a promise if it should be resolved synchronously? The previous code seems better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SourceBufferParser has two mode of operations. One synchronous like here. And one as used by the SourceBuffer where you define the thread on which the callbacks will be called. That is asynchronous.
We should make the webaudio decoder fully asynchronous too. But it was out of scope for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made SourceBufferParser::appendData return an Expected<void, int>
instead so that it can be used synchronously. Will migrate in a follow-up change
// duration set to the maximum of the current duration and the group end timestamp. | ||
promises.append(m_client->sourceBufferPrivateDurationChanged(m_groupEndTimestamp)); | ||
} | ||
m_client->sourceBufferPrivateReportExtraMemoryCost(totalTrackBufferSizeInBytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a change of timing, we are now calling this synchronously, while before it could be called asynchronously.
Is this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending the report to how much memory is used can be done at any time. Previously we serialised the operation, now we run them in parallel because Native Promise::all allows to conveniently do so.
The order of operation here doesn't matter. We only need them to happen before we complete the appendBuffer operation
m_client->seekToTime(time, WTFMove(completionHandler)); | ||
if (!m_client) | ||
return GenericPromise::createAndReject(-1); | ||
return m_client->seekToTime(time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One liner ?
|
||
if (m_lastErrorCode) | ||
return GenericPromise::createAndReject(m_lastErrorCode.value()); | ||
return GenericPromise::createAndResolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one liner ?
}); | ||
}); | ||
return invokeAsync(m_appendQueue, [data = WTFMove(data), parser = m_parser]() mutable { | ||
return parser->appendData(WTFMove(data)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see why you changed to a promise.
If it is the only use of a promise, maybe you could promisify the synchronous result directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. All the callbacks are called asynchronously too
return; | ||
} | ||
if (!m_remoteMediaPlayerProxy) | ||
ReceiveResultPromise::createAndReject(ReceiveResult::ClientDisconnected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we return here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, good spotting, thanks
47d30c7
to
148c357
Compare
EWS run on previous version of this PR (hash 148c357) |
148c357
to
cbf4e77
Compare
EWS run on current version of this PR (hash cbf4e77) |
EWS run on current version of this PR (hash cbf4e77) |
cbf4e77
to
57f069d
Compare
β¦romise https://bugs.webkit.org/show_bug.cgi?id=264847 rdar://problem/118426574 Reviewed by Youenn Fablet. By using NativePromise, the SourceBufferPrivate and MediaSourcePrivate can be made to run on any threads as NativePromise let you control on which thread the result of an operation should be delivered. We make all internal SourceBufferPrivate/MediaSourcePrivate methods communicate via their respective SourceBufferPrivateClient/MediaSourcePrivateClient using NativePromise instead of CompletionHandler. It also allows to more explicitly handle errors. The communication between SourceBuffer and SourceBufferPrivate will also be made to use NativePromise in a follow-up change. No change in behaviour in WK2, in WK1 some asynchronicity introduced which makes the behaviour more similar to WK2 * LayoutTests/media/media-source/media-source-restrictions.html: Update test as we've introduced more asynchronicity, and in WK1 events are fired in a different order (but still per spec). So ensure the events are still fired but don't log them. * Source/WebCore/Modules/mediasource/MediaSource.cpp: (WebCore::MediaSource::waitForTarget): (WebCore::MediaSource::completeSeek): (WebCore::MediaSource::seekToTime): (WebCore::MediaSource::detachFromElement): (WebCore::MediaSource::stop): (WebCore::MediaSource::onReadyStateChange): * Source/WebCore/Modules/mediasource/MediaSource.h: * Source/WebCore/Modules/mediasource/SourceBuffer.cpp: (WebCore::SourceBuffer::computeSeekTime): (WebCore::SourceBuffer::sourceBufferPrivateDidReceiveInitializationSegment): (WebCore::SourceBuffer::sourceBufferPrivateDurationChanged): (WebCore::SourceBuffer::sourceBufferPrivateBufferedChanged): * Source/WebCore/Modules/mediasource/SourceBuffer.h: * Source/WebCore/Modules/webaudio/BaseAudioContext.cpp: (WebCore::BaseAudioContext::decodeAudioData): * Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp: (WebCore::AudioFileReader::demuxWebMData const): * Source/WebCore/platform/graphics/MediaSourcePrivate.h: * Source/WebCore/platform/graphics/MediaSourcePrivateClient.h: * Source/WebCore/platform/graphics/SourceBufferPrivate.cpp: (WebCore::SourceBufferPrivate::setBufferedRanges): (WebCore::SourceBufferPrivate::updateBufferedFromTrackBuffers): (WebCore::SourceBufferPrivate::processAppendCompletedOperation): (WebCore::SourceBufferPrivate::computeSeekTime): (WebCore::SourceBufferPrivate::removeCodedFrames): (WebCore::SourceBufferPrivate::didReceiveInitializationSegment): (WebCore::SourceBufferPrivate::didUpdateFormatDescriptionForTrackId): (WebCore::SourceBufferPrivate::processPendingOperations): (WebCore::SourceBufferPrivate::abortPendingOperations): (WebCore::SourceBufferPrivate::processInitOperation): (WebCore::SourceBufferPrivate::appendCompleted): Deleted. * Source/WebCore/platform/graphics/SourceBufferPrivate.h: (WebCore::SourceBufferPrivate::precheckInitialisationSegment): (WebCore::SourceBufferPrivate::processInitialisationSegment): (WebCore::SourceBufferPrivate::processFormatDescriptionForTrackId): (WebCore::SourceBufferPrivate::updateBufferedFromTrackBuffers): Deleted. (WebCore::SourceBufferPrivate::setBufferedRanges): Deleted. * Source/WebCore/platform/graphics/SourceBufferPrivateClient.h: (WebCore::SourceBufferPrivateClient::isAsync const): Deleted. * Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm: (WebCore::MediaPlayerPrivateMediaSourceAVFObjC::seekInternal): * Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.h: * Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm: (WebCore::MediaSourcePrivateAVFObjC::waitForTarget): (WebCore::MediaSourcePrivateAVFObjC::seekToTime): * Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.h: * Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm: (WebCore::SourceBufferParserAVFObjC::appendData): (WebCore::SourceBufferParserAVFObjC::didFailToParseStreamDataWithError): * Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h: * Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm: (WebCore::SourceBufferPrivateAVFObjC::setTrackChangeCallbacks): (WebCore::SourceBufferPrivateAVFObjC::didParseInitializationData): (WebCore::SourceBufferPrivateAVFObjC::precheckInitialisationSegment): (WebCore::SourceBufferPrivateAVFObjC::processInitialisationSegment): (WebCore::SourceBufferPrivateAVFObjC::processFormatDescriptionForTrackId): (WebCore::SourceBufferPrivateAVFObjC::appendInternal): (WebCore::SourceBufferPrivateAVFObjC::appendCompleted): (WebCore::SourceBufferPrivateAVFObjC::didEncounterErrorDuringParsing): Deleted. (WebCore::SourceBufferPrivateAVFObjC::didUpdateFormatDescriptionForTrackId): Deleted. * Source/WebCore/platform/graphics/cocoa/MediaPlayerPrivateWebM.h: * Source/WebCore/platform/graphics/cocoa/MediaPlayerPrivateWebM.mm: (WebCore::MediaPlayerPrivateWebM::~MediaPlayerPrivateWebM): (WebCore::MediaPlayerPrivateWebM::append): (WebCore::MediaPlayerPrivateWebM::didEncounterErrorDuringParsing): Deleted. (WebCore::MediaPlayerPrivateWebM::abort): Deleted. * Source/WebCore/platform/graphics/cocoa/SourceBufferParser.h: * Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp: (WebCore::SourceBufferParserWebM::appendData): * Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.h: (WebCore::SourceBufferParserWebM::appendData): Deleted. * Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp: (WebCore::MediaPlayerPrivateGStreamerMSE::doSeek): * Source/WebCore/platform/graphics/gstreamer/mse/MediaSourcePrivateGStreamer.cpp: (WebCore::MediaSourcePrivateGStreamer::waitForTarget): (WebCore::MediaSourcePrivateGStreamer::seekToTime): * Source/WebCore/platform/graphics/gstreamer/mse/MediaSourcePrivateGStreamer.h: * Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp: (WebCore::SourceBufferPrivateGStreamer::appendInternal): (WebCore::SourceBufferPrivateGStreamer::didReceiveInitializationSegment): (WebCore::SourceBufferPrivateGStreamer::precheckInitialisationSegment): (WebCore::SourceBufferPrivateGStreamer::processInitialisationSegment): (WebCore::SourceBufferPrivateGStreamer::didReceiveAllPendingSamples): (WebCore::SourceBufferPrivateGStreamer::appendParsingFailed): * Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h: * Source/WebCore/platform/mock/mediasource/MockMediaPlayerMediaSource.cpp: (WebCore::MockMediaPlayerMediaSource::seekToTarget): * Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp: (WebCore::MockMediaSourcePrivate::waitForTarget): (WebCore::MockMediaSourcePrivate::seekToTime): * Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.h: * Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp: (WebCore::MockSourceBufferPrivate::appendInternal): (WebCore::MockSourceBufferPrivate::didReceiveInitializationSegment): * Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.h: * Source/WebKit/GPUProcess/media/RemoteMediaSourceProxy.cpp: (WebKit::RemoteMediaSourceProxy::waitForTarget): (WebKit::RemoteMediaSourceProxy::seekToTime): * Source/WebKit/GPUProcess/media/RemoteMediaSourceProxy.h: * Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.cpp: (WebKit::RemoteSourceBufferProxy::sourceBufferPrivateDidReceiveInitializationSegment): (WebKit::RemoteSourceBufferProxy::sourceBufferPrivateDurationChanged): (WebKit::RemoteSourceBufferProxy::sourceBufferPrivateBufferedChanged): (WebKit::RemoteSourceBufferProxy::computeSeekTime): * Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.h: * Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.messages.in: * Source/WebKit/WebProcess/GPU/media/MediaSourcePrivateRemote.cpp: (WebKit::MediaSourcePrivateRemote::waitForTarget): (WebKit::MediaSourcePrivateRemote::proxyWaitForTarget): (WebKit::MediaSourcePrivateRemote::seekToTime): (WebKit::MediaSourcePrivateRemote::proxySeekToTime): * Source/WebKit/WebProcess/GPU/media/MediaSourcePrivateRemote.h: * Source/WebKit/WebProcess/GPU/media/MediaSourcePrivateRemote.messages.in: * Source/WebKit/WebProcess/GPU/media/SourceBufferPrivateRemote.cpp: (WebKit::SourceBufferPrivateRemote::appendInternal): (WebKit::SourceBufferPrivateRemote::removeCodedFrames): (WebKit::SourceBufferPrivateRemote::computeSeekTime): (WebKit::SourceBufferPrivateRemote::sourceBufferPrivateDidReceiveInitializationSegment): (WebKit::SourceBufferPrivateRemote::sourceBufferPrivateDurationChanged): (WebKit::SourceBufferPrivateRemote::sourceBufferPrivateBufferedChanged): * Source/WebKit/WebProcess/GPU/media/SourceBufferPrivateRemote.h: * Source/WebKit/WebProcess/GPU/media/SourceBufferPrivateRemote.messages.in: Canonical link: https://commits.webkit.org/270788@main
57f069d
to
f15fc78
Compare
Committed 270788@main (f15fc78): https://commits.webkit.org/270788@main Reviewed commits have been landed. Closing PR #20513 and removing active labels. |
f15fc78
cbf4e77