Bug 386105

This is example FFmpeg decoding code
https://www.ffmpeg.org/doxygen/trunk/demuxing_decoding_8c-example.html

It has some differences from what is present in k3b ffmpeg plugin.

Another very characteristic screenshot:
https://i.imgur.com/l9wpxYG.png
comparison between corrupted file produced by k3b ffmpeg plugin (above) and normal (below)

plugins/decoder/ffmpeg/k3bffmpegwrapper.cpp:

int K3bFFMpegFile::read( char* buf, int bufLen )
{

// ...

    // TODO: only swap if needed
    for( int i = 0; i < len-1; i+=2 ) {
        char a = buf[i];
        buf[i] = buf[i+1];
        buf[i+1] = a;
    }

// ...

}

What this code actually does? It looks the most suspicious.
And what "if needed" does mean?

Hi blaze,

Thanks for your report!

> Also ffmpeg plugin currently uses deprecated API

I migrated libAV before https://github.com/isoft-linux/libKeyFrame/blob/master/libKeyFrame.c#L295  and welcome the patch about migration :)

> https://i.imgur.com/l9wpxYG.png

That is very good DIFF! it is clear that K3B ffmpeg plugin bring in the noise. but it is not easy to use FFT and Band-pass filter to filter it ...

> What this code actually does? It looks the most suspicious. And what "if needed" does mean?

I have no idea why need to SWAP, perhaps it is better to ask the original author :) https://github.com/KDE/k3b/commit/97f72a28ef87ef62f89967e237f441e03d3b5b3b#diff-edc47aa8fac740c80253cd4bec4ba15dR207


You can test to comment the SWAP code, then rebuild K3B to verify whether or not produce the noise.

Regards,
Leslie Zhai

> You can test to comment the SWAP code, then rebuild K3B to verify whether or not produce the noise

Did it but with that it's even worse. So it's not the case. I'm worriyng however about the "if needed" part.

The general idea is that decoding algorythm is broken (see the example usages of ffmpeg libavcodec). It decodes packets but does nothing to frames.

Also why K3bFFMpegFile::fillOutputBuffer() method is being called recursively? It's better to use while loop here.

Also it would be reasonable to get rid of all the legacy ffmpeg support code. The most part of it exists since KDE3 times. No point in having it now.

I found that every frame produced by fillOutputBuffer() contains (some patch of) corrupted data just right after the middle.

Also citing the FFMPEG docs:

>Some decoders (those marked with AV_CODEC_CAP_DELAY) have a delay between input and output. This means that for some packets they will not immediately produce decoded output and need to be flushed at the end of decoding to get all the decoded data. Flushing is done by calling this function (avcodec_decode_audio4()) with packets with avpkt->data set to NULL and avpkt->size set to 0 until it stops returning samples. It is safe to flush even those decoders that are not marked with AV_CODEC_CAP_DELAY, then no samples will be returned.

Experimenting further I found that if the calculated buffer size returned by av_samples_get_buffer_size() is divided by two then the resulting waveform looks normal (no noise), but the sound is twice faster than it should be. So now the question is how to slow down or stretch it. :D

One more important addition: I think that the second half of every frame is just some memory garbage as I found in previous step. But you not supposed to divide the frame size by two. The actual string where things go wrong is:

> d->outputBufferPos = reinterpret_cast<char*>( d->frame->data[0] );

This is not the way how you should read the stereo data. Maybe the second channel can be accessed with d->frame->data[1]? Not really sure, just guessing.

And one more: this plugin does not decode wma1, wma2 and aac at all, produces total garbage. And these formats supposed to be "tested", lol, according to the source comments.

Created a phabricator diff here https://phabricator.kde.org/D11634
It fixes decoding for some lossless formats.
Compressed formats like WMA and AAC still have to be attended (probably need flushing).

So here's my code which converts float audio to 16bit (for WMA and AAC):

>        for(int sample=0; sample<nb_s; sample++) {
>            for(int ch=0; ch<nb_ch; ch++) {
>                float smpl = *(reinterpret_cast<float*>(
>                    d->frame->extended_data[ch] + sample*4)) * 32767.0;
>                smpl = smpl > 32767.0 ? 32767.0 : smpl;
>                smpl = smpl < -32767.0 ? -32767.0 : smpl;
>                int16_t smpl2 = (((int16_t)(smpl + 32768.5)) - 32768);
>                ::memcpy(d->outputBufferPos + (sample * nb_ch + ch) * 2,
>                         &smpl2,
>                         2);
>            }
>        }

It works nicely: no noise or clipping. Made it just out of curiosity. But it's better to use libswresample. When it's done right it will cover all possible cases for the input audio. I'll get to that later. Stay tuned for patches.

After accepting the latest revision of https://phabricator.kde.org/D11634 this bug can be considered as fixed in the upstream for future releases.

Git commit 059d0da2e2c6865c93b7d5726d8f19830074be57 by Leslie Zhai.
Committed on 03/04/2018 at 14:45.
Pushed by lesliezhai into branch 'master'.

[plugins] K3b FFMpeg Decoder Plugin Fix

A patch by Mikołaj Płomieński!

Reviewers: lesliezhai, anthonyfieroni

Differential Revision: https://phabricator.kde.org/D11634

M  +56   -23   plugins/decoder/ffmpeg/k3bffmpegwrapper.cpp

https://commits.kde.org/k3b/059d0da2e2c6865c93b7d5726d8f19830074be57