rsutphin / handbrake.rb

A ruby wrapper for HandBrakeCLI, a video transcoder
GNU Lesser General Public License v2.1
34 stars 6 forks source link

Problem with CLI options using optional arguments. #6

Open bmatsuo opened 13 years ago

bmatsuo commented 13 years ago

HandBrakeCLI options which make use of optional arguments need to have their argument given using the equals sign in the form --option=value. The gem doesn't do this. And as a result, these options always take on their default values.

For example, --denoise can be given with no arguments or as --denoise=strong, but not as --denoise strong. It seems the token "strong" is interpreted as a normal command line argument (which HandBrakeCLI doesn't appear to use or care about).

There appear to be many options which can be accepted with or without an argument. So this problem is fairly widespread amongst the more advanced HandBrakeCLI options (denoise, detelecine, deblock, deinterlace, ...).

edit: no need to worry about the following

It seems to me that the easiest solution would be to pass all options which are given an argument in the form '--option=option\'s value'. Note the quotes (and escaping any quotes contained in the option name/argument). It would also be nice to implement this option stringification as a method. Then, a script running on Windows can easily and safely override it.

bmatsuo commented 13 years ago

After analyzing the code and understanding it a little more than I did before, I found that the fix is fairly simple. The arguments method of HandBrake::CLI should be replaced with

    def arguments
        @args.collect { |req, *rest| "--#{req.to_s.gsub('_', '-')}#{rest.empty? ? '' : '=' + rest.join(":")}" }
    end

Edit: I don't know about the join, but currently there is really no legitimate reason to pass multiple arguments. However many of the filters accept multi-parameter arguments (e.g. --denoise=3:2:3:2). So I figured that it would be nice to use multiple arguments in this way (e.g. cli.denoise(3, 2, 3, 2)). But you should do whatever you think is best. Its not a big deal.

Double edit: There was no more reason to flatten

rsutphin commented 13 years ago

For example, --denoise can be given with no arguments or as --denoise=strong, but not as --denoise strong. It seems the token "strong" is interpreted as a normal command line argument (which HandBrakeCLI doesn't appear to use or care about).

Do you have reference for this? I should have some time to take a look at this issue this weekend.

bmatsuo commented 13 years ago

Only experimentation and some peeking through the HandBrake source.

$ HandBrakeCLI --denoise=254:254:254:254 --input Test.mp4 --output Out.mp4 2>&1 | egrep -i denoise || echo && rm Out.mp4 
[21:41:57]      + Denoise (hqdn3d) (254:254:254:254)
^C
$ HandBrakeCLI --denoise 254:254:254:254 --input Test.mp4 --output Out.mp4 2>&1 | egrep -i denoise || echo && rm Out.mp4 
[21:42:12]      + Denoise (hqdn3d) (default settings)
^C
$ HandBrakeCLI --denoise --input Test.mp4 --output Out.mp4 2>&1 | egrep -i denoise || echo && rm Out.mp4 
[21:42:25]      + Denoise (hqdn3d) (default settings)
^C
$ HandBrakeCLI --input Test.mp4 --output Out.mp4 2>&1 | egrep -i denoise || echo && rm Out.mp4 
^C
$ 

In the first three examples, denoising is turned on. The first correctly identifies the option value. The third demonstrates the value-less --denoise option. The second takes the parameters 254:254:254:254 as a command line argument and uses default denoising (the 254:...:254 parameters will produce terrible video, so you know it worked).

I found the exact lines of HandBrake source code once. I'll look it up again post it so you can find an exact list of options that exhibit this behavior.

Edit: There is a list of all options in the source around line 2922 of this file: https://github.com/HandBrake/HandBrake/blob/master/test/test.c