Summary: | [WebAudio] Add webm/opus container support | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||||||||||||||||||||||||||||||||||||||||||
Component: | Web Audio | Assignee: | 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
Jean-Yves Avenard [:jya]
2021-06-17 00:36:39 PDT
Will do opus first, Vorbis doesn't work for multiple reasons and will be handled in a different bug Created attachment 433984 [details]
Patch
Created attachment 433988 [details]
Patch
rebase; fix compilation on tvOS and watchOS fix linker error when < BigSur
Created attachment 433989 [details]
Patch
duh, fix compilation on tvOS and watchOS #2
Created attachment 433990 [details]
Patch
Make WebM code conditional on MEDIA_SOURCE as SourceBufferParserWebM is only included with that
Created attachment 433991 [details]
Patch
Make WebM code conditional on MEDIA_SOURCE as SourceBufferParserWebM is only included with that
Created attachment 433993 [details]
Patch
Fix compilation #4
Created attachment 433995 [details]
Patch
webm sample files got dropped during rebase :(
Created attachment 434003 [details]
Patch
fix vorbis decoding error (was missing magic cookie), avoid intermediate buffer
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 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? 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
(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. Created attachment 434007 [details]
Patch
Apply comment, can just do static_cast to MediaSampleAVFObjC safely
Created attachment 434046 [details]
Patch
hopefully fix compilation on windows
Created attachment 434060 [details]
Patch
Window compilation fix #2. Manually edit the FeatureFlags plist to reduce diff size
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 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 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) Created attachment 434186 [details]
Patch
Apply comments
Created attachment 434187 [details]
Patch
fix tvos compilation
Created attachment 434189 [details]
Patch
fix tvos compilation, with the right patch it works better
Created attachment 434193 [details]
Patch
Reduce size of patch a bit, make methods const
Created attachment 434195 [details]
Patch
tvos again
Created attachment 434196 [details]
Patch
opus.webm and vorbis.webm got dropped somehow from patch
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 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. Created attachment 434485 [details]
Patch
Apply comments
Created attachment 434487 [details]
Patch
Apply comments
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Created attachment 434493 [details]
Patch
Fix changelog
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]. |