Attached are two patch files for the files:

ffmpeg_wrapper_ui/ffmpeg_wrapper_ui.module
ffmpeg_wrapper.module

These patch files will add integration with the FFmpeg Converter module. Features:

- Adds a "Default preset for converting files" option on the admin page that allows the user to select a default preset from FFmpeg Converter module to use when converting files
- Adds new options "Use Preset?" and a preset dropdown on the "Convert File" page that gives the user the option to convert files using the default (or other) preset.

CommentFileSizeAuthor
#21 ffmpeg_wrapper.1202902.20111002.patch4.51 KBiva2k
#19 ffmpeg_wrapper.#1202902.20111002.patch4.51 KBiva2k
#16 ffmpeg_wrapper-default-preset-1202902-16.patch1.53 KBosopolar
#11 ffmpeg_wrapper_ui.module.patch2.27 KBNathan Goulding
#11 ffmpeg_wrapper.module.patch1.07 KBNathan Goulding
#4 ffmpeg_wrapper_ui.module.patch3.75 KBNathan Goulding
#1 ffmpeg_wrapper_ui.module.patch3.42 KBNathan Goulding
ffmpeg_wrapper.module.patch1.35 KBNathan Goulding
ffmpeg_wrapper_ui.module.patch2.5 KBNathan Goulding

Comments

Nathan Goulding’s picture

Nathan Goulding commented
StatusFileSize
new ffmpeg_wrapper_ui.module.patch3.42 KB

This patch file is better for ffmpeg_wrapper_ui.module as it should only display on the convert page.

arthurf’s picture

arthurf commented

Can you just do this with form_alter instead of this direct integration?

Nathan Goulding’s picture

Nathan Goulding commented

This might be possible, I'll check it out and submit a patch to ffmpeg converter if possible

Nathan Goulding’s picture

Nathan Goulding commented
StatusFileSize
new ffmpeg_wrapper_ui.module.patch3.75 KB

Unfortunately this won't work because there actually has to be a logic to determine whether to display the presets or not. One slightly changed patch attached.

arthurf’s picture

arthurf commented

I'm pretty sure that you can accomplish this by using form alter- you just need to check the form values in your form alter.

My basic concern here is that I don't want to maintain code for another module inside of this one. I'm open to having some kinds of checks but I'd rather do this on an API level so that other modules can do the same kinds of things. Basically I'd rather fix why this doesn't work (if it doesn't) with form alter than include module specific code.

Nathan Goulding’s picture

Nathan Goulding commented

I was able to accomplish 90% of what I needed using form_alter, but unfortunately when actually doing the transcoding, by the time hook_form_alter hits my module, the conversion has already taken place. Example:

code>

function mymodule_form_alter(&$form, &$form_state, $form_id) {
  switch ($form_id) {
    case 'ffmpeg_wrapper_ui_node_convert_form': {
        echo 'does this happen too late?';
        exit;
    }
  }
}

In ffmpeg_wrapper_ui_node_action_form():

<?php
    echo 'converting now. ';
    $ffmpeg_object = ffmpeg_wrapper_convert_file($form_state['values']);
?>

The result:

An error occurred. <br />/ffmpeg_wrapper/js<br />converting now. does this happen too late?

So there's nothing I can do to alter the form before ffmpeg_wrapper converts the file. I tried setting my module's weight to be lower than ffmpeg_wrapper and it still doesn't get executed before. So I can't do this using form_alter unfortunately.

I know that this isn't the best-case scenario, but it's pretty common for similar modules to include support for others (think of all the views support, ctools, panels, cck). That ffmpeg_wrapper might have some cross-compatibility code with ffmpeg_converter doesn't sound that unreasonable.

Thoughts? (Please?)

arthurf’s picture

arthurf commented

Ah ha! Ok this finally is starting to make some sense to me. Looking at the code sequence here I think that the ffmpeg_wrapper_convert_file() in ffmpeg_wrapper_ui_node_action_form() needs to be moved into a validation function rather than happen during the form build process. I'm investigating this.

arthurf’s picture

arthurf commented

I made some changes of where the conversion actually happens which I think should help make things work correctly for form alter.

Nathan Goulding’s picture

Nathan Goulding commented

Unfortunately this conversion is still happening before it reaches my module. On line 722 of ffmpeg_wrapper_ui.module in the ffmpeg_wrapper_ui_js() function it calls drupal_process_form() and the first thing that does is validate:

http://api.drupal.org/api/drupal/includes--form.inc/function/drupal_proc...

So that it's never reaching my hook_form_alter.

Suggestions?

arthurf’s picture

arthurf commented

All right, I'm not sure that there is going to be an easy fix for this. I think it's an architecture problem that I created with the way that the ajax works. Can you reroll this patch against the current dev and I'll apply it?

Nathan Goulding’s picture

Nathan Goulding commented
StatusFileSize
new ffmpeg_wrapper.module.patch1.07 KB
new ffmpeg_wrapper_ui.module.patch2.27 KB

Sure thing, attached both patches here

arthurf’s picture

arthurf commented

