Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder #37699

jyavenard · 2024-12-10T09:41:18Z

`c1b41e0`

Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder
https://bugs.webkit.org/show_bug.cgi?id=284019
rdar://140889671

Reviewed by Youenn Fablet.

We add AudioEncoderCocoa, AudioToolbox implementation of WebCodec's AudioEncoder. We only support
encoding to Opus and AAC as the framework doesn't provide encoder for Flac, mp3 and vorbis (decoder only)

Enabled WPT tests.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/webcodecs/audio-encoder-codec-specific.https.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/webcodecs/audio-encoder-config.https.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/webcodecs/audio-encoder-config.https.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/webcodecs/audio-encoder.https.any-expected.txt:
* LayoutTests/platform/glib/TestExpectations: Remove passing expectations, add the remaining ones that do fail.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/webcodecs/audio-encoder-config.https.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/webcodecs/audio-encoder-config.https.any.worker-expected.txt: Removed.
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/webcodecs/audio-encoder-codec-specific.https.any-expected.txt: Added. The
only failure remaining is related to different default for Opus encoding which yield slightly different values to what expected.
The test performs very rough float comparisons.
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/webcodecs/audio-encoder.https.any-expected.txt: Added.
* LayoutTests/platform/mac/imported/w3c/web-platform-tests/webcodecs/audio-encoder-codec-specific.https.any-expected.txt: Added.
* LayoutTests/platform/mac/imported/w3c/web-platform-tests/webcodecs/audio-encoder.https.any-expected.txt: Added.
* Source/WebCore/Modules/webcodecs/OpusEncoderConfig.h:
* Source/WebCore/Modules/webcodecs/WebCodecsAudioEncoder.cpp:
(WebCore::isSupportedEncoderCodec): Add additional checks to ensure that the values provided in the Opus config are sane.
While this isn't probably the best place to do so, as this is common for both GStreamer and Cocoa AudioEncoder, it's the simplest.
(WebCore::isValidEncoderConfig): Make sure sampleRate and numberOfChannels aren't 0.
(WebCore::createAudioEncoderConfig): Increase code readability by using struct's named member initialisation.
(WebCore::WebCodecsAudioEncoder::configure):
(WebCore::WebCodecsAudioEncoder::encode): Both Firefox and Chromes rejects AudioData that has a different sampleRate or numberOfChannels.
While we did the same, we didn't process the errors as per spec which requires that we also change the state to Closed just before
queueing the error.

(WebCore::WebCodecsAudioEncoder::isConfigSupported):
* Source/WebCore/PlatformMac.cmake:
* Source/WebCore/SourcesCocoa.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/platform/AudioEncoder.cpp:
(WebCore::AudioEncoder::create):  Plumb AudioEncoderCocoa.
* Source/WebCore/platform/AudioEncoder.h:
* Source/WebCore/platform/AudioEncoderActiveConfiguration.h: Add default initialiser to comply with more recent clang version.
It allows to make the struct member initialisation optional.
* Source/WebCore/platform/audio/cocoa/AudioDecoderCocoa.cpp:
(WebCore::InternalAudioDecoderCocoa::initialize): Change default AudioData creation to use interleaved frames instead of planar. A few
tests has this expectations, even though this isn't mandated by the specs.
* Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp: Added.
(WebCore::InternalAudioEncoderCocoa::create):
(WebCore::InternalAudioEncoderCocoa::reset):
(WebCore::InternalAudioEncoderCocoa::converter):
(WebCore::InternalAudioEncoderCocoa::queueSingleton):
(WebCore::AudioEncoderCocoa::create):
(WebCore::AudioEncoderCocoa::AudioEncoderCocoa):
(WebCore::AudioEncoderCocoa::~AudioEncoderCocoa):
(WebCore::AudioEncoderCocoa::encode):
(WebCore::AudioEncoderCocoa::flush):
(WebCore::AudioEncoderCocoa::reset):
(WebCore::AudioEncoderCocoa::close):
(WebCore::InternalAudioEncoderCocoa::InternalAudioEncoderCocoa):
(WebCore::InternalAudioEncoderCocoa::initialize):
(WebCore::InternalAudioEncoderCocoa::compressedAudioOutputBufferCallback):
(WebCore::InternalAudioEncoderCocoa::generateDecoderDescriptionFromSample const):
(WebCore::InternalAudioEncoderCocoa::activeConfiguration const):
(WebCore::InternalAudioEncoderCocoa::processEncodedOutputs):
(WebCore::InternalAudioEncoderCocoa::encode):
(WebCore::InternalAudioEncoderCocoa::flush):
(WebCore::InternalAudioEncoderCocoa::close):
* Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.h: Copied from Source/WebCore/platform/AudioEncoderActiveConfiguration.h.

Canonical link: https://commits.webkit.org/287787@main

85d240d

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-12-10T09:41:26Z

EWS run on previous version of this PR (hash 9224027)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	✅ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	⏳ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	❌ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2
	✅ 🛠 tv	~~🛠 mac-safer-cpp~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-12-10T10:56:05Z

EWS run on previous version of this PR (hash e61db94)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	❌ 🛠 win
❌ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	❌ 🧪 win-tests
❌ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🛠 wpe-cairo
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🛠 gtk
	❌ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	❌ 🛠 vision-sim	❌ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	⏳ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2
	❌ 🛠 tv	~~🛠 mac-safer-cpp~~
	❌ 🛠 tv-sim
	❌ 🛠 watch
	❌ 🛠 watch-sim

webkit-early-warning-system · 2024-12-10T11:06:43Z

EWS run on previous version of this PR (hash e3088e4)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	❌ 🛠 win
❌ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	❌ 🧪 win-tests
❌ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🛠 wpe-cairo
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🛠 gtk
	❌ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	❌ 🛠 vision-sim	❌ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	⏳ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2
	❌ 🛠 tv	~~🛠 mac-safer-cpp~~
	❌ 🛠 tv-sim
	❌ 🛠 watch
	❌ 🛠 watch-sim

webkit-early-warning-system · 2024-12-10T11:26:37Z

EWS run on previous version of this PR (hash 71de9c9)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	⏳ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	❌ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2
	✅ 🛠 tv	❌ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-12-10T13:33:51Z

EWS run on previous version of this PR (hash f591872)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	❌ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-12-10T14:55:25Z

EWS run on previous version of this PR (hash 44a1c71)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	✅ 🛠 wpe	~~🛠 win~~
✅ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2	~~🧪 win-tests~~
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	~~🧪 api-wpe~~
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	❌ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	⏳ 🧪 gtk-wk2
	~~🛠 vision-sim~~	❌ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
	⏳ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2
	~~🛠 tv~~	~~🛠 mac-safer-cpp~~
	~~🛠 tv-sim~~
	~~🛠 watch~~
	~~🛠 watch-sim~~

webkit-early-warning-system · 2024-12-10T15:03:44Z

EWS run on previous version of this PR (hash a3d0d86)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

youennf

Some comments below, I was not able to finish the review.

Source/WebCore/Modules/webcodecs/OpusEncoderConfig.h

youennf · 2024-12-10T18:09:42Z

Source/WebCore/Modules/webcodecs/WebCodecsAudioEncoder.cpp

+    if (!config.sampleRate || !config.numberOfChannels)
+        return false;
+
+    if (config.bitrate && *config.bitrate > std::numeric_limits<int>::max())


Do we need this fix or should we instead update the WebIDL to add EnforceRange on the various properties instead?

That's what the spec states indeed. However this is existing, I could add this as a follow-up

the IDL already has them as [EnforceRange] but this is checking that it's not > int::max() ; while the spec has it has a "long long" I will lodge a WPT bug.

web-platform-tests/wpt#49634

youennf · 2024-12-10T18:22:07Z

Source/WebCore/Modules/webcodecs/WebCodecsAudioEncoder.cpp

@@ -84,6 +85,15 @@ static bool isSupportedEncoderCodec(const StringView& codec)
    if (!isCodecAllowed)
        return false;

+    // Can't find restrictions for those values, but this is what Firefox/Gecko does and WPT audio-encoder-config.https.any.html checks for.
+    if (config.sampleRate < 3000 || config.sampleRate > 384000)


I would be tempted to file a bug and do that as a follow-up.

then we will fail both wpt tests.

web-platform-tests/wpt#49635

youennf · 2024-12-10T18:25:27Z

