Implementation of high-level FFmpeg API

[andcscott]

For reference:

• Issue Improvement: Add FFmpegController Abstraction Layer #4
• Issue Feature: Implement FFmpegCommandBuilder to MediaProcessor #31
• Issue Feature: Add FFmpegController to core #60
• PR Added FFmpegController class #28
• PR Add FFmpegControllerBuilder class #41
• PR Add FFmpegSettingsManager to core #44
• PR Add FFmpegCommandBuilder class #51

Proposed classes:

• FFmpegConfigManager: This is the global configuration manager for paths and thread settings, acting as a single source of truth.
• FFmpegCommandBuilder: This guy builds individual FFmpeg commands, mainly an abstraction from the low-level string manipulations. Particularly useful for cross-platform compatability
• FFmpegControllerBuilder: This builds the controller with configurations from the FFmpegConfigManager and commands from FFmpegCommandBuilder.
• FFmpegController: This is our public API that orchestrates operations like extractAudio(), etc. This is what the user interacts with, while everything else happens under the hood.
• FFmpegSettingsManager: A singleton that Manages global, audio and video settings specific to ffmpeg. It does not manage configurations -that is done via the ConfigManager. It has some limited internal utility to smooth out implementations.

I wanted to start this discussion so we/I can stay on the same page for these improvements. It has expanded quite a bit beyond the initial issues, and your suggestion to silo our conversation seemed reasonable. It seems like FFmpegConfigManager and FFmpegCommandBuilder need to be handled before I can fix up FFmpegControllerBuilder, so my primary question at this point is how do you want me to proceed?

[omeryusufyagci]

Thanks for this @andcscott !

Indeed, we've added a few classes along the way to better share and define the closely related but distinct functionality. In the end, I believe this will provide a stable and extensible interface for us!

As you may have seen, I've already made a first implementation of the FFmpegSettingsManager. I plan to take a second look at it tonight, before merging. I may have missed a few settings, haven't really doubled checked yet, but it's easily extensible.

There is no longer an FFmpegConfigManager to do, this idea was replaced with the FFmpegSettingsManager I described above. For configs, we remain to use the ConfigManager.

Then, we need 2 classes that use directly the FFmpegSettingsManager:

1. FFmpegCommandBuilder: simply put, string ops abstraction layer to build OS calls.
2. FFmpegControllerBuilder: means to construct an FFmpegController with user settings

I'd propose for you to implement first "1", then "2", due to the dependency nature in between the two.
Once these are all in place, we can discuss again the final implementation of the FFmpegController.

Does this sound good to you? I'm happy to go over any aspect again, just let me know!

[andcscott]

Hi @omeryusufyagci thanks for your reply!

Sounds good so far! Thank you for providing a little more direction, I think it's pretty obvious I got myself mixed up :) I took a better look at FFmpegSettingsManager too. It's funny, I almost added enums to my last PR for common options but I second guessed myself! I'll get started on the first, I agree that makes sense. I'm happy to iterate through but if you think of any requirements beyond what we discussed feel free to let me know.

I apologize that I've needed a little hand-holding through this, however I'm learning a lot about the project and finally putting all that fiddling with ffmpeg to good use. It's been a great learning opportunity and I appreciate your guidance, thanks again.

[omeryusufyagci]

Hey @andcscott, and please, no worries at all! I'm glad you find the project interesting and value your contributions. If anything, it's on me that I didn't provide a clear roadmap from the beginning.

I don't have anything to add on the FFmpegCommandBuilder, aside from what we discussed previously. Thanks for staying the course and good luck ! :)

[andcscott]

Hi @omeryusufyagci

I was working on FFmpegCommandBuilder today and I had a couple questions.

I'll start with the easier one: should I be using FFmpegSettingsManager::enumToString to get the String values for the codecs? The value maps are private in FFmpegSettingsManager so I'd probably need to 1) add getters to settings manager, 2) declare the builder a friend, or 3) make the maps public.

Second, I wanted to share some code before opening a PR. Hopefully I'm on the right track this time but we'll see ;)

Example header:

public:
  FFmpegCommandBuilder(FFmpegSettingsManager& ffmpegSettings);

  FFmpegCommandBuilder& addOverwrite();

  FFmpegCommandBuilder& addInputFile();

private:
  FFmpegSettingsManager m_ffmpegSettings;

Example implementation:

FFmpegCommandBuilder& FFmpegCommandBuilder::addOverwrite() {
    m_ffmpegSettings.getOverwrite() ? addFlag("-y") : addFlag("-n");

    return *this;
}

FFmpegCommandBuilder& FFmpegCommandBuilder::addInputFile() {
    addFlag("-i", m_ffmpegSettings.getInputFile());

    return *this;
}

How does that look to you? Edit: I also had the idea of either overloading the "add" methods or maybe making the parameters optional, with the values from FFmpegSettings as default, in case the user wants to provide a custom value. I'm not sure if that's desirable though.

[omeryusufyagci]

Hi @andcscott,

• We should move the enumToString to Utils namespace -thanks for bringing it up! You can use the same method in your implementation. Feel free to update this in your PR as well.
• Regarding the settings maps and getters, the settings manager already has getters. We can indeed use the getters, and keep the structs private.
• About returning this: I think that's really good to have! Could you also update the base class in your PR to allow chaining?
• The builder itself shouldn't ideally have decision making logic. So, even the ternary shouldn't be there. If we don't pass addOverwrite it's disabled and using an addOverwrite with a no flag seems unnecessary. This may apply to other methods as well.
• The specific builders like this one should have sensible defaults. So that's definitely something desired.

Does this help? Thanks!

[andcscott]