Patch is committed. I'm not going to port this up to the 7.x version- I'd like to work to figure out how to make the architecture work with form_alter() which I'm pretty confident is possible. Ready for testing.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented
Status: Needs review » Needs work

BAD CODE!

both patches: variable_get('ffmpeg_converter_wrapper_default_preset') is wrong, missing 2nd parameter.
error message: "Warning: Missing argument 2 for variable_get(), called in ...modules/ffmpeg_wrapper/ffmpeg_wrapper.module on line 94"

ui patch: the default value of use_preset and ffmpeg_converter_preset in ffmpeg_wrapper_ui_configuration_form() form should be feet by $configuration-array, otherwise the form will always reset to the default values.

Bad comments: When I read the code, I does not know why there is ffmpeg_converter stuff. I think this should be explained.

BTW: I don't know the problem (I came across because I got above error message) but I don't like the fix. I see ffmpeg_wrapper as an API and therefor it should not care if on special module needs some special treatment. What do you do if there are 20 different modules using this API? You will implement exceptions for all of them?
If there is a need to invoke code from other modules, it should be done by calling a hook.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented

Another thing I don't understand is, why do you show the use_preset and ffmpeg_converter_preset on ffmpeg_wrapper_ui_configuration_form()? I was configuring my ffmpeg_converter presets and at the end of the preset I can: "Select which preset to use to convert this file." ... I am confused.

Status: needs work » needs even more work

arthurf’s picture

arthurf commented

@ospolar if you look through this thread it's pretty clear that I'm not happy about making this exception for ffmpeg converter. I'd very much like some suggestions on how to architect it in such a way that that this wasn't necessary- this should be possible via form_alter. If you look at the 7.x branch you can see that things are cleaner there, but not perfect.

Since I'm not using the converter module I accepted the patch on face value. I'd be glad to commit any changes that improve it.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented
StatusFileSize
new ffmpeg_wrapper-default-preset-1202902-16.patch1.53 KB

Quick fix to get rid of the warnings.

As said in #15 this should be done in a better way.

@Nathan Goulding: Any suggestions?

iva2k’s picture

iva2k commented
Priority: Normal » Critical

First, code issues. I see multiple problems in the committed patches here:

1. why 2 patches instead of one? A single patch can modify more than one file at a time (I advise the contributor to use diff on the whole directory). Just for clarity I will refer to the two patch files as the "committed patch".
2. It causes warnings (missing argument in variable_get()) - that alone prompts 'major' priority.
3. Not functional as intended (code in validate function uses wrong fields of $form_state, and thus never gets to work and does nothing)
4. Have usability problems ($use_preset argument is not used in the committed patch, the preset configuration form contains two extraneous inputs that are confusing and misleading, thankfully non-functional)

These need to be either fixed ASAP, or the whole committed patch removed until it is fixed.

Second, structure issue of responsibilities between involved modules:

From what I can understand, the purpose of the committed patch is merely to allow selecting an existing preset from ffmpeg_converter module when testing (or performing?) file conversion with ffmpeg_wrapper.

In the current state of things there is an overlap of functionality between ffmpeg_wrapper and ffmpeg_converter, which causes these integration headaches. When two modules partition one function (such as conversion) somewhere in the middle like siamese twins (one part is done in one module, another is in the second module without a clear-cut interface inbetween) - there will always be problems and they will keep coming back.

I see two possible approaches here that have inner logic and therefore will be clean and simple:
A. Make presets a part of ffmpeg_wrapper module's functionality (then dependence on other modules is removed from ffmpeg_wrapper)
B. Make file conversion a part of ffmpeg_converter module (and therefore move the whole conversion form to ffmpeg_converter).
(here ffmpeg_wrapper can keep its converter functionality, but only as a "test", as I believe was intended in the first place).

What do people think about A vs. B vs. current state?

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented

@iva2k you are right, there is no need for two patches in one issue. But yes there is a need for two tickets for two different issues, don't mix them, because it's difficult to handle. BTW: Before opening a new issue for the second part (it's clearly a ffmpeg_converter issue) just check the issue queue there, see for example #1236572: Use ffmpeg_wrapper_convert_file instead of ffmpeg_converter_convert. Would be nice if you may help out there.

iva2k’s picture

iva2k commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new ffmpeg_wrapper.#1202902.20111002.patch4.51 KB

Here's a patch with a quick fix of the code in 6.x-2.x-dev. It resolves issues 1 through 4 mentioned in #17. I tested it and conversion now works with ffmpeg_converter preset. Please commit.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented
Status: Reviewed & tested by the community » Needs work

@iva2k: May you post the patch in #19 again ... without the # character - I get not found error.
BTW: your patch was not reviewed yet.

iva2k’s picture

iva2k commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new ffmpeg_wrapper.1202902.20111002.patch4.51 KB

oops. Here it is renamed.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
commented
Status: Reviewed & tested by the community » Needs review

Don't rtbc your own patches.

iva2k’s picture

iva2k commented

I know what you mean. But it was not my patch. I tested it, found problems, reviewed it, fixed problems, tested again, passed tests. And I'm part of the community. Don't know what else you need from the community. Maybe another reviewer can chime in?