Source/WebCore/platform/audio/cocoa/AudioDecoderCocoa.cpp

@@ -288,7 +289,7 @@ String InternalAudioDecoderCocoa::initialize(const String& codecName, const Audi

    m_inputFormatDescription = createAudioFormatDescription(*m_inputDescription, m_codecDescription.span());

-    m_outputDescription = CAAudioStreamDescription { double(config.sampleRate), uint32_t(config.numberOfChannels), AudioStreamDescription::Float32, CAAudioStreamDescription::IsInterleaved::No };
+    m_outputDescription = CAAudioStreamDescription { double(config.sampleRate), uint32_t(config.numberOfChannels), AudioStreamDescription::Float32, CAAudioStreamDescription::IsInterleaved::Yes };


We should probably file a spec bug, can you do that?

the spec lets the UA choose, there's no option to tell what you want out which is unfortunate.
In theory, how values are packed internally should be an implementation details and it should behave the same regardless.

However, copyTo has a very different behaviour if the origin is interleaved or not where a planeIndex of 0 now means "all planes"

I've opened w3c/webcodecs#859

youennf · 2024-12-10T18:28:42Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+            asbd.mFramesPerPacket = m_config.sampleRate * m_frameDuration->toDouble();
+        m_outputDescription = asbd;
+    }
+    m_codecString = codecName.isolatedCopy();


Do we really need this, or could we use an enumeration instead?
That could prevent issues with Strings...

I'm happy to change this as a follow-up, right now that's pre-existing and how the GStreamer implementation uses it. Don't want to touch it here.

we probably don't need the isolatedCopy but we generate the activeConfiguration on the encode workqueue. So the strings needs to work across threads.

webkit-early-warning-system · 2024-12-11T00:38:30Z

EWS run on previous version of this PR (hash c860488)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	~~🛠 ios~~	✅ 🛠 mac	~~🛠 wpe~~	~~🛠 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	~~🧪 win-tests~~
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🧪 api-mac	~~🧪 api-wpe~~
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 wpe-cairo
	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🛠 gtk
	✅ 🛠 vision	⏳ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	~~🛠 vision-sim~~	~~🧪 mac-wk2-stress~~	~~🧪 api-gtk~~
	⏳ 🧪 vision-wk2	⏳ 🧪 mac-intel-wk2
	✅ 🛠 tv	~~🛠 mac-safer-cpp~~
	~~🛠 tv-sim~~
	~~🛠 watch~~
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-12-11T01:40:12Z

EWS run on previous version of this PR (hash 76211bc)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	~~🛠 ios~~	✅ 🛠 mac	~~🛠 wpe~~	~~🛠 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	~~🧪 win-tests~~
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🧪 api-mac	~~🧪 api-wpe~~
	⏳ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	~~🛠 wpe-cairo~~
	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	~~🛠 gtk~~
	~~🛠 vision~~	⏳ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	~~🛠 vision-sim~~	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
	⏳ 🧪 vision-wk2	⏳ 🧪 mac-intel-wk2
	~~🛠 tv~~	~~🛠 mac-safer-cpp~~
	~~🛠 tv-sim~~
	~~🛠 watch~~
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-12-11T01:59:18Z

EWS run on previous version of this PR (hash 27cefcf)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-12-12T07:47:47Z

EWS run on previous version of this PR (hash 2b0ba8e)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

youennf · 2024-12-12T12:41:36Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+
+    bool m_isClosed WTF_GUARDED_BY_CAPABILITY(queueSingleton()) { false };
+    const AudioEncoder::Config m_config;
+    String m_codecString; // set during initialisation, const after.


It is not great to use strings with work queues, I would tend to use an enum instead.

I had planned to do it in a followup

I've removed it fully, WebCodecsAudioEncoder turned out to copy the original string if none is provided in the new description

youennf · 2024-12-12T12:43:27Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+    std::optional<MediaTime> m_frameDuration; // set during initialisation, const after.
+    std::optional<unsigned> m_complexity; // set during initialisation, const after.
+    std::optional<unsigned> m_packetlossperc; // set during initialisation, const after.
+    std::optional<bool> m_useinbandfec; // set during initialisation, const after.


