Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#12720 closed bug (fixed)

Garbled audio due to incorrect parameters on some codecs

Reported by: anevilyak Owned by: pulkomandy
Priority: normal Milestone: Unscheduled
Component: Add-Ons/Media Version: R1/Development
Keywords: ffmpeg Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

I haven't yet been able to bisect what revision this regression was introduced in, but at least in hrev50216 AAC and Ogg Vorbis decoding appears to be broken. The audio plays with stuttering artifacts, which appears to be due to something along the chain deciding that the format sample size is 32 bits rather than 16. This occurs for both standalone audio files and audio tracks in video files. This problem is not observable with either MPEG 2 or 3 audio.

Change History (11)

comment:1 by anevilyak, 3 years ago

Cc: pulkomandy added
Owner: changed from nobody to korli
Status: newassigned

comment:2 by anevilyak, 3 years ago

I should add that this was on an x86 primary host, ergo gcc5 and respective ffmpeg package.

comment:3 by korli, 3 years ago

Tested on x86_64, works on hrev49856, doesn't on hrev50072.

comment:4 by korli, 3 years ago

Cc: pulkomandy removed
Owner: changed from korli to pulkomandy

on x86_64, I tested with success this patch: it simply skips libswr, and interleaves samples.

diff --git a/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp b/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp
index 27ec813..a18f4d9 100644
--- a/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp
+++ b/src/add-ons/media/plugins/ffmpeg/AVCodecDecoder.cpp
@@ -927,6 +927,7 @@ AVCodecDecoder::_MoveAudioFramesToRawDecodedAudioAndUpdateStartTimes()
        // Some decoders do not support format conversion on themselves, or use
        // "planar" audio (each channel separated instead of interleaved samples).
        // In that case, we use swresample to convert the data
+#if 0
        if (av_sample_fmt_is_planar(fContext->sample_fmt)) {
                const uint8_t* ptr[8];
                for (int i = 0; i < 8; i++) {
@@ -950,6 +951,21 @@ AVCodecDecoder::_MoveAudioFramesToRawDecodedAudioAndUpdateStartTimes()
 
                if (frames < 0)
                        debugger("resampling failed");
+#endif
+       if (fOutputFrameSize != fInputFrameSize) {
+               // planar audio
+               uintptr_t out = (uintptr_t)fRawDecodedAudio->data[0];
+               int32 offset = fDecodedDataBufferOffset;
+               for (int i = 0; i < frames; i++) {
+                       for (int j = 0; j < fContext->channels; j++) {
+                               memcpy((void*)out, fDecodedDataBuffer->data[j]
+                                       + offset, fInputFrameSize);
+                               out += fInputFrameSize;
+                       }
+                       offset += fInputFrameSize;
+               }
+               outFrames = frames;
+               inFrames = frames;
        } else {
                memcpy(fRawDecodedAudio->data[0], fDecodedDataBuffer->data[0]
                                + fDecodedDataBufferOffset, frames * fOutputFrameSize);

comment:5 by pulkomandy, 3 years ago

I would still use av_sample_fmt_is_planar to detect planar formats, instead of trying to guess from the input/output sizes.

Also, the libswr is able to do other conversions (float<>int or whatever), which may be required by some codecs where only float output is supported.

So, this needs careful testing with a lot of audio/video files (for example there are a lot at http://samples.ffmpeg.org), to make sure we don't need something else.

It's a bit sad that we can't get libswr working and we have to rewrite it, however :/

in reply to:  5 comment:6 by korli, 3 years ago

Thanks for the feedback!

I would still use av_sample_fmt_is_planar to detect planar formats, instead of trying to guess from the input/output sizes.

Yeah I guess.

Also, the libswr is able to do other conversions (float<>int or whatever), which may be required by some codecs where only float output is supported.

AFAIK the plugin maps the avcodec sample format to the media kit sample format, this means we don't require any format. I don't understand what might be required then.

So, this needs careful testing with a lot of audio/video files (for example there are a lot at http://samples.ffmpeg.org), to make sure we don't need something else.

Sure.

It's a bit sad that we can't get libswr working and we have to rewrite it, however :/

Yeah, or avfilter. In this case though, we don't really need some audio conversion, just another structure. IMO it's not too bad if we can skip a dependency on libswr.

comment:7 by pulkomandy, 3 years ago

We always request a sample format that matches what we want to output, but some codecs in ffmpeg may ignore the request (we also request non-planar output, and they ignore that). It depends on the format used, and also on the version of ffmpeg.

This is why I tried to go with swr: it would solve all the possible mismatches and always make sure we ouput exactly what was requested. It is a bit nicer and more predictible for apps and they don't have to do the format conversion on their side this way.

But, as it doesn't work reliably, we can go with a simple rematrixing as you did, it should cover most cases for now.

comment:8 by korli, 3 years ago

Applied in hrev50639.

comment:9 by pulkomandy, 3 years ago

Resolution: fixed
Status: assignedclosed

I guess that fixes the issue (no working soundcard on this laptop so can't be sure).

comment:10 by anevilyak, 3 years ago

I'll try to verify this locally tonight.

comment:11 by khallebal, 3 years ago

I can confirm that the stuttering is gone, i also mentioned this behaviour with AAC,FLAC, AC3 & OGG but not with MP3 in my ticket #12594.

Note: See TracTickets for help on using tickets.