New issue
Advanced search Search tips

Issue 668999 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

AAC decoding leaves a gap at the start/end

Project Member Reported by jakearchibald@chromium.org, Nov 28 2016

Issue description

https://jakearchibald.github.io/aac-decode-bug/

Decoding an AAC leaves a gap at the end of the buffer.

It used to leave a gap at the start too (bug in stable, fixed in Canary). I tried to bisect this, but I guess we don't support AAC in Chromium?

Firefox and Edge have the same bug, so I guess it's a common component. Safari gets it right.
 
I've been chatting to Paul Adenot of Mozilla, and he suggests that the gap at the start of the audio is added by the encoder, and it's visible through ffprobe. Question is, should web audio retain this padding on decode? Firefox does, Safari doesn't.

Comment 2 by rtoy@chromium.org, Nov 28 2016

The last time I looked at this, ffmpeg does this.  In some special cases ffmpeg will remove the initial and trailing gaps, but not always.  In particular, if the LAME encoder is used enough information is left in the encoded file for ffmpeg to remove the initial gap (pre-roll or priming frames) but not the trailing (remainder) frames.

There's very little WebAudio can do about this since we depend on ffmpeg for the decoding. (Note that Android's media decoder removed these, but WebAudio no longer uses that; ffmpeg is used everywhere now.)

Comment 3 by rtoy@chromium.org, Nov 28 2016

Oh.  Just tried the test URL using today's ToT chromium. AAC decoding no longer has the priming frames (but the remainder frames are still there).  And Opus and FLAC are supported now too.
There's still a small non-zero gap at the start in ToT.

Anyway, I think there's more to discuss here, so I've opened an issue with the spec https://github.com/WebAudio/web-audio-api/issues/1091

Comment 5 by rtoy@chromium.org, Jan 17 2017

Status: Available (was: Untriaged)

Comment 6 by rtoy@chromium.org, Sep 13 2017

Status: WontFix (was: Available)
Closing as WontFix(WAI).

WebAudio takes whatever ffmpeg decodes.  If ffmpeg fixes the decoders to remove the priming frames and the trailing frames, this will be fixed.  But until then, we don't plan on doing anything about this.
Huh, this seems pretty bad for cross-browser compatibility. Have we reached out to ffmpeg about it?
Status: ExternalDependency (was: WontFix)
Can we at least track this?
Hmm, this seems like ffmpeg is actually eating valid data since it's present in the .wav file.

I feel like we would be seeing more issues if that was happening everywhere, so I wonder if it's specific to this file. Have you tried re-encoding+remuxing the .wav into a .m4a file with a more recent ffmpeg version?
I didn't use ffmpeg, I used "afconvert -s 3 -f mp4f tiny-clip.wav" on mac. I just tried it with a more recent version and Chrome still sees a gap that Safari doesn't.

I've updated https://jakearchibald.github.io/aac-decode-bug/ so you can now drag & drop your own audio files into it.
Components: Internals>Media>FFmpeg
Owner: dalecur...@chromium.org
Status: Assigned (was: ExternalDependency)
Thanks will take a look to make sure we understand what's going awry here.
Last packet is < 1024 samples, but ffmpeg still outputs the full 1024:

[PACKET]
codec_type=audio
stream_index=0
pts=29632
pts_time=0.617333
dts=29632
dts_time=0.617333
duration=379
duration_time=0.007896
convergence_duration=N/A
convergence_duration_time=N/A
size=6
pos=15263
flags=K_
[/PACKET]

[mov,mp4,m4a,3gp,3g2,mj2 @ 0x1263574c4200] stream 0, sample 31, dts 617333
[1:3:0622/134917.862536:ERROR:audio_file_reader.cc(221)] OnNewFrame: 1024, 0.379ms

This patch clipping the frames read value to the packet duration works, but feels a bit risky since I'm not sure it's always valid. I'll double check with the ffmpeg folks if the aac decoder is expected to always output 1024 samples regardless of duration. I think some encoders specify a trim value for the end in such cases.

