Skip to content

Commit

Permalink
WPT webcodecs videoDecoder-codec-specific.https.any.html test is fail…
Browse files Browse the repository at this point in the history 
…ing due to resolve promises out of an event queue task

https://bugs.webkit.org/show_bug.cgi?id=258814
rdar://111692360

Reviewed by Eric Carlson.

The WPT test change is coming from upstream and checks that the flush promise callbacks are executed before the error event handlers.
The implementation is correctly resolving the promises before scheduling a task to fire the error event.
But the promises are resolved outside of the event loop and are thus delayed to a loater task, and the error event fires before.
To prevent this, we are changing the way the internal decoder is calling back into WebCodecsVideoDecoder code.

Instead of just hoping to the right thread, we are now hoping to the right thread and then enqueueing a task.
We add a weakThis check there so that it is no longer necessary on each callback.

Covered by updated tests.

* LayoutTests/imported/w3c/web-platform-tests/webcodecs/videoDecoder-codec-specific.https.any.js:
* Source/WebCore/Modules/webcodecs/WebCodecsVideoDecoder.cpp:
(WebCore::WebCodecsVideoDecoder::configure):
(WebCore::WebCodecsVideoDecoder::decode):
(WebCore::WebCodecsVideoDecoder::flush):
(WebCore::WebCodecsVideoDecoder::resetDecoder):

Canonical link: https://commits.webkit.org/265843@main
  • Loading branch information
