Replace libav with FFmpeg 3.4.2 *do not merge*

[zevarito]

This is a POC about switching to FFmpeg due to the reasons that @jnoring exposed in the past #232 , plus a couple more that I've tested.

• Experimenting with other kind of Outputs I was able to accomplish tasks with FFmpeg that didn't work with the same code compiling against LibAV (even using edge LibAV).
• When you look for LibAV documentation, examples, discussions, you always end on FFmpeg related docs, forums etc.
• Ubuntu (mayor supporter before) dropped support of LibAV in favor of FFmpeg.
• LibAV 12.3 (latest, Licode uses 11.6) is still behind FFmpeg 3.2.4 which is updated here, while FFmpeg 4.0 was recently released. Please take a look at what changed in each version to see that are not trivial changes/fixes. https://www.ffmpeg.org.

I would like to know if you guys are up to make this change, if so, I will commit myself to work on remove deprecation warnings an rewrite decode/encode parts to following the latest encoding pipeline[0].

[0] https://ffmpeg.org/doxygen/3.3/group__lavc__encdec.html

[NO] It needs and includes Unit Tests

[NO] It includes documentation for these changes in /doc.

[kekkokk]

there's a particular reason in this pr? just curious, I prefer using ffmpeg too

[zevarito]

@kekkokk #232 and the bullet list, what other kind of reasons do you have in mind?

[kekkokk]

No one reason currently, I just use the standalone ffmpeg for postprocessing. I think that actually libav gets it's job done well...

[zevarito]

@kekkokk well, in the case of post-processing if you are doing some of that simultaneously on the same server, you can take some advantage of having ffmpeg installed instead libav, since it works better, and you might avoid to spawn ffmpeg process, containers, etc. to pipe what is inside Licode writing specific code for that instead. Also I've had bad experience processing MKV with ffmpeg command line while it is being written, that lead me to replace libav and do some stuff from code.

However there are implications about the design and scope of the project itself, I believe Licode want's to stay as SFU and lightweight as possible, but at the same time it is wrote in a way that is fairly easy extensible and modular, so would be great to have certain dependencies updated so anyone could add component features for different purposes.

You are right about it does his job well, so far.

[kekkokk]

I think it's never a good idea to start the transconding in a fine that it's being written. I use separate machines to do the transcoding and the mosaic. Still having everything updated it's good as you said.

[lodoyun]

We originally used libav because it was the main option in Ubuntu at the time as the original idea was to use the apt package version. We no longer have any reason to use libav instead of ffmpeg though. From what I've read ffmpeg is usually in a better state but, frankly, I'm not keeping up with the development of either of them.

In general, I would say I'd rather switch to ffmpeg instead of updating libav when we need something from a newer version so this PR looks good in that sense. @kekkokk is there anything in particular you don't like about ffmpeg?

[kekkokk]

nono absolutely not, I really like FFmpeg. Probably I badly explained myself.

[zevarito]

@lodoyun that's cool, I've closed this PR since it was working fine on OSX but wasn't working on Ubuntu, I'll send a new PR as soon as I can. Thanks!

[zevarito]

@lodoyun do you know why some deprecation/warnings on OSX becomes deprecation/errors on Ubuntu? There is some setting to handle warnings as errors?

[zevarito]

@lodoyun I've started to update API calls to eliminate all warnings, it looks like a big change touching several files on the project, I'll try to commit as small parts as I can so it gets easy for you to review and give me some feedback, there is a strange behavior that actually forces me to remove all deprecation warnings.

The thing is that when the project is compiled in OSX, it seems to have -Wall flag, but when the same codebase is compiled in Ubuntu it seems to also get -Werror for Erizo which leads to the compile errors noticed in CI.

I believe it could be due a mismatch version of CMake between OSX (3.x.x) and Ubuntu (2.x.x) or by some build configuration of a dependency of the project that introduces that flag, still not clear since I've tried to override it in Ubuntu without success.

[jcague]

