Hi @andcscott, thanks for this good start!

After thinking about this more, I think the best solution is to add an FFmpegControllerBuilder (I know, another builder!) that would allow for something like:

FFmpegController controller;
controller.setFFmpegPath("/path/to/ffmpeg")
         .setAudioCodec("aac")
         // ...

This would offload a lot of work from this class and help with maintenance down the road, especially if we need to do frequent changes.

The main point missing in the current PR is that the controller isn’t orchestrating the actions as expected. The user shoould interface with FFmpegController and not directly with the FFmpegCommandBuilder. So, the intended usage would look something like:

// set the settings on controller and then
controller.extractAudio(inputVideoPath, outputAudioPath);

Then you'd have a method FFmpegController::extractAudio() that would invoke the builder with the class members.

I was also kind of thinking that the user may pass 3 structs in the ctor, and we make the structs public, but we can come back this again, as it ties with the point above.

Otherwise thanks a lot for the good start, and please let me know if anything is unclear and if you have any feedback!

After thinking about this more, I think the best solution is to add an FFmpegControllerBuilder

Sure, it makes sense to make it modular from the start. Is this builder the same as #31 or are you thinking of a new class altogether?

The main point missing in the current PR is that the controller isn’t orchestrating the actions as expected. The user shoould interface with FFmpegController and not directly with the FFmpegCommandBuilder.

Sorry, I guess I had the backwards. I had thought the user would interface with FFmpegCommandBuilder which would in turn call FFmpegController but I see the direction you're moving now.

I was also kind of thinking that the user may pass 3 structs in the ctor, and we make the structs public, but we can come back this again, as it ties with the point above.

The declaration of the structs is in the MediaProcessor namespace so you should be able to create them from anywhere in the project, it's only the instances held by a FFmpegController that are private. So in this case it's still possible to pass the structs to the constructor, however are you saying you might prefer that to be the default? Another option would be to make the structs protected rather than private, and make the builder a friend. In either case it wouldn't be a difficult change, just thinking out loud and looking to clarify - although I understand if you want to think on it.

Hi @andcscott

Yes, this would be a new builder specifically for constructing the FFmpegController more cleanly. The idea is that the user interfaces with FFmpegController directly for actions e.g. extractAudio(), while the builder handles the configuration. This should separate well the config of our ffmpeg controller with the user interface, which is the FFmpegController class.

Let's revisit the rest after you introduce this change, which should make it easier to assess how we want to move forward. Thanks a lot!

Heya @omeryusufyagci , so I was working through this today and I'm a little paranoid that it won't adhere to the goals. Well paranoid is maybe too strong, but anyway here's what I have planned so far:

1. Split the existing FFmpegController from this PR into FFmpegCommandBuilder, adding or removing methods where relevant and open a PR for the issue specifc branch.
2. Implement a new FFmpegController with methods to offload the work being done by VideoProcessor and AudioProcessor, and open a new PR (on the issue branch if desired). For example:

bool extractAudio(const fs::path &inputVideoPath, const fs::path &outputAudioPath);

bool chunkAudio(const double startTime, const double duration, const fs::path &outputAudioPath,
                    const fs::path &chunkPath);

bool filterChunks(const fs::path &deepFilterPath, const fs::path &processedChunksDir,
                      const fs::path &chunkPath);

bool mergeChunks();

double getAudioDuration(const fs::path &audioPath);

3. User will interface with FFmpegController, which in turn calls FFmpegCommandBuilder

If that all looks OK I can proceed, however I'm still not sure about FFmpegControllerBuilder. If that class manages the configuration for the controller then the user will also have to interface with it to 'set' options that FFmpegController later 'gets.' To me that suggests the FFmpegControllerBuilder class should hold the structs I implemented?

Sorry for the brain dump, I realize that's a lot of info so no hurry getting back to me. I mainly want to be sure I'm going in the right direction. Thanks!

Just FYI @omeryusufyagci I didn't mean to close this, synced my fork without thinking yesterday, sorry for any confusion... future PRs will be on a dev branch

Hi @andcscott, thanks for the update and no worries on the PR!

You're right, the user will deal with FFmpegControllerBuilder to set options like audioCodec, ffmpegPath, etc.
The idea is that the builder handles the configuration, and once that’s done, the user will work with FFmpegController for actions like extractAudio().

So yes, the builder is for setup, and the controller is for executing commands. This decouples the setup of ffmpeg from using it.

Just a remark on the filterChunks() you put in the snippet: since this is filter-specific, this will eventually go under the FilterController class - that'll be the same arch for filters, which reminds me an interface for controllers would be needed!

Using the issue branch would be ideal indeed.

Does this help? I'm happy to go over it again.

Hi @omeryusufyagci yes that helps, thank you. The separate class for FilterController will also make this a little cleaner I think. I appreciate you taking the time to reiterate all of that. I should be OK now but I'll let you know if anything else comes up. Thanks!