Revive FFmpeg Plugin #7

This Pull Request migrates the FFmpeg decoder to FFmpeg 4.x API.

The decoder plugin has been tested while there are still some deprecation warnings.

Supersedes #6.

There will be more efforts to ensure compatibility with older versions of FFMpeg / libav, which we need to support.
See comments below.

@SlavekB Thanks for your suggestions.

I can make those changes, but I can only test them with newer FFmpeg versions since I don't know how to test them with FFmpeg 0.x on a modern GNU+Linux system.

Also, just a side question: is it allowed to use C++11 or any C++ features from C++14 or above in the TDE code? If so, what's the C++ feature ceiling? If not, which version(s) of compiler (GCC/Clang) I should stick to and test on?

Thanks.

In the structure of AvFrame, next to field pkt_size, there is a need to pay attention even field channels.

Currently, the oldest distribution for which we still provide support are Jessie and Trusty. This means GCC 4.8.x, where the default is std=gnu++98. It would be good, for the time being to stick to these standards, at most C++11, if necessary.

So to avoid mutual confusions, to my understanding, basically what I am required to do are:

This implementation needs to support FFmpeg/Libav 0.6~4.4
It needs to be able to compile under GCC 4.8+
The libraries used (if any) need to be available in Debian 8 and/or Ubuntu 14.04

Am I understand that correctly?

Basically yes. If you are unable to test on old distros, just upload the code and Slavek will be able to test and report back :-)

Btw, I have added Tianhao Chai to the Contributors group as well, just in case he needs/wants to submit PRs for other stuff.

Basically yes. If you are unable to test on old distros, just upload the code and Slavek will be able to test and report back :-)

Unfortunately, meeting all three requirements plus supporting newer distros is surprisingly difficult.

I could understand your need to keep backward-compatibility, but please also understand I am not able to make it support FFmpeg 0.6 all the way up to 4.4 (current stable) also including an incompatible fork, libav/avconv FFmpeg (too much work involved).

So in which case, I would appreciate that if you could offer some direct help with the implementation supporting ffmpeg 1.x to 3.x: considering that the decoder did not work with anything above ffmpeg 0.6 previously.

Thanks

Ok, thanks @liushuyu,
I will discuss with Slavek on the best way forward. We will probably need some version check and conditional compiling rules to use either the old or new code. We will come back to you on this.
Btw, AOSC OS looks nice 😄

Ok, thanks @liushuyu,
I will discuss with Slavek on the best way forward. We will probably need some version check and conditional compiling rules to use either the old or new code. We will come back to you on this.

Thank you!

Btw, AOSC OS looks nice 😄

Thanks.

I tried to add CMake checking the AVFrame structure members. Now I build successfully on Jessie and Bullseye. You can try to build on your systems.

Builds fine in bookworm too.

I tried to add CMake checking the AVFrame structure members. Now I build successfully on Jessie and Bullseye. You can try to build on your systems.

Thank you. It built and worked correctly!

Thanks.

Thank you all for testing. Please, @liushuyu, you can do squash for all commits plugins/ffmpeg_decoder:… into one? Thank you.

Thank you all for testing. Please, @liushuyu, you can do squash for all commits plugins/ffmpeg_decoder:… into one? Thank you.

Gladly. And done.

Three notes:

It seems that I misunderstood the comment in FFMPEG commit and avio_context_free is available from 57.80.100, not 57.11.00 – see commit b12e4d3bb8. Please can you update it in code?
Leaving the comment plugins/ffmpeg_decoder: address SlavekB's comments in commit log seems unnecessary.
I assumed that commit 42d940767e you retain separate because it is not directly related to FFMPEG. And it seems like a good idea to make an independent part as a separate commit.

Three notes:

It seems that I misunderstood the comment in FFMPEG commit and avio_context_free is available from 57.80.100, not 57.11.00 – see commit b12e4d3bb8. Please can you update it in code?

Leaving the comment plugins/ffmpeg_decoder: address SlavekB's comments in commit log seems unnecessary.

I assumed that commit 42d940767e you retain separate because it is not directly related to FFMPEG. And it seems like a good idea to make an independent part as a separate commit.

Okay, will do.

Are there still any outstanding items preventing from this PR from being merged?

Probably a new test in Stretch, since we previously had a FTBFS. Slavek is now asleep, so it can probably be done after that.

@liushuyu please a bit more patience, contribution to TDE is always a long process, here are the reasons:

The core dev is tiny, only two people are reviewing code (Slavek and Michele)
those two run a busy life as professionals with responsabilities, their tasks in TDE is taken from their spare time, which is short.
Some PR are more demanding than others because those "fools" have been providing binaries (packages) for many Debian/Ubuntu releases; some are very old, some are new but keeping compatibility along the extremes is both challenging and time consuming. In that regard, PRs are run againts all deb distros...and FreeBSD.