It would be better to have a function that returns all these parameters and then we create the InternalAudioEncoderCocoa.
That way, we can make them const.

that was more effort than worth imho.
but done

youennf · 2024-12-12T12:43:43Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+    std::optional<unsigned> m_packetlossperc; // set during initialisation, const after.
+    std::optional<bool> m_useinbandfec; // set during initialisation, const after.
+    std::optional<CAAudioStreamDescription> m_inputDescription WTF_GUARDED_BY_CAPABILITY(queueSingleton());
+    bool m_isADTS { false };


Probably set at init time. Maybe add the same comment or make it atomic otherwise.

not used yet. I'll add a link to the bwo

I've removed it completely for now

youennf · 2024-12-12T12:46:50Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+        CMBlockBufferRef rawBuffer = PAL::CMSampleBufferGetDataBuffer(cmSample.get());
+        ASSERT(rawBuffer);
+        RetainPtr buffer = rawBuffer;
+        // Make sure block buffer is contiguous.


We have this pattern in a few place, it might be handy to add a utility someday.

youennf · 2024-12-12T12:47:36Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+        if (!PAL::CMBlockBufferIsRangeContiguous(rawBuffer, 0, 0)) {
+            CMBlockBufferRef contiguousBuffer;
+            if (auto error = PAL::CMBlockBufferCreateContiguous(nullptr, rawBuffer, nullptr, nullptr, 0, 0, 0, &contiguousBuffer)) {
+                RELEASE_LOG_ERROR(MediaStream, "Couldn't create buffer with error %d", error);


Shouldn't we error the encoder in this case (and below) by rejecting the encode native promise?

I don't think it can ever happen. It will OOM first

youennf · 2024-12-12T12:50:53Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+        return EncodePromise::createAndReject("Couldn't retrieve AudioData's basic description"_s);
+
+    if (*asbd != m_inputDescription) {
+        if (m_converter)


Suggested change

if (m_converter)

if (auto converter = std::exchange(m_converter, { }))

youennf · 2024-12-12T12:52:54Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+
+    InternalAudioEncoderCocoa::queueSingleton().dispatch([weakEncoder = ThreadSafeWeakPtr { *encoder }] {
+        if (auto strongEncoder = weakEncoder.get())
+            strongEncoder->processEncodedOutputs();


It is not clear to me that processEncodedOutputs() will be executed before we resolve the encode native promise. Maybe we should keep the native promise producer somewhere and resolve/reject as part of processEncodedOutputs (and handle the case where processEncodedOutputs is not called).

This is always called before because we return the promise itself which is resolved after the frames have returned. Same pattern as AudioDecoder

I added a call to processEncodedOutput() at the completion of the AudioSampleBufferConverter promise to remove all doubts.

youennf · 2024-12-12T12:53:30Z

Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp

+        return GenericPromise::createAndResolve(); // No frame encoded yet, nothing to flush;
+    return converter()->drain()->whenSettled(queueSingleton(), [protectedThis = Ref { *this }] {
+        assertIsCurrent(queueSingleton());
+        protectedThis->processEncodedOutputs();


If this fails, maybe we should fail the flush promise.

This promise is a no op in practice it can’t fail

webkit-early-warning-system · 2024-12-13T06:47:01Z

EWS run on current version of this PR (hash 85d240d)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

…ec's AudioEncoder https://bugs.webkit.org/show_bug.cgi?id=284019 rdar://140889671 Reviewed by Youenn Fablet. We add AudioEncoderCocoa, AudioToolbox implementation of WebCodec's AudioEncoder. We only support encoding to Opus and AAC as the framework doesn't provide encoder for Flac, mp3 and vorbis (decoder only) Enabled WPT tests. * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/webcodecs/audio-encoder-codec-specific.https.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/webcodecs/audio-encoder-config.https.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/webcodecs/audio-encoder-config.https.any.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/webcodecs/audio-encoder.https.any-expected.txt: * LayoutTests/platform/glib/TestExpectations: Remove passing expectations, add the remaining ones that do fail. * LayoutTests/platform/glib/imported/w3c/web-platform-tests/webcodecs/audio-encoder-config.https.any-expected.txt: Removed. * LayoutTests/platform/glib/imported/w3c/web-platform-tests/webcodecs/audio-encoder-config.https.any.worker-expected.txt: Removed. * LayoutTests/platform/ios/imported/w3c/web-platform-tests/webcodecs/audio-encoder-codec-specific.https.any-expected.txt: Added. The only failure remaining is related to different default for Opus encoding which yield slightly different values to what expected. The test performs very rough float comparisons. * LayoutTests/platform/ios/imported/w3c/web-platform-tests/webcodecs/audio-encoder.https.any-expected.txt: Added. * LayoutTests/platform/mac/imported/w3c/web-platform-tests/webcodecs/audio-encoder-codec-specific.https.any-expected.txt: Added. * LayoutTests/platform/mac/imported/w3c/web-platform-tests/webcodecs/audio-encoder.https.any-expected.txt: Added. * Source/WebCore/Modules/webcodecs/OpusEncoderConfig.h: * Source/WebCore/Modules/webcodecs/WebCodecsAudioEncoder.cpp: (WebCore::isSupportedEncoderCodec): Add additional checks to ensure that the values provided in the Opus config are sane. While this isn't probably the best place to do so, as this is common for both GStreamer and Cocoa AudioEncoder, it's the simplest. (WebCore::isValidEncoderConfig): Make sure sampleRate and numberOfChannels aren't 0. (WebCore::createAudioEncoderConfig): Increase code readability by using struct's named member initialisation. (WebCore::WebCodecsAudioEncoder::configure): (WebCore::WebCodecsAudioEncoder::encode): Both Firefox and Chromes rejects AudioData that has a different sampleRate or numberOfChannels. While we did the same, we didn't process the errors as per spec which requires that we also change the state to Closed just before queueing the error. (WebCore::WebCodecsAudioEncoder::isConfigSupported): * Source/WebCore/PlatformMac.cmake: * Source/WebCore/SourcesCocoa.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/platform/AudioEncoder.cpp: (WebCore::AudioEncoder::create): Plumb AudioEncoderCocoa. * Source/WebCore/platform/AudioEncoder.h: * Source/WebCore/platform/AudioEncoderActiveConfiguration.h: Add default initialiser to comply with more recent clang version. It allows to make the struct member initialisation optional. * Source/WebCore/platform/audio/cocoa/AudioDecoderCocoa.cpp: (WebCore::InternalAudioDecoderCocoa::initialize): Change default AudioData creation to use interleaved frames instead of planar. A few tests has this expectations, even though this isn't mandated by the specs. * Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.cpp: Added. (WebCore::InternalAudioEncoderCocoa::create): (WebCore::InternalAudioEncoderCocoa::reset): (WebCore::InternalAudioEncoderCocoa::converter): (WebCore::InternalAudioEncoderCocoa::queueSingleton): (WebCore::AudioEncoderCocoa::create): (WebCore::AudioEncoderCocoa::AudioEncoderCocoa): (WebCore::AudioEncoderCocoa::~AudioEncoderCocoa): (WebCore::AudioEncoderCocoa::encode): (WebCore::AudioEncoderCocoa::flush): (WebCore::AudioEncoderCocoa::reset): (WebCore::AudioEncoderCocoa::close): (WebCore::InternalAudioEncoderCocoa::InternalAudioEncoderCocoa): (WebCore::InternalAudioEncoderCocoa::initialize): (WebCore::InternalAudioEncoderCocoa::compressedAudioOutputBufferCallback): (WebCore::InternalAudioEncoderCocoa::generateDecoderDescriptionFromSample const): (WebCore::InternalAudioEncoderCocoa::activeConfiguration const): (WebCore::InternalAudioEncoderCocoa::processEncodedOutputs): (WebCore::InternalAudioEncoderCocoa::encode): (WebCore::InternalAudioEncoderCocoa::flush): (WebCore::InternalAudioEncoderCocoa::close): * Source/WebCore/platform/audio/cocoa/AudioEncoderCocoa.h: Copied from Source/WebCore/platform/AudioEncoderActiveConfiguration.h. Canonical link: https://commits.webkit.org/287787@main

webkit-commit-queue · 2024-12-13T09:02:37Z

Committed 287787@main (c1b41e0): https://commits.webkit.org/287787@main

Reviewed commits have been landed. Closing PR #37699 and removing active labels.

jyavenard self-assigned this Dec 10, 2024

jyavenard added the Media Bugs related to the HTML 5 Media elements. label Dec 10, 2024

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 10, 2024

jyavenard removed the merging-blocked Applied to prevent a change from being merged label Dec 10, 2024

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from 9224027 to e61db94 Compare December 10, 2024 10:55

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from e61db94 to e3088e4 Compare December 10, 2024 11:06

jyavenard changed the title ~~Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder~~ A Dec 10, 2024

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from e3088e4 to 71de9c9 Compare December 10, 2024 11:26

jyavenard changed the title A Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder Dec 10, 2024

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 10, 2024

jyavenard removed the merging-blocked Applied to prevent a change from being merged label Dec 10, 2024

jyavenard changed the title ~~Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder~~ A Dec 10, 2024

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from 71de9c9 to f591872 Compare December 10, 2024 13:33

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 10, 2024

jyavenard changed the title A Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder Dec 10, 2024

jyavenard requested review from youennf and jernoble December 10, 2024 14:54

jyavenard marked this pull request as ready for review December 10, 2024 14:54

jyavenard removed the merging-blocked Applied to prevent a change from being merged label Dec 10, 2024

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from f591872 to 44a1c71 Compare December 10, 2024 14:55

jyavenard changed the title ~~Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder~~ A Dec 10, 2024

jyavenard changed the title A Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder Dec 10, 2024

jyavenard changed the title ~~Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder~~ A Dec 10, 2024

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from 44a1c71 to a3d0d86 Compare December 10, 2024 15:03

jyavenard changed the title A Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder Dec 10, 2024

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 10, 2024

youennf reviewed Dec 10, 2024

View reviewed changes

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from a3d0d86 to c860488 Compare December 11, 2024 00:38

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from c860488 to 76211bc Compare December 11, 2024 01:40

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from 76211bc to 27cefcf Compare December 11, 2024 01:59

WebKit deleted a comment from webkit-ews-buildbot Dec 11, 2024

jyavenard removed the merging-blocked Applied to prevent a change from being merged label Dec 12, 2024

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from 27cefcf to 2b0ba8e Compare December 12, 2024 07:47

youennf approved these changes Dec 12, 2024

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 12, 2024

jyavenard removed the merging-blocked Applied to prevent a change from being merged label Dec 13, 2024

jyavenard force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from 2b0ba8e to 85d240d Compare December 13, 2024 06:46

jyavenard added the merge-queue Applied to send a pull request to merge-queue label Dec 13, 2024

webkit-commit-queue force-pushed the bz/284019_Add_AudioEncoderCocoa_class_main branch from 85d240d to c1b41e0 Compare December 13, 2024 09:02

webkit-commit-queue merged commit c1b41e0 into WebKit:main Dec 13, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 13, 2024

	if (m_converter)
	if (auto converter = std::exchange(m_converter, { }))

Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder #37699

Add AudioEncoderCocoa class, an AudioToolbox implementation of WebCodec's AudioEncoder #37699

Conversation

jyavenard commented Dec 10, 2024 • edited by webkit-early-warning-system Loading

webkit-early-warning-system commented Dec 10, 2024 • edited Loading

webkit-early-warning-system commented Dec 10, 2024 • edited Loading

webkit-early-warning-system commented Dec 10, 2024 • edited Loading

webkit-early-warning-system commented Dec 10, 2024 • edited Loading

webkit-early-warning-system commented Dec 10, 2024 • edited Loading

webkit-early-warning-system commented Dec 10, 2024 • edited Loading

webkit-early-warning-system commented Dec 10, 2024 • edited Loading

youennf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Dec 11, 2024 • edited Loading

webkit-early-warning-system commented Dec 11, 2024 • edited Loading

webkit-early-warning-system commented Dec 11, 2024 • edited Loading

webkit-early-warning-system commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Dec 13, 2024 • edited Loading

webkit-commit-queue commented Dec 13, 2024

jyavenard commented Dec 10, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Dec 10, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 10, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 10, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 10, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 10, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 10, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 10, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 11, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 11, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 11, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 12, 2024 •

edited

Loading

webkit-early-warning-system commented Dec 13, 2024 •

edited

Loading