Bug 227110

Summary: [WebAudio] Add webm/opus container support
Product: WebKit Reporter: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Component: Web AudioAssignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, annulen, ashley, calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, peng.liu6, philipj, ryuan.choi, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226922
https://bugs.webkit.org/show_bug.cgi?id=228139
https://bugs.webkit.org/show_bug.cgi?id=228221
https://bugs.webkit.org/show_bug.cgi?id=229129
https://bugs.webkit.org/show_bug.cgi?id=229799
Bug Depends on: 227205, 227208, 227250    
Bug Blocks: 227111    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jean-Yves Avenard [:jya] 2021-06-17 00:36:39 PDT
WebAudio currently only works with content AudioToolBox/CoreMedia supports; and it doesn't support webm.
Comment 1 Radar WebKit Bug Importer 2021-06-17 00:38:14 PDT
<rdar://problem/79437067>
Comment 2 Jean-Yves Avenard [:jya] 2021-07-21 00:21:24 PDT
Will do opus first, Vorbis doesn't work for multiple reasons and will be handled in a different bug
Comment 3 Jean-Yves Avenard [:jya] 2021-07-21 20:20:15 PDT
Created attachment 433984 [details]
Patch
Comment 4 Jean-Yves Avenard [:jya] 2021-07-21 21:33:35 PDT
Created attachment 433988 [details]
Patch

rebase; fix compilation on tvOS and watchOS fix linker error when < BigSur
Comment 5 Jean-Yves Avenard [:jya] 2021-07-21 21:42:45 PDT
Created attachment 433989 [details]
Patch

duh, fix compilation on tvOS and watchOS #2
Comment 6 Jean-Yves Avenard [:jya] 2021-07-21 22:07:37 PDT
Created attachment 433990 [details]
Patch

Make WebM code conditional on MEDIA_SOURCE as SourceBufferParserWebM is only included with that
Comment 7 Jean-Yves Avenard [:jya] 2021-07-21 22:10:13 PDT
Created attachment 433991 [details]
Patch

Make WebM code conditional on MEDIA_SOURCE as SourceBufferParserWebM is only included with that
Comment 8 Jean-Yves Avenard [:jya] 2021-07-21 23:18:01 PDT
Created attachment 433993 [details]
Patch

Fix compilation #4
Comment 9 Jean-Yves Avenard [:jya] 2021-07-22 00:31:54 PDT
Created attachment 433995 [details]
Patch

webm sample files got dropped during rebase :(
Comment 10 Jean-Yves Avenard [:jya] 2021-07-22 03:49:28 PDT
Created attachment 434003 [details]
Patch

fix vorbis decoding error (was missing magic cookie), avoid intermediate buffer
Comment 11 Jean-Yves Avenard [:jya] 2021-07-22 04:00:53 PDT
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
Comment 12 Alicia Boya García 2021-07-22 04:18:04 PDT
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?
Comment 13 Jean-Yves Avenard [:jya] 2021-07-22 04:49:07 PDT
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
Comment 14 Jean-Yves Avenard [:jya] 2021-07-22 04:52:47 PDT
(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.
Comment 15 Jean-Yves Avenard [:jya] 2021-07-22 05:48:05 PDT
Created attachment 434007 [details]
Patch

Apply comment, can just do static_cast to MediaSampleAVFObjC safely
Comment 16 Jean-Yves Avenard [:jya] 2021-07-22 17:27:32 PDT
Created attachment 434046 [details]
Patch

hopefully fix compilation on windows
Comment 17 Jean-Yves Avenard [:jya] 2021-07-22 21:09:20 PDT
Created attachment 434060 [details]
Patch

Window compilation fix #2. Manually edit the FeatureFlags plist to reduce diff size
Comment 18 Eric Carlson 2021-07-23 04:46:51 PDT
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.
Comment 19 Peng Liu 2021-07-23 10:36:11 PDT
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?
Comment 20 Jean-Yves Avenard [:jya] 2021-07-25 07:02:54 PDT
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)
Comment 21 Jean-Yves Avenard [:jya] 2021-07-25 17:17:50 PDT
Created attachment 434186 [details]
Patch

Apply comments
Comment 22 Jean-Yves Avenard [:jya] 2021-07-25 18:12:21 PDT
Created attachment 434187 [details]
Patch

fix tvos compilation
Comment 23 Jean-Yves Avenard [:jya] 2021-07-25 18:59:13 PDT
Created attachment 434189 [details]
Patch

fix tvos compilation, with the right patch it works better
Comment 24 Jean-Yves Avenard [:jya] 2021-07-26 00:57:07 PDT
Created attachment 434193 [details]
Patch

Reduce size of patch a bit, make methods const
Comment 25 Jean-Yves Avenard [:jya] 2021-07-26 01:22:11 PDT
Created attachment 434195 [details]
Patch

tvos again
Comment 26 Jean-Yves Avenard [:jya] 2021-07-26 02:51:50 PDT
Created attachment 434196 [details]
Patch

opus.webm and vorbis.webm got dropped somehow from patch
Comment 27 Jer Noble 2021-07-27 16:28:23 PDT
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/
Comment 28 Jean-Yves Avenard [:jya] 2021-07-27 16:39:01 PDT
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.
Comment 29 Jean-Yves Avenard [:jya] 2021-07-28 18:50:03 PDT
Created attachment 434485 [details]
Patch

Apply comments
Comment 30 Jean-Yves Avenard [:jya] 2021-07-28 18:53:08 PDT
Created attachment 434487 [details]
Patch

Apply comments
Comment 31 EWS 2021-07-28 20:23:03 PDT
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Comment 32 Jean-Yves Avenard [:jya] 2021-07-28 20:26:50 PDT
Created attachment 434493 [details]
Patch

Fix changelog
Comment 33 EWS 2021-07-28 21:18:23 PDT
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].