@zevarito yes, we're aware of such mismatch in the compiler flags between OSX and Ubuntu. Actually, the main reason is that OSX uses clang while in Linux we use gcc, so even having -Werror (which I think would be a good idea) does not ensure you will not have warnings in Linux. Anyway, I also think it sometimes makes it hard to submit PRs to the repo when we're using OSX for development, we will consider alternatives (like using clang in linux, or maybe providing an easy way to develop using the docker container.

[zevarito]

@jcague Could you clarify to me how/where are InputProcessor/OutputProcessor in MediaProcessor are being used? specifically decodeAudio and encodeAudio functions and same ones in AudioCodec, I am looking to cleanup code instead of migrate the one that is not being used. Thanks!

[zevarito]

@jcague please correct me if I am wrong.

• MediaProcessor and AudioCodec both had dup logic for encode/decode audio.
• Looks like encodeAudio not being used anywhere yet.
• Could be the intention of AudioCodec be the layer that connects to ffmpeg and leave MediaProcessor as a higher level to handle rtc stuff? If so, MediaProcessor should be using AudioCodec API itself, as External/InternalOutput should do as-well?

[lodoyun]

@zevarito Yes, that's about it. The AudioEncoder/AudioDecoder should have been the audio equivalent of VideoEncoder/VideoDecoder and abstract libav(ffmpeg) at that level to be used in MediaProcessor. I really can't tell why that is not the case. It does not make sense to have that code duplicated and if you could use it in MediaProcessor it would be great.

MediaProcessor should be the abstraction to use anywhere encoding/decoding is needed. ExternalOutput is a grey area, since we are not really transcoding and only used it to write to a specific container.

[zevarito]

@lodoyun That's cool, I will look upon that and put everything there.

About ExternalOuput I have a wip that might be interesting to merge once FFMpeg is fully working, it allows to output to diferent containers, protocols and codecs.

I didn't look deeply into the test suite but I believe lot of things are mocked so I was willing to have a checklist of manual test-cases that ensure all pieces of code are touched, like a guideline for developers to test that everything works once low level parts of the code are changed. Do you guys have something like that right now?

[lodoyun]

The ExternalOutput wip sounds interesting, we will probably want to keep using a combination that works by default and possibly add options in the configuration as we test more.

The tests for erizo are almost exclusively unit tests. For instance for LibNice or Nicer we will test that the interface that we use to call the library is the right one, that we are calling the right things at the right time, etc. They are not testing the library itself but it's generally a good indicator. We haven't covered ExternalOutput, for instance, which would be helpful.

We do have full integration tests here that are run daily but they don't cover recording and are focused on testing different browsers.

Both need to cover recording to be helpful in this case but I think that would cover what you are asking, right?

[zevarito]

@lodoyun I was thinking checklist QA from devs before PR, but that you've pointed is totally helpful.

[zevarito]

@lodoyun @jcague Can you briefly take a look to see how this is going?

6d21295

• Add sample format and channels to MediaInfo struct.
• Move MediaInfo, RTPInfo, ProcessorType structs into a single file.
• Abstract audio/video enc/dec from Input/OutputProcessor.
• Abstract basic enc/dec FFmpeg API into Coder class.
• Rename variables to follow cpp code guideline.

Coder is a class that allocate and open contexts, encode and decode in a general way (AVFrame<->AVPacket) as the latest FFmpeg API. It is being used by VideoEncoder/Decoder and AudioEncoder/Decoder as an aggregate class for now. Those classes provide functions to encode and decode from buffers as example.

It's worth to notice that Encoding operation result is made through a Callback, since the encoder might output multiple packets after receive a single frame. Alternatively to this approach I think a FIFO can be used so that once you send a Frame to encoding you drain the FIFO of result packets.

Are you running test project standalone or replicating CircleCI locally?

[zevarito]

@lodoyun I've addressed most of your concerns except for the one I've commented about. I've also committed a change to erizoController/Client startRecording function to allow to pass a different extension or full url with the ability of templating recordingId, this change is backward compatible but I had not documented this since I don't think is a final API, it just a thing to easy test EO.

[peererror]

@zevarito what is the status of this pull is it going to merge in main repo

[zevarito]

@zevarito what is the status of this pull is it going to merge in main repo

I think it was ready... 10 months ago :D, right now I don't think so.

[lodoyun]

I've updated and tested this in this branch. Unfortunately, there are some issues I think related to the rescaling of the timestamps when not transcoding that prevent me from merging it, the result is that no audio codec works out of the box, all need postprocessing. I have other more pressing things to do atm, but I'll get back to this in the near future

[zevarito]

Hey @lodoyun looking around to see in what this pr was, after read your last comment I realize that this commit might be related: f6494e1195c8830987dfdb658034151cdf0d4f48.

Also, of 32 commits from the PR I think these are the most relevant: 6d2129523f75fe8ceb66a067d61d8b759cd46690 7700db04319ff74ef4861599189e24d736b96888 595c9003cc17798cc45111abd3d935606671a149

[zevarito]

@lodoyun any update on this PR? did you guys have merged/implemented parts of this somewhere?

[lodoyun]

Sadly it's not ready. I have a branch locally that is a lot closer to the current head but I run into some problems with the audio codec, which always needed postprocessing (whereas now, we don't always need it) and it fell through the priority list. I still want to merge it so thank you for the ping. I'll see if I can allocate some time for this soon.