Summary: | [WebAudio] webm; properly trim frames according to the codec delay information | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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: | calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, philipj, sergio, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Jean-Yves Avenard [:jya]
2021-07-21 00:49:41 PDT
Created attachment 434497 [details]
Patch
Created attachment 434614 [details]
Patch
For the proper amount of frames to be returned, we need to explicitly tell the AudioConverter that we've reached EOS; this is done by returning noErr and set the number of decoded frames to 0
Created attachment 434615 [details]
Patch
Created attachment 434733 [details]
Patch
Fix style
Comment on attachment 434733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434733&action=review > Source/WebCore/ChangeLog:11 > + vorbis decoding return the right number of frames. s/return/returns/ > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:266 > + if (static_cast<char*>(data->mBuffers[0].mData) + data->mBuffers[0].mDataByteSize > userData->m_data + userData->m_dataSize) > + return kAudioConverterErr_UnspecifiedError; This is serious enough that it might be worth logging an error. > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:322 > + PAL::AudioConverterSetProperty(converter, kAudioConverterPrimeInfo, sizeof(AudioConverterPrimeInfo), &primeInfo); Nit: sizeof(primeInfo) > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:377 > + } while (status != kNoMoreDataErr && status != noErr); It seems a little weird to use "noErr" to signal eos, why not define a specific error like we did for kNoMoreDataErr? > Source/WebCore/platform/graphics/cocoa/AudioTrackPrivateWebM.cpp:93 > + return MediaTime(m_track.codec_delay.value(), 1000000000); It would be helpful to people that don't know about WebM who read this to use a named constant for 1000000000. > Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:1036 > + m_callOnClientThreadCallback([this, protectedThis = makeRef(*this), trackID = trackData->track().track_uid.value(), padding = MediaTime(blockGroup.discard_padding.value(), 1000000000)]() { Ditto. Created attachment 434814 [details]
Patch
Apply comments
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Comment on attachment 434733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434733&action=review > Source/WebCore/platform/audio/cocoa/AudioFileReaderCocoa.cpp:378 > } Unfortunately, this is required for the AudioConverter to work properly. We must return a number of packet of 0 and noErr for it to detect EOS. If we don't, any pending frames in the opus decoder will not be output and we'll be one packet (960 frames) short. This is an undocumented behaviour unfortunately and I only found out once I could check their code. The AudioConverter will return the error the callback method returned. So we have to deal with noErr Created attachment 434816 [details]
Patch
Add reviewer to Changelogs
Committed r280584 (240206@main): <https://commits.webkit.org/240206@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434816 [details]. |