Replace libav with FFmpeg 3.4.2 *do not merge* #1218
Conversation
|
there's a particular reason in this pr? just curious, I prefer using ffmpeg too |
|
No one reason currently, I just use the standalone ffmpeg for postprocessing. |
|
@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. |
|
I think it's never a good idea to start the transconding in a fine that it's being written. |
|
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. 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? |
|
nono absolutely not, I really like FFmpeg. Probably I badly explained myself. |
|
@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! |
|
@lodoyun do you know why some deprecation/warnings on OSX becomes deprecation/errors on Ubuntu? There is some setting to handle warnings as errors? |
Replicate Ubuntu behavior where Erizo is built with -Wall -Werror by default, not sure if it is inherited from another project or by differences between Ubuntu 14.04 CMake (2.8.x) and OSX Brew (3.x.x)
|
@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. |
|
@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. |
|
@jcague Could you clarify to me how/where are InputProcessor/OutputProcessor in MediaProcessor are being used? specifically |
|
@jcague please correct me if I am wrong.
|
|
@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 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. |
|
@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? |
|
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? |
|
@lodoyun I was thinking checklist QA from devs before PR, but that you've pointed is totally helpful. |
* 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.
|
@lodoyun @jcague Can you briefly take a look to see how this is going?
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 |
a86c50e to
62ba1e0
Compare
* Function syncClose returns if not recording. * Properly check if context_ isn't null before free it. * Do not exit if context couldn't be initialized. * Remove not used save jpeg function from Coder class. * Add CoderCodec isInitialized getter. * Add CoderCodec width/height accessors. * Add CoderCodec abitlity to initialize AVStream. * Rename InitContextCB to InitContextBeforeOpenCB. * Fix Coder::initContext return variable was overrided. * InitEncoder / InitDecord now returns bool. * Protect AVCodec in CoderCodec. * Check ffmpeg is installed before download and compile.
|
@lodoyun I've addressed most of your concerns except for the one I've commented about. |
| @@ -682,14 +682,22 @@ const Room = (altIo, altConnectionHelpers, altConnectionManager, specInput) => { | |||
| }; | |||
|
|
|||
| // Returns callback(id, error) | |||
| that.startRecording = (stream, callback = () => {}) => { | |||
| that.startRecording = (stream, callbackOrOptions) => { | |||
There was a problem hiding this comment.
Even if it's an API just to test things, I'd rather have
that.startRecording = (stream, callback, advancedOptions=undefined) => {
We can decide later at what point and in what format we can expose that options in the API and update the docs accordingly.
There was a problem hiding this comment.
@lodoyun yes, totally, I've even forgot you can add arbitrary params to a js fn, I'll update as soon as I can.
|
@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. |
|
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 |
|
@lodoyun any update on this PR? did you guys have merged/implemented parts of this somewhere? |
|
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. |
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.
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.