WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
228140
[WebAudio] webm; properly trim frames according to the codec delay information
https://bugs.webkit.org/show_bug.cgi?id=228140
Summary
[WebAudio] webm; properly trim frames according to the codec delay information
Jean-Yves Avenard [:jya]
Reported
2021-07-21 00:49:41 PDT
The webm metadata contains codec delay information, that specify how many audio frames should be trimmed at the start. This is used to properly deal with the Opus and Vorbis encoder delay behaviour that make them output silence for the first few packets. Opus encoder delay is typically 45ms so we must drop that much from the start; otherwise that silence is audible and quite noticeable when you play the sound repeatedly.
Attachments
Patch
(11.62 KB, patch)
2021-07-28 22:56 PDT
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch
(17.79 KB, patch)
2021-07-30 03:31 PDT
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch
(18.91 KB, patch)
2021-07-30 04:53 PDT
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch
(18.92 KB, patch)
2021-08-01 20:53 PDT
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch
(21.51 KB, patch)
2021-08-02 20:43 PDT
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch
(21.51 KB, patch)
2021-08-02 20:51 PDT
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-07-21 00:50:05 PDT
<
rdar://problem/80883882
>
Jean-Yves Avenard [:jya]
Comment 2
2021-07-28 22:56:32 PDT
Created
attachment 434497
[details]
Patch
Jean-Yves Avenard [:jya]
Comment 3
2021-07-30 03:31:44 PDT
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
Jean-Yves Avenard [:jya]
Comment 4
2021-07-30 04:53:02 PDT
Created
attachment 434615
[details]
Patch
Jean-Yves Avenard [:jya]
Comment 5
2021-08-01 20:53:45 PDT
Created
attachment 434733
[details]
Patch Fix style
Eric Carlson
Comment 6
2021-08-02 09:13:34 PDT
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.
Jean-Yves Avenard [:jya]
Comment 7
2021-08-02 20:43:49 PDT
Created
attachment 434814
[details]
Patch Apply comments
EWS
Comment 8
2021-08-02 20:47:55 PDT
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Jean-Yves Avenard [:jya]
Comment 9
2021-08-02 20:47:57 PDT
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
Jean-Yves Avenard [:jya]
Comment 10
2021-08-02 20:51:30 PDT
Created
attachment 434816
[details]
Patch Add reviewer to Changelogs
EWS
Comment 11
2021-08-02 21:21:54 PDT
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug