Bug 227110 – [WebAudio] Add webm/opus container support

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Jean-Yves Avenard [:jya]]

WebAudio currently only works with content AudioToolBox/CoreMedia supports; and it doesn't support webm.

[Radar WebKit Bug Importer]

<rdar://problem/79437067>

[Jean-Yves Avenard [:jya]]

Will do opus first, Vorbis doesn't work for multiple reasons and will be handled in a different bug

[Jean-Yves Avenard [:jya]]

Created attachment 433984 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 433988 [details]
Patch

rebase; fix compilation on tvOS and watchOS fix linker error when < BigSur

[Jean-Yves Avenard [:jya]]

Created attachment 433989 [details]
Patch

duh, fix compilation on tvOS and watchOS #2

[Jean-Yves Avenard [:jya]]

Created attachment 433990 [details]
Patch

Make WebM code conditional on MEDIA_SOURCE as SourceBufferParserWebM is only included with that

[Jean-Yves Avenard [:jya]]

Created attachment 433991 [details]
Patch

Make WebM code conditional on MEDIA_SOURCE as SourceBufferParserWebM is only included with that

[Jean-Yves Avenard [:jya]]

Created attachment 433993 [details]
Patch

Fix compilation #4

[Jean-Yves Avenard [:jya]]

Created attachment 433995 [details]
Patch