As I personal note (I'm just a mere contributor), thanks for your contribution, this is nice. 👍

Thanks @cethyel, although it is ok. In fact it is good to see active contributors 😄

Dev team is small, true. But we try our best! Btw, you and @blu.256 are also important people in the team, regardless of whether you are a contributor or a core member.

@liushuyu: due to unexpected previous FTBFS on stretch, Slavek will build-test vs all distros to make sure we don't miss out on potential FTBFS.

I did a test on all currently supported Debian / Ubuntu and FreeBSD. Because of Ubuntu 14.04 I added some more #ifdef.

I did a test on all currently supported Debian / Ubuntu and FreeBSD. Because of Ubuntu 14.04 I added some more #ifdef.

@liushuyu I think what Slavek is saying is "can you test with his latest commit on your side?". Otherwise he would have merged the PR already 😄

I did a test on all currently supported Debian / Ubuntu and FreeBSD. Because of Ubuntu 14.04 I added some more #ifdef.

@liushuyu I think what Slavek is saying is "can you test with his latest commit on your side?". Otherwise he would have merged the PR already 😄

I have just tested the changes and it worked.

I am sorry but English is not my native language and I can't deduce what they implied was that I needed to report back the testing results.

I can understand TDE is consists of a tiny team and I admire what you do. But please also understand that new contributors like me will have issues like this and will need guidance. Mutual understand is the key and I am trying to understand your codeing standards, project culture and so on.

Thanks for your understanding.

I can understand TDE is consists of a tiny team and I admire what you do. But please also understand that new contributors like me will have issues like this and will need guidance. Mutual understand is the key and I am trying to understand your codeing standards, project culture and so on.

Very well said @liushuyu, a plause to you. Not all contributors come around with such open mind. It is very good to read that! We are very happy to provide help and guidance when needed 😄

I reckon this PR is now ready to be merged, but I will leave it to Slavek.
Thanks for the contribution and looking forward for more if you have any (@liushuyu)

Building tdemultimedia WITH_ARTS_AKODE=ON, and with akode patched to commit d4affcc0, I get this error:

In file included from ../akode_artsplugin/akodePlayObject_impl.cpp:43:
../akode_artsplugin/arts_inputstream.h:78:10: error: conflicting return type specified for ‘virtual bool Arts_InputStream::seek(long int, int)’
     bool seek(long to, int whence) {
          ^~~~
In file included from /opt/tde/include/akode/localfile.h:24,
                 from ../akode_artsplugin/akodePlayObject_impl.cpp:36:
/opt/tde/include/akode/file.h:79:21: note: overridden function is ‘virtual ssize_t aKode::File::seek(long int, int)’
     virtual ssize_t seek(long to, int whence = SEEK_SET) = 0;
                     ^~~~

@Ray-V, thank you, this is a very good notice.

I apologize, in Wednesday I had a long day, so I didn't write a comment. Indeed, I hesitated whether to keep defining __STDC_CONSTANT_MACROS always (as it is in patch now), or whether to make a detection whether this definition is needed. But it seems to do detection is unnecessary work, so I tend to keep it as it is. Thank you for your testing.

Now, after notice from @Ray-V, we need to assess the API change, which is proposed for the seek method. Therefore, merge PR will be waiting for a while.

I have added a commit where I am proposing to rename File::seek() to File::lseek(). This makes the name method consistent with its behavior and most importantly, will help catching any possible subtle error that resulted from the change of return type proposed in this PR.

@SlavekB what do you think? If you think it is not necessary, we can simply discard the commit. In the meanime I am going to prepare/adjust the other parts in the code where this is used.

@SlavekB what do you think? If you think it is not necessary, we can simply discard the commit. In the meanime I am going to prepare/adjust the other parts in the code where this is used.

Yes, this change forces more changes elsewhere, but it make sure we don't miss out on any use of it that may require adjustment.

Building tdemultimedia WITH_ARTS_AKODE=ON, and with akode patched to commit d4affcc0, I get this error:

@Ray-V: TDE/tdemultimedia#28 is a proposed fix for the build failure.

@liushuyu could you test one more time with the latest commit added to this PR? Most likely this will be the final form of this PR, unless some other unexpected issue shows up.

Although change seek => lseek for File class seems extensive, it helps in the code clarity. And this is a substantial benefit. At the same time, it seems that the File class API may be considered internal, so it should not be a problem to merge it and backport to stable branch.

Thank you for the good work to everyone involved.

It looks good. If there are no new problems during tests, it seems ready to merge.

It looks good. If there are no new problems during tests, it seems ready to merge.

Builds fine here but let's wait till @Ray-V tests as well.

Thank you for the good work to everyone involved.

Yes, it was good to see almost all the dev team involved in this PR 😄

Builds fine here but let's wait till @Ray-V tests as well.

All OK here.

akode to commit 30b73db1
tdemultimedia to commit bb98c0c2

Works for 14.0.x as well.

Thanks for testing Ray, all good then!

Merged and backported to R14.0.x.

Thanks to @liushuyu for the initial work and thanks to all the people involved in reviewing/testing.

     d->videoStream = -1;
     for (int i = 0; i < d->ic->nb_streams; i++) {
         if (d->ic->streams[i]->codec->codec_type == CODEC_TYPE_AUDIO)
         if (d->ic->streams[i]->codecpar->codec_type == AVMEDIA_TYPE_AUDIO)

     if ( d->stream ) {
         if (d->stream->buffer)
             av_free(d->stream->buffer);
         avio_context_free(&d->stream);

     int len = avcodec_decode_audio4( d->ic->streams[d->audioStream]->codec,
                                     decodeFrame, &decoded,
                                     &d->packet );
     d->packetSize = decodeFrame->pkt_size;

   DESTINATION ${LIB_INSTALL_DIR}
 )
 target_compile_definitions(${target}-module PRIVATE _FILE_OFFSET_BITS=64)

Revive FFmpeg Plugin #7

Reviewers