@youennf
youennf committed Jul 7, 2023
1 parent 1866a4e commit 42d2cdb
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 35 deletions.
32 changes: 26 additions & 6 deletions ...tTests/imported/w3c/web-platform-tests/webcodecs/videoDecoder-codec-specific.https.any.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,10 @@ promise_test(async t => {
const callbacks = {};

let errors = 0;
callbacks.error = e => errors++;
let gotError = new Promise(resolve => callbacks.error = e => {
errors++;
resolve(e);
});
callbacks.output = frame => { frame.close(); };

const decoder = createVideoDecoder(t, callbacks);
Expand All @@ -372,9 +375,16 @@ promise_test(async t => {
decoder.decode(new EncodedVideoChunk(
{type: 'key', timestamp: 1, data: new ArrayBuffer(0)}));

await promise_rejects_dom(t, 'AbortError', decoder.flush());
await promise_rejects_dom(t, "EncodingError",
decoder.flush().catch((e) => {
assert_equals(errors, 0);
throw e;
})
);

assert_equals(errors, 1, 'errors');
let e = await gotError;
assert_true(e instanceof DOMException);
assert_equals(e.name, 'EncodingError');
assert_equals(decoder.state, 'closed', 'state');
}, 'Decode empty frame');

Expand All @@ -384,7 +394,10 @@ promise_test(async t => {
const callbacks = {};

let errors = 0;
callbacks.error = e => errors++;
let gotError = new Promise(resolve => callbacks.error = e => {
errors++;
resolve(e);
});

let outputs = 0;
callbacks.output = frame => {
Expand All @@ -397,10 +410,17 @@ promise_test(async t => {
decoder.decode(CHUNKS[0]); // Decode keyframe first.
decoder.decode(createCorruptChunk(2));

await promise_rejects_dom(t, 'AbortError', decoder.flush());
await promise_rejects_dom(t, "EncodingError",
decoder.flush().catch((e) => {
assert_equals(errors, 0);
throw e;
})
);

assert_less_than_equal(outputs, 1);
assert_equals(errors, 1, 'errors');
let e = await gotError;
assert_true(e instanceof DOMException);
assert_equals(e.name, 'EncodingError');
assert_equals(decoder.state, 'closed', 'state');
}, 'Decode corrupt frame');

Expand Down
47 changes: 18 additions & 29 deletions Source/WebCore/Modules/webcodecs/WebCodecsVideoDecoder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,32 +119,26 @@ ExceptionOr<void> WebCodecsVideoDecoder::configure(ScriptExecutionContext& conte

queueControlMessageAndProcess([this, config = WTFMove(config), identifier = scriptExecutionContext()->identifier()]() mutable {
m_isMessageQueueBlocked = true;
VideoDecoder::PostTaskCallback postTaskCallback;
if (isMainThread()) {
postTaskCallback = [](auto&& task) {
ensureOnMainThread(WTFMove(task));
};
} else {
postTaskCallback = [identifier](auto&& task) {
ScriptExecutionContext::postTaskTo(identifier, [task = WTFMove(task)](auto&) mutable {
VideoDecoder::PostTaskCallback postTaskCallback = [identifier, weakThis = WeakPtr { *this }](auto&& task) {
ScriptExecutionContext::postTaskTo(identifier, [weakThis, task = WTFMove(task)](auto&) mutable {
if (!weakThis)
return;
weakThis->queueTaskKeepingObjectAlive(*weakThis, TaskSource::MediaElement, [task = WTFMove(task)]() mutable {
task();
});
};
}

VideoDecoder::create(config.codec, createVideoDecoderConfig(config), [this, weakedThis = WeakPtr { *this }](auto&& result) {
if (!weakedThis)
return;
});
};

VideoDecoder::create(config.codec, createVideoDecoderConfig(config), [this](auto&& result) {
if (!result.has_value()) {
closeDecoder(Exception { NotSupportedError, WTFMove(result.error()) });
return;
}
setInternalDecoder(WTFMove(result.value()));
m_isMessageQueueBlocked = false;
processControlMessageQueue();
}, [this, weakedThis = WeakPtr { *this }](auto&& result) {
if (!weakedThis || m_state != WebCodecsCodecState::Configured)
}, [this](auto&& result) {
if (m_state != WebCodecsCodecState::Configured)
return;

if (!result.has_value()) {
Expand Down Expand Up @@ -184,15 +178,10 @@ ExceptionOr<void> WebCodecsVideoDecoder::decode(Ref<WebCodecsEncodedVideoChunk>&
--m_decodeQueueSize;
scheduleDequeueEvent();

m_internalDecoder->decode({ { chunk->data(), chunk->byteLength() }, chunk->type() == WebCodecsEncodedVideoChunkType::Key, chunk->timestamp(), chunk->duration() }, [this, weakedThis = WeakPtr { *this }](auto&& result) {
if (!weakedThis)
return;

m_internalDecoder->decode({ { chunk->data(), chunk->byteLength() }, chunk->type() == WebCodecsEncodedVideoChunkType::Key, chunk->timestamp(), chunk->duration() }, [this](auto&& result) {
--m_beingDecodedQueueSize;
if (!result.isNull()) {
if (!result.isNull())
closeDecoder(Exception { EncodingError, WTFMove(result) });
return;
}
});
});
return { };
Expand All @@ -206,9 +195,9 @@ ExceptionOr<void> WebCodecsVideoDecoder::flush(Ref<DeferredPromise>&& promise)
m_isKeyChunkRequired = true;
m_pendingFlushPromises.append(promise.copyRef());
m_isFlushing = true;
queueControlMessageAndProcess([this, clearFlushPromiseCount = m_clearFlushPromiseCount]() mutable {
m_internalDecoder->flush([this, weakThis = WeakPtr { *this }, clearFlushPromiseCount] {
if (!weakThis || clearFlushPromiseCount != m_clearFlushPromiseCount)
queueControlMessageAndProcess([this, clearFlushPromiseCount = m_clearFlushPromiseCount] {
m_internalDecoder->flush([this, clearFlushPromiseCount] {
if (clearFlushPromiseCount != m_clearFlushPromiseCount)
return;

m_pendingFlushPromises.takeFirst()->resolve();
Expand Down Expand Up @@ -269,7 +258,7 @@ ExceptionOr<void> WebCodecsVideoDecoder::closeDecoder(Exception&& exception)
return { };
}

ExceptionOr<void> WebCodecsVideoDecoder::resetDecoder(const Exception&)
ExceptionOr<void> WebCodecsVideoDecoder::resetDecoder(const Exception& exception)
{
if (m_state == WebCodecsCodecState::Closed)
return Exception { InvalidStateError, "VideoDecoder is closed"_s };
Expand All @@ -283,9 +272,9 @@ ExceptionOr<void> WebCodecsVideoDecoder::resetDecoder(const Exception&)
scheduleDequeueEvent();
}
++m_clearFlushPromiseCount;
// FIXME: Check whether aligning with the spec or with WPT tests.

while (!m_pendingFlushPromises.isEmpty())
m_pendingFlushPromises.takeFirst()->reject(Exception { AbortError, "aborting flush as decoder is reset"_s });
m_pendingFlushPromises.takeFirst()->reject(exception);

return { };
}
Expand Down

0 comments on commit 42d2cdb

Please sign in to comment.