webm sample files got dropped during rebase :(

[Jean-Yves Avenard [:jya]]

Created attachment 434003 [details]
Patch

fix vorbis decoding error (was missing magic cookie), avoid intermediate buffer

[Jean-Yves Avenard [:jya]]

Created attachment 434005 [details]
Patch

limit number of frames to be decoded at once to a sane limit as CM only deals with uint32_t sizes

[Alicia Boya García]

Comment on attachment 434005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434005&action=review

> Source/WebCore/platform/MediaSample.h:129
> +    virtual bool isMediaSampleAVFObjC() const { return false; }

Why is this necessary? Is there a second specialization of MediaSample for the Apple ports that I'm missing?

[Jean-Yves Avenard [:jya]]

Created attachment 434006 [details]
Patch

limit number of frames to be decoded at once to a sane limit as CM only deals with uint32_t sizes

[Jean-Yves Avenard [:jya]]

(In reply to Alicia Boya García from comment #12)
> Comment on attachment 434005 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434005&action=review
> 
> > Source/WebCore/platform/MediaSample.h:129
> > +    virtual bool isMediaSampleAVFObjC() const { return false; }
> 
> Why is this necessary? Is there a second specialization of MediaSample for
> the Apple ports that I'm missing?

On Mac no, but the code does get a MediaSample; seems nicer to me that way than just assuming and using static_cast.

[Jean-Yves Avenard [:jya]]

Created attachment 434007 [details]
Patch

Apply comment, can just do static_cast to MediaSampleAVFObjC safely

[Jean-Yves Avenard [:jya]]

Created attachment 434046 [details]
Patch

hopefully fix compilation on windows

[Jean-Yves Avenard [:jya]]

Created attachment 434060 [details]
Patch

Window compilation fix #2. Manually edit the FeatureFlags plist to reduce diff size

[Eric Carlson]

Comment on attachment 434060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434060&action=review

This looks OK to me, but it'd be good to have another set of eyes on it.

> Source/WTF/ChangeLog:9
> +        Add webm_webaudio preference to make canPlayType return probably for mimetype:

`webm_webaudio` is the system feature flag and the preference is `WebMWebAudioEnabled` so you probably mean the later?

> Source/WebKit/ChangeLog:9
> +        Add default value for webm_webaudio pref for the different platforms.

s/pref/feature flag/

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:187
> +#if ENABLE(MEDIA_SOURCE)
> +    if (isMaybeWebM(static_cast<const uint8_t*>(data), dataSize)) {
> +        auto parser = adoptRef(new SourceBufferParserWebM());
> +        bool error = false;
> +        std::optional<uint64_t> audioTrackId;
> +        MediaTime duration;
> +        SourceBufferParserWebM::InitializationSegment initSegment;
> +        Vector<Ref<MediaSampleAVFObjC>> samples;
> +        parser->setDidEncounterErrorDuringParsingCallback([&](uint64_t) {
> +            error = true;
> +        });
> +        parser->setDidParseInitializationDataCallback([&](SourceBufferParserWebM::InitializationSegment&& init) {
> +            for (auto& audioTrack : init.audioTracks) {
> +                if (audioTrack.track && audioTrack.track->trackUID()) {
> +                    duration = init.duration;
> +                    audioTrackId = audioTrack.track->trackUID();
> +                    initSegment = WTFMove(init);
> +                    return;
> +                }
> +            }
> +        });
> +        parser->setDidProvideMediaDataCallback([&](Ref<MediaSample>&& sample, uint64_t trackID, const String&) {
> +            if (!audioTrackId || trackID != *audioTrackId)
> +                return;
> +            samples.append(static_reference_cast<MediaSampleAVFObjC>(WTFMove(sample)));
> +        });
> +        parser->setCallOnClientThreadCallback([](WTF::Function<void()>&& function) {
> +            function();
> +        });
> +        Vector<uint8_t> vector(static_cast<const uint8_t*>(data), dataSize);
> +        SourceBufferParser::Segment segment(WTFMove(vector));
> +        parser->appendData(WTFMove(segment));
> +        if (audioTrackId) {
> +            parser->flushPendingAudioBuffers();
> +            m_webmData = makeUnique<AudioFileReaderWebMData>(AudioFileReaderWebMData { WTFMove(initSegment), WTFMove(duration), WTFMove(samples) });
> +        }
> +        return;
> +    }
> +#endif

Nit: this is big enough that I would put it into a separate method.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:253
> +static OSStatus passthroughInputDataCallback(
> +    AudioConverterRef, UInt32* numDataPackets, AudioBufferList* data, AudioStreamPacketDescription** packetDesc, void* inUserData)

Nit: WebKit style is to have the parameters on the same line as the function.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:283
> +    if (m_webmData) {

Nit: flip this logic and it can be an early return.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:298
> +            if (!asbd)
> +                return -1;

Is it worth RELEASE_LOG_FAULT-ing this unexpected failures too?

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:302
> +            if (PAL::CMSampleBufferGetAudioStreamPacketDescriptions(sampleBuffer, 0, nullptr, &packetDescriptionsSize) != noErr)
> +                return -1;

Ditto.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:305
> +            if (!numDescriptions)
> +                return -1;

Ditto.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:308
> +            if (PAL::CMSampleBufferGetAudioStreamPacketDescriptions(sampleBuffer, packetDescriptionsSize, descriptions.get(), nullptr) != noErr)
> +                return -1;

Ditto.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:443
> +        AudioConverterRef converter;
> +        if (PAL::AudioConverterNew(&*inFormat, &outFormat, &converter) != noErr)
> +            return nullptr;
> +        auto cleanup = makeScopeExit([&] {
> +            PAL::AudioConverterDispose(converter);
> +        });
> +        CMFormatDescriptionRef formatDescription = PAL::CMSampleBufferGetFormatDescription(m_webmData->m_samples[0]->sampleBuffer());

Nit: this is big enough that I would put it in a separate method.

[Peng Liu]

Comment on attachment 434060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434060&action=review

> Source/WebCore/platform/audio/AudioBus.h:87
> +    void setLength(size_t newLength);

Nit. The parameter name can be omitted.

> Source/WebCore/platform/audio/AudioChannel.h:77
> +    // Set new length. Can only be set to a value lower than the currently

Nit. s/currently set length/current length/

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:121
> +    AudioBufferList* operator*() const { return m_bufferList; }

Return a reference here?

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:128
> +static constexpr uint32_t kVorbisMaxFrameLength = 4096;

Nit. Maybe a reference or comment regarding the constants?

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:255
> +    PassthroughUserData* userData = (PassthroughUserData*)inUserData;

Can we use `auto *` here?

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:292
> +            CMSampleBufferRef sampleBuffer = sample->sampleBuffer();

Use auto?

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:293
> +            const CMFormatDescriptionRef formatDescription = PAL::CMSampleBufferGetFormatDescription(sampleBuffer);

Ditto.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:309
> +            CMItemCount numPackets = PAL::CMSampleBufferGetNumSamples(sampleBuffer);

Use auto?

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:357
> +    AudioStreamBasicDescription outFormat = inFormat;

Use auto?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.h:47
> +    WEBCORE_EXPORT RefPtr<JSC::Uint8ClampedArray> getRGBAImageData() const override;

Nit. Not a problem of this patch. I am wondering whether we can replace all these "override" with "final" here.

> Source/WebKit/FeatureFlags/WebKit-watchos.plist:93
> +        <true/>

Will we enable the feature for all Cocoa ports?

[Jean-Yves Avenard [:jya]]

Comment on attachment 434060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434060&action=review

>> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:121
>> +    AudioBufferList* operator*() const { return m_bufferList; }
> 
> Return a reference here?

not in the context in which it is used.
AudioBufferListHolder is a wrapper of AudioBufferList* not an AudioBufferList

I guess I could rename the class AudioBufferListPtrHolder to make it clearer.

>> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:128
>> +static constexpr uint32_t kVorbisMaxFrameLength = 4096;
> 
> Nit. Maybe a reference or comment regarding the constants?

actually, those are no longer used; will remove

>> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:357
>> +    AudioStreamBasicDescription outFormat = inFormat;
> 
> Use auto?

I find it makes the code less readable, particularly as we are modifying the internal structure of the object rather than using it as a temporary object that we just carried around. So prefer to leave it as-is

>> Source/WebKit/FeatureFlags/WebKit-watchos.plist:93
>> +        <true/>
> 
> Will we enable the feature for all Cocoa ports?

wherever WebM is supported I guess. in any case wherever the webm parser is enabled (as this feature flag follows the same logic)

[Jean-Yves Avenard [:jya]]

Created attachment 434186 [details]
Patch

Apply comments

[Jean-Yves Avenard [:jya]]

Created attachment 434187 [details]
Patch

fix tvos compilation

[Jean-Yves Avenard [:jya]]

Created attachment 434189 [details]
Patch

fix tvos compilation, with the right patch it works better

[Jean-Yves Avenard [:jya]]

Created attachment 434193 [details]
Patch

Reduce size of patch a bit, make methods const

[Jean-Yves Avenard [:jya]]

Created attachment 434195 [details]
Patch

tvos again

[Jean-Yves Avenard [:jya]]

Created attachment 434196 [details]
Patch

opus.webm and vorbis.webm got dropped somehow from patch

[Jer Noble]

Comment on attachment 434196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434196&action=review

Overall comments, I wonder if we should subclass AudioFileReader to AudioFileReaderWebM or something, rather than putting two implementations in the same class.

> Source/WebCore/html/HTMLMediaElement.cpp:8186
> +bool HTMLMediaElement::webMWebAudioEnabled() const
> +{
> +#if ENABLE(MEDIA_SOURCE)
> +    return RuntimeEnabledFeatures::sharedFeatures().webMWebAudioEnabled();
> +#else
> +    return false;
> +#endif
> +}
> +

Nit: this can just be a static method; no need to add it to the class header.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:206
> +    parser->setCallOnClientThreadCallback([](WTF::Function<void()>&& function) {

Nit: could just use `auto&& function` here.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:210
> +    Vector<uint8_t> vector(data, dataSize);
> +    SourceBufferParser::Segment segment(WTFMove(vector));

Nit: could this just be: `SourceBufferParser::Segment segment({data, dataSize})`? Or is type deduction not smart enough here?

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:221
> +    UInt32 m_dataSize;
> +    const void* m_data;

Could PassthroughUserData just take a CMSampleBufferRef, rather than a `void*` and a data size?

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:232
> +static OSStatus passthroughInputDataCallback(AudioConverterRef, UInt32* numDataPackets, AudioBufferList* data, AudioStreamPacketDescription** packetDesc, void* inUserData)
> +{
> +    auto* userData = static_cast<PassthroughUserData*>(inUserData);

Out of an abundance of caution, we should null-check all the inputs to this method, with ASSERT() macros to catch null inputs in tests.

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:257
> +Vector<AudioStreamPacketDescription> AudioFileReader::getPacketDescriptions(CMSampleBufferRef sampleBuffer) const
> +{

Ditto the comment about null-checking. (Not sure if the CM* methods crash or throw if fed a null CMSampleBufferRef.)

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:282
> +std::optional<size_t> AudioFileReader::decodeWebMData(AudioBufferList* bufferList, size_t numberOfFrames, const AudioStreamBasicDescription& inFormat, const AudioStreamBasicDescription& outFormat) const
> +{

Ditto null-checking (or make AudioBufferList a &-ref).

> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:291
> +    auto formatDescription = PAL::CMSampleBufferGetFormatDescription(m_webmData->m_samples[0]->sampleBuffer());

Ditto here.

/more to come/

[Jean-Yves Avenard [:jya]]

Comment on attachment 434196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434196&action=review

>> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:221
>> +    const void* m_data;
> 
> Could PassthroughUserData just take a CMSampleBufferRef, rather than a `void*` and a data size?

but then you have to re-run the code to extract the data and size every single sub-packets. Seems rather inefficient when we could just do it once.

>> Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:282
>> +{
> 
> Ditto null-checking (or make AudioBufferList a &-ref).

this is a private method, only called internally. AudioBufferList can't ever be null as it is always checked at the point of creation. I can add an assert for clarity. But it can never be null.

[Jean-Yves Avenard [:jya]]

Created attachment 434485 [details]
Patch

Apply comments

[Jean-Yves Avenard [:jya]]

Created attachment 434487 [details]
Patch

Apply comments

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Jean-Yves Avenard [:jya]]

Created attachment 434493 [details]
Patch

Fix changelog

[EWS]

Committed r280416 (240056@main): <https://commits.webkit.org/240056@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434493 [details].