diff --git a/media/filters/audio_file_reader.cc b/media/filters/audio_file_reader.cc
index ae37ab7b3753..b95d55002448 100644
--- a/media/filters/audio_file_reader.cc
+++ b/media/filters/audio_file_reader.cc
@@ -215,10 +215,13 @@ bool AudioFileReader::OnNewFrame(
     int* total_frames,
     std::vector<std::unique_ptr<AudioBus>>* decoded_audio_packets,
     AVFrame* frame) {
-  const int frames_read = frame->nb_samples;
+  int frames_read = frame->nb_samples;
   if (frames_read < 0)
     return false;
 
+  if (audio_codec_ == kCodecAAC && frame->pkt_duration < frames_read)
+    frames_read = frame->pkt_duration;
+
   const int channels = frame->channels;
   if (frame->sample_rate != sample_rate_ || channels != channels_ ||
       frame->format != av_sample_format_) {

Project Member

Comment 13 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75fe9b33ef2ec997b32582ef801ccc15c0fb34d7

commit 75fe9b33ef2ec997b32582ef801ccc15c0fb34d7
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Tue Jun 26 18:01:30 2018

Avoid trailing silence in AAC decodes with WebAudio.

FFmpeg won't strip any trailing audio data without end discard being
specified in the container, even if the container says the duration
of the last packet is smaller than what's decoded. In fact it mostly
just has hard coded values for the number samples coming out of the
decoding process unfortunately.

This change uses the packet duration information for AAC decodes to
drop the last few samples. Hopefully at some point in the future it
is possible for ffmpeg to do something smarter internally.

There is some risk to this approach. Media with invalid duration
information may now end up losing data. We may need to restrict this
workaround to just the last packet in a given stream.

This doesn't fix AAC audio decoded via <audio> or <video> tags, only
WebAudio's decodeAudioData(). FFmpegDemuxer (src=) and ChunkDemuxer
(MSE) don't agree on how packet duration is reported when applying
discard padding, so we can't use that duration to trim the packet.
Which isn't really a problem since plain src= tags can't splice
streams and MSE requires an explicit appendWindow for correctness.

BUG= 668999 
TEST=updated unittest expectations.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I68d579a68ae476407b89f8398a892090d17c9edd
Reviewed-on: https://chromium-review.googlesource.com/1114094
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Raymond Toy <rtoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570460}
[modify] https://crrev.com/75fe9b33ef2ec997b32582ef801ccc15c0fb34d7/media/filters/audio_file_reader.cc
[modify] https://crrev.com/75fe9b33ef2ec997b32582ef801ccc15c0fb34d7/media/filters/audio_file_reader_unittest.cc
[add] https://crrev.com/75fe9b33ef2ec997b32582ef801ccc15c0fb34d7/media/test/data/440hz-10ms.m4a
[modify] https://crrev.com/75fe9b33ef2ec997b32582ef801ccc15c0fb34d7/third_party/WebKit/LayoutTests/webaudio/codec-tests/aac/m4a-short-duration-44khz-expected.wav

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e898f02e3b55adb617f9f63193491267201bd8c

commit 7e898f02e3b55adb617f9f63193491267201bd8c
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Tue Jun 26 19:27:48 2018

Fix front discard when the discard padding spans multiple packets.

FFmpeg both specifies a multi-packet discard and marks packets for
complete discard. When we overwrite the discard padding value for
complete discard, we lose information about furture packets which
may need bits discarded from them.

This resulted in us discarding 2048 out of 2112 frames instead of
the full 2112 when playing an AAC file encoded by Apple. All of
our existing tests use a 1024 frame padding instead of Apple's
much larger padding, so this patch brings in a new test case.

As noted in the test though, the hash isn't quite 100% correct,
since we don't discard post-decode silence. This isn't a huge
deal since plain src= tags can't splice streams and MSE requires
an explicit append window for correctness.

BUG= 668999 
TEST=new unittest.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I6dad531ab986c3deb7e855dddad3011b98e6d9b6
Reviewed-on: https://chromium-review.googlesource.com/1114289
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570493}
[modify] https://crrev.com/7e898f02e3b55adb617f9f63193491267201bd8c/media/filters/ffmpeg_demuxer.cc
[modify] https://crrev.com/7e898f02e3b55adb617f9f63193491267201bd8c/media/test/pipeline_integration_test.cc

Labels: M-69
Status: Fixed (was: Assigned)

Sign in to add a comment