Custom ffmpeg dialog #9249
Custom ffmpeg dialog #9249
Conversation
3991b0c to
0f9f891
Compare
There was a problem hiding this comment.
A mixture of more or less important comments. (I haven't reviewed the QML part, which @kryksyh is reviewing, so please wait for both approvals before proceeding.)
The thing that we have to clarify is about this modularity. Au3's FFMPEG is a module, meaning we can build Au3 without it, and the app will otherwise function normally.
Here it is not the case: I verified that, setting AU_USE_FFMPEG to OFF yields successful compilation but the export dialog cannot be loaded anymore.
Maybe we don't need modularity for FFMPEG, in which case this PR could be simplified. If we do, however, I recommend that we put it in place now: postponing it can only make it more work later.
|
|
||
| bool ExportPreferencesModel::customFFmpegOptionsVisible() | ||
| { | ||
| return currentFormat() == "Custom FFmpeg Export"; |
There was a problem hiding this comment.
Better would be to iterate through ExportFFmpegOptions::fmts to find the one with fmtid == FFmpegExposedFormat::FMT_OTHER.
There was a problem hiding this comment.
well... it would but there's the catch: exportpreferencesmodel doesn't know a thing about AU3 code (and we don't want it to) so we cannot just include ExportFFmpegOptions and iterate throught fmts as it's only available within the ExportFFmpegOptions.cpp/h internally.
There was a problem hiding this comment.
Also, in AU3, this is a translatable string
There was a problem hiding this comment.
I see.
Could IExporter have a isCustomFfmpegExportFormat(const std::string& format) const method? Au3Exporter would then do the dirty job.
src/importexport/export/internal/au3/au3ffmpegoptionsaccessor.cpp
Outdated
Show resolved
Hide resolved
|
As discussed in our daily: we do not care that FFMPEG support can be removed from the Audacity 4 binary (the important thing for FFMPEG being that the absence of FFMPEG dynamic libraries should not get Audacity to misbehave). Hence @grliszas14 the #ifdefs can be removed. In the unlikely event that we need this kind of modularity, we'll have to do it properly then. |
0f9f891 to
c389f97
Compare
|
|
||
| bool ExportPreferencesModel::customFFmpegOptionsVisible() | ||
| { | ||
| return currentFormat() == "Custom FFmpeg Export"; |
There was a problem hiding this comment.
Also, in AU3, this is a translatable string
src/importexport/export/qml/Export/internal/FLACOptionsSection.qml
Outdated
Show resolved
Hide resolved
c389f97 to
7fd0f7d
Compare
| CustomFFmpegPreferencesModel::CustomFFmpegPreferencesModel(QObject* parent) | ||
| : QObject(parent) | ||
| { | ||
| init(); |
There was a problem hiding this comment.
Just noticing this now, sorry: @Eism once warned me that init method (with all the onNotify and onReceived subscriptions) should be called from Component.onCompleted, that these ctor inits caused problems in the past in MSS, can't remember which.
It's also called from the ctor in exportpreferencesmodel.cpp (but it was already like that).
If you want to keep it in the ctor, since I can't remember why it's bad ( :D ) I'll be okay with it, but then the init method doesn't need to be Q_INVOKABLE anymore and could be private.
There was a problem hiding this comment.
yep, fixed for both models
7fd0f7d to
c80bc37
Compare
Resolves: #9174
Recommended: