Fix metainfo processing, AAC decoding, some domestic code clearance #8

Initially code entirely taken and adopted from k3b upstream
1e09c7d77f
with subsequent fix of the read() function and reducing of the
compiler warnings.

fixes #7

I want to note that the plugin state is still far to stable (see added comments) and have faint chance to be polished in the KDE5 branch (it's broken entirely), futhermore, FFmpeg usage is difficult and it's API is also fluent. Probably better way will be making a xine-lib decoder for this task.

Hi Mashiro, thanks for the PR. Either Slavek or I will take a look and either merge or come back with feedback, as soon as we make time for it. Please be patient 😄

The PR looks good and builds fine on bullseye. we will need @SlavekB to test build on older distros.

Also as explain in #7, I can't reproduce this bug, so if @Mashiro can provide a sample file for that, it would be great.

I did a test on Debian 7.x (Wheezy), which we still support, and there was FTBFS. I'll take a closer look at what causes it later.

thanks for testing Slavek 😄

There are two problems:

Using nullptr is not possible on older distributions without enforcing the C++ dialect. Enforcing a particular dialect is a matter that requires caution. Some time ago, we already had to get rid of it in the common admin module, because it enforced a standard that was now obsolete.
The patch breaks compatibility with older versions of FFMPEG. The code needs to be modified to include #ifdefs to ensure compatibility between versions of ffmpeg.

Sadly, but i can't make any edits now due to lack of time, so this patch potentionally can be abandoned.

The patch breaks compatibility with older versions of FFMPEG.

Hmm, i've googled for the versions first when wrote #ifdef version checks. Maybe there an issue with function defines?

No worries Mashiro. We will work on this again at some point, no reason to abandon the patch. It works on newer distros, so we just need to find the time and fix it up for older ones.

some notes before i forget about:

av_frame_alloc() came in ffmpeg with 55,28,1 ; if I'm not mistaken.

something like this, might do

K3bFFMpegFile::K3bFFMpegFile(const TQString &filename) : m_filename(filename) {
  d = new Private;
  d->formatContext = 0;
  d->codec = 0;
  d->audio_stream = 0;
#if LIBAVCODEC_VERSION_INT >= AV_VERSION_INT(55,28,1)
  d->frame = av_frame_alloc();
#else
  d->frame = avcodec_alloc_frame();
#endif
}

"codecpar->format" is probably "codec->sample_fmt", see commit https://github.com/FFmpeg/FFmpeg/commit/955b818 for reference.

some notes before i forget about:

👍 we should try to include this for R14.0.9 since most of the work has been done already

...we should try to include this for R14.0.9...

Agreed, don't let Mashiro's good job to sleep for too long. This deserves to be in R14.0.9 👍

Hi @Mashiro, apologies if this patch has sit here for so long. I am going to renew the effort in the coming days, so we can add it in time for R14.0.11 release.

EDIT: patch has been rebase on top of current master before relooking into it.

@MicheleC no worries, it's still great!

some notes before i forget about:

av_frame_alloc() came in ffmpeg with 55,28,1 ; if I'm not mistaken.

Debian Jessie is on v56 but ubuntu trusty is on v52, xenial on v54 and bionic on v55. So definitely we need some conditional code there.

Commit e24ff5c0cf replaces nullptr with NULL since at the moment we are not supporting c++11 standard (this may change after R14.0.11 release). The commit can be reverted once support for c++11 is added.

av_frame_alloc() came in ffmpeg with 55,28,1 ; if I'm not mistaken.

This seems to come with version 2.0 (52,38,100) of libavutil. It seems version numbers between libavutil and libavcodec are not really aligned. Version 2.0 of avcodec is 55,18,102, which makes tracing function introduction/removal more challenging.
Good news is that now I can build in trusty, so once I work out the various function call - version relationship, I should have a patch ready for testing in other distro versions too.

av_frame_alloc() came in ffmpeg with 55,28,1 ; if I'm not mistaken.

This seems to come with version 2.0 (52,38,100) of libavutil. It seems version numbers between libavutil and libavcodec are not really aligned. Version 2.0 of avcodec is 55,18,102, which makes tracing function introduction/removal more challenging.
Good news is that now I can build in trusty, so once I work out the various function call - version relationship, I should have a patch ready for testing in other distro versions too.

Each of the modules (avcodec, avformat, avutil,…) has its own version numbering. Therefore, there must be a test for version of a particular module in which the appropriate API belongs.

With the latest commit I am able to build successfully in both debian bookworm and ubuntu trusty.
@SlavekB could you test building on all supported distros to make sure I got the various versions in the #if blocks correct?

I have also checked on both bookworm and trusty that k3b does not crash any more with the offending file uploaded on issue #7.

I did a small adjustment:

For av_frame_alloc and av_frame_free I used the same conditions, such as in akode => dependent on avcodec version instead of avutil.
For CODEC_ID_* => AV_CODEC_ID_* I added two missing definitions.

Building was successful on all supported distributions (only amd64 was tested).

Now everything looks good.

For av_frame_alloc and av_frame_free I used the same conditions, such as in akode => dependent on avcodec version instead of avutil.

Those functions are part of libavutil, which is why the condition was on avutil instead of avcodec. Using avcoded seems not correct to me, although it may just work fine.

For av_frame_alloc and av_frame_free I used the same conditions, such as in akode => dependent on avcodec version instead of avutil.

Those functions are part of libavutil, which is why the condition was on avutil instead of avcodec. Using avcoded seems not correct to me, although it may just work fine.

These functions were previously part of the avcodec, subsequently moved to avutil. Therefore, both views of the matter are valid. For free function, there are two levels of changes where the first was within avcodec. So, the first condition must be according to avcodec, for second condition, it is possible to use any of them – avcodec or avutil. As I mentioned, I used the conditions so that it is the same manner as in akode.

Agree, as discussed on jabber as well. I will merge as is.

@Mashiro thanks for your contribution and sorry if it took so long before we merged it.

Feel free to propose more changes, if you have any. Will try to be more diligent next time 😄

Fix metainfo processing, AAC decoding, some domestic code clearance #8

Reviewers