Hi @omeryusufyagci, yes that's great, thank you! Hoping to have a PR ready today, the only concern I have at the moment is accessing the unordered maps in Settings Manager. There are getters for the codec, but unless I'm missing something the value maps that I'd pass to enumToString are inaccessible?

[andcscott]

Sorry you can disregard that question, I think clangd was just giving me a hard time - sent a PR for review

[andcscott]

Hey there @omeryusufyagci, hope you're not tired of hearing from me yet ;)

I put some time into FFmpegController for #60 today and I have a couple questions.

First - I've been adding methods to FFmpegCommandBuilder where it seems appropriate. Mostly for mergeChunks and mergeMedia but, I want to double check if that's OK with you. So far I've added these methods:

• addFFmpegPath()
• addStartTime(const std::ostringstream& startTime) (user would pass a double to FFmpegController)
• addDuration(const std::ostringstream& duration) (user would pass a double to FFmpegController)
• addChunkPath(const fs::path& chunkPath)
• addInputFile(const fs::path& inputFilePath) (overload for mergeMedia() which has 2 inputs)

I'm wondering if those parameters might actually be something that FFmpegConfigManager should handle so the user doesn't have to keep track of which class accepts configs, especially since the builder is now solely focused on building and no I/O?

Second - Should the controller have logic for adding (or not adding) the overwrite flag? I recall that you didn't want the command builder making decisions, so I want to check here too. Since the position of the arguments is important a conditional could disrupt the flow of method chaining, but there's really no way around FFmpeg's positional arguments.

Third - Did you want me to take care of updateSettings() for the config manager?

Ok... I think that last edit is everything, it's been a long day and I may think of something else later haha. Anyway, thanks for your help!

[omeryusufyagci]

Hey there @omeryusufyagci, hope you're not tired of hearing from me yet ;)

Hi @andcscott, no..not anytime soon ! :))

I'll try to tackle these in the same order:

First:

I've been adding methods to FFmpegCommandBuilder where it seems appropriate.

Adding methods to FFmpegCommandBuilder is perfectly fine.

So far I've added these methods:

• addFFmpegPath()

This might be a good one for the ctor to immediately take care of. Since any FFmpeg op will need this and it needs to be the first argument, you can immediately do an addArgument() on the ctor to eliminate this build step.

I'm wondering if those parameters might actually be something that FFmpegConfigManager should handle so the user doesn't have to keep track of which class accepts configs, especially since the builder is now solely focused on building and no I/O?

Yes the user shouldn't track these details. The user will set the their settings and use the controller to perform ffmpeg operations.
This builder is more targeted for the developers: it's mainly to have a streamlined way of adding high level functions to the controller. It also comes in handy for direct OS-calling like during tests.

Also, maintaining the controller should ideally not require to know which flag to pass for an operation. That's why we're adding the respective build command for each, so that we can in the future add more operations like extractAudio() easily -which essentially is a bunch of parameters/flags set in a certain order.

The parameter values should indeed come from the settings manager. Those values will be used to construct these build steps. You could update it in your PR as its closely related, or provide the params you need and I can take care of it. Let me know please

Second:

Should the controller have logic for adding (or not adding) the overwrite flag? I recall that you didn't want the command builder making decisions, so I want to check here too. Since the position of the arguments is important a conditional could disrupt the flow of method chaining, but there's really no way around FFmpeg's positional arguments.

I'd like to keep the focus of the controller on orchestration. Anything that's a setting or config should come from the FFmpegConfigManager. Although, I'm not very keen on the fact that configs and settings are managed together right now, it was a compromise to unblock these a bit...

Positional arguments really are a pain; same for the DeepFilter as well. If we can find clever solutions avoid them it'd be great, but in any case, we construct the build stages for the high level API so we could survive like this for the time being. The user should use the controller, and if they want to hack around with the builder directly, it's acceptable that it has its quirks; that's why we're providing a stable API.

Third:

Did you want me to take care of updateSettings() for the config manager?

Would be great, of course, but not a hard requirement, especially if you're time constrained for other activies, etc. I'll leave that up to you and I'm happy to implement it later if you don't have the bandwidth for it at this time.

Fourth:
This one's an extra from me! : )) Since this API will be quite central for the near future at least, it'd be great if we could merge it with tests. Thanks to the great work from @nikhiljangra264, we have a nice tests structure, and working tests for the processors.
Could you also add a test suite for the public API?

I'm more than happy to help you with anything above -although it might not come before the weekend.
Please also don't hesitate to provide feedback on anything, and let me know about how you'd like to proceed. Thanks as always!

[omeryusufyagci]

Thanks a lot for the great work on this @andcscott!

In the end, this initial implementation consisted of:

• An FFmpegConfigManager that takes care of managing FFmpeg-specific configuration fields, ensuring their validity (still a todo because of me), and their update functionality.
• An FFmpegCommandBuilder that abstracts the rather tedious syntax of FFmpeg to allow building specific FFmpeg commands in plain English.
• An FFmpegController that is an orchestrator for our custom FFmpeg operations. The controller is instantiated with a const FFmpegConfigManager& and acts as our high level interface for FFmpeg operations, such as extractAudio().

Overall, this serves as a solid base that should allow us to easily make changes if and when we might need to. Special thanks to your perseverance and openness as the design went through a few iterations!

[andcscott]

Hey @omeryusufyagci, thank you so much! I really appreciate that! I'm definitely glad I stuck around to see this stage finished. Your guidance was also extremely helpful, and if nothing else my work is a reflection of that. Fast Music Remover is one of the first FOSS projecs I've contributed to since I graduated, and I'm so glad I chose it for Hacktoberfest. I'll definitely hang out in the discord and check the issues page from time to time to see if there's anything else I might be able to help with!