I see that FFmpeg Converter is using its own ffmpeg_converter_convert() instead of ffmpeg_wrapper_convert_file() which seems to be more appropriate. I understand ffmpeg wrapper as a module which will do all the ffmpeg stuff.

If this has just historical reasons, I recommend to use ffmpeg_wrapper_convert_file(). May we migrate to use this function? At first glance it seems that there are some differences, but maybe we could implement them into the ffmpeg wrapper function and if we need we just build a hook_ffmpeg_wrapper_convert_file_alter() into ffmpeg_wrapper_convert_file(). I can provide patches.

Otherwise we should at least add a comment why we don't use ffmpeg_wrapper_convert_file().

Comments

zoo33’s picture

zoo33 commented

Good point. Yes, there are reasons for why we're using our own function, but right now I don't remember exactly what they were. I have discussed this earlier with Arthur who's the maintainer of FFmpeg Wrapper. At that time we came to the conclusion that it would be best to keep the functions separate, but that may no longer be the case. A new hook might indeed be a good solution if it is. I'm in favor of anything that reduces the amount of code, as long as we don't break anything.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented
Title: Why not use ffmpeg_wrapper_convert_file. » Use ffmpeg_wrapper_convert_file instead of ffmpeg_converter_convert

I don't feel comfortable with, that ffmpeg_converter_convert does stuff that should be done by ffmpeg_wrapper. IMO ffmpeg converter should just describe the parameters for the transformation (like codecs, bitrates, size, input and output files) and ffmpeg wrapper will transform this description in a ffmpeg command and execute it and finally ffmpeg converter will do the necessary post processing. This might be achieved with the help of #1236964: Make the module more extentable by providing alter hooks.

But maybe I have just some misunderstandings of the ffmpeg_wrapper concept. As I see, you already concerned to use ffmpeg_wrapper_convert_file and it didn't work. But maybe things changed in the version 2.x. Hope you will remember.

zoo33’s picture

zoo33 commented

No, I think you understand the concept perfectly. FFmpeg Wrapper is supposed to do the heavy lifting, preferably. My only concern is that switching to ffmpeg_wrapper_convert() may change the behavior of the module for existing users. Running some tests with this setup would be nice, and I haven't had the time to do that yet. Maybe the easiest way to have people test it would be to commit it to -dev and wait for potential bug reports.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented

I guess it won't work just use ffmpeg_wrapper_convert_file() otherwise I would have implemented it already. I also have little time. On my side it needs to be postponed until the next payed job on video transformation ... I'm still open for offers ;)