Passing arguments to system commands from perl

Question

I'm writing a perl script that uses some ImageMagick commands, specifically identify. The relevant code is here:

my $height = system("identify -format %h $curPic");
my $width = system("identify -format %w $curPic");

When I run the whole script,it gets hung up on these lines and this is the output:

identify: unable to open image 'if0W211.jpg': No such file or directory @ error/blob.c/OpenBlob/3323
identify: unable to open image 'if0W211.jpg': No such file or directory @ error/blob.c/OpenBlob/3323

At first, the problem was related to ImageMagick not have the correct format delegates to work with jpg images, but after fixing that, I'm still getting this error. I can't find any error documentation related to "error/blob.c/OpenBlob/3323". After writing some test code to see what the problem might be, I think I've determined that it has something to do with the way perl passes arguments to system commands, because when I write that system command identify -format %h xxxx.jpg in the terminal, it works just fine. I also noticed that when I print "$curPic\n, a 256 is prepended to the filename during the print. I don't know why this would be.

For reference, here is how I collect the filenames:

opendir DIR, $folder or die "Cannot open directory: $!";
my @files = readdir(DIR);
closedir(DIR);

And here is the full script:

#!/usr/bin/perl -w

use strict;
use diagnostics;
use File::Copy;

my $folder = "/media/sf_Pictures_from_Reddit";
my $oriFolder = "/media/sf_WrongOrientation";
my $resFolder = "/media/sf_LowRes";

#Collect all the files
opendir DIR, $folder or die "Cannot open directory: $!";
my @files = readdir(DIR);
closedir(DIR);

#Iterate through each file and check its orientation and resolution
foreach my $curPic (@files) {
    my $height = system("identify -format %h $curPic");
    my $width = system("identify -format %w $curPic");

    #move those that are vertically oriented to a different folder
    if ($height >= ($width*0.8)) {
    move($curPic, $oriFolder//$curPic) or die "The ori move operation     failed for image $curPic: $!";
        print "$curPic was not approved because of its orientation.";
        next;
    }
    #move those that are low res to a third folder
    elsif (($height < 1080) | ($width < 1920)) {
    move($curPic, $resFolder//$curPic) or die "The res move operation     failed for image $curPic: $!";
        print "$curPic was not approved because of its resolution.";
        next;
    }
    print "$curPic is approved as a desktop background";
}

EDIT: I'm switching to the recommended Image::Size library, so here is my updated script. It works for a time, giving me the output I want, but suddenly breaks and says the variables are uninitialized. "Use of uninitialized variable..." There's an error for $height and $width, but once again they both happen after about 20 successful iterations. And it seems to vary if I run the script multiple times back to back.

#!/usr/bin/perl -w

use strict;
use diagnostics;
use File::Copy;
use Image::Size;

my $folder = "/media/sf_Pictures_from_Reddit";
my $oriFolder = "/media/sf_WrongOrientation";
my $resFolder = "/media/sf_LowRes";

my $height = 0;
my $width = 0;

#Collect all the files
opendir DIR, $folder or die "Cannot open directory: $!";
my @files = readdir(DIR);
closedir(DIR);

#Iterate through each file and check its orientation and resolution
foreach my $curPic (@files) {
    ($width, $height) = imgsize("$folder/$curPic");

    #move those that are vertically oriented to a different folder
    if ($height >= ($width*0.8)) {
        move("$folder/$curPic", "$oriFolder/$curPic") or die "The ori move operation failed for image $curPic: $!";
        print "$curPic was not approved because of its orientation.\n";
        next;
    }
    #move those that are low res to a third folder
    elsif (($height < 1080) | ($width < 1920)) {
        move("$folder/$curPic", "$resFolder/$curPic") or die "The res move operation failed for image $curPic: $!";
        print "$curPic was not approved because of its resolution.\n";
        next;
    }
    print "$curPic is approved as a desktop background.\n";
}

Answer 1

As the message says, you are passing the path to a non-existent file. Instead of passing

if0W211.jpg

you should be passing

/media/sf_Pictures_from_Reddit/if0W211.jpg

That still leaves you with the following problems:

• An injection bug
• Possible misinterpretation of the path as an option.
• A lack of error handling
• Lack of capture of the output of the program.
• Repeated executions of an external process.

All these can be fixed by using Image::Size instead.

But if you insist on using identify,

use IPC::System::Simple qw( capturex );

my $dimensions = eval { capturex("identify", "-format", "%h,%w", "--", "$folder/$curPic") }
    or do {
       warn("Can't determine the dimensions of \"$folder/$curPic\": $@");
       next;
    };

my ($height, $width) = $dimensions =~ /^(\d+),(\d+)$/
   or do {
       warn("Can't determine the dimensions of \"$folder/$curPic\": Unexpected output from \"identify\": $dimensions\n");
       next;
   };

If your identify doesn't support -- (or even if it does), you can replace

"--", "$folder/$curPic"

with

"$folder/$curPic" =~ s{^-}{./-}r

Answer 2

Here is the final script that works for my purposes. I have added definition checks and non-zero checks to make sure that the variables are receiving the proper input before moving forward.

#!/usr/bin/perl -w

use strict;
use diagnostics;
use File::Copy;
use Image::Size;

my $folder = "/media/sf_Pictures_from_Reddit";
my $oriFolder = "/media/sf_WrongOrientation";
my $resFolder = "/media/sf_LowRes";

my $height = 0;
my $width = 0;

#Collect all the files
opendir DIR, $folder or die "Cannot open directory: $!";
my @files = readdir(DIR);
closedir(DIR);

#Iterate through each file and check its orientation and resolution
foreach my $curPic (@files) {

    ($width, $height) = imgsize("$folder/$curPic") or die "Couldn't get the image dimensions for $curPic: $!";

    if($curPic eq "." || $curPic eq ".."){ next;}
    if((defined $height) & (defined $width)){
        if(($height != 0) & ($width != 0)) {

            print "File: $curPic\nHeight: $height\nWidth: $width\n";
            #sleep(0.5);

            #move those that are vertically oriented to a different folder
            if($height >= ($width*0.8)) {
                move("$folder/$curPic", "$oriFolder/$curPic") or die "The ori move operation failed for $curPic: $!";
                print "$curPic was not approved because of its orientation.\n\n";
                next;
            }
            #move those that are low res to a third folder
            elsif(($height < 1080) | ($width < 1920)) {
                move("$folder/$curPic", "$resFolder/$curPic") or die "The res move operation failed for $curPic: $!";
                print "$curPic was not approved because of its resolution.\n\n";
                next;
            }
            print "$curPic is approved as a desktop background.\n\n";
            $height = 0;
            $width = 0;
        }
        else{
            print "Variables failed to initialize.\n\n";
        }
    }
    else{
        print "Variables are undefined.\n\n";
    }
}

Answer 3

As it stands, you are creating two new processes (one to identify the width and another to identify the height) for each and every image, so that potentially causes a considerable load on your system if you have large numbers of images.

As an alternative, you can just invoke one single identify process and pass it all the image names, like this:

identify -format "%w:%h:%f\n" *jpg

Sample Output

2000:1200:ok.jpg
1200:2000:toolow2.jpg
1000:500:vert.jpg

You can then parse that with bash or Perl:

#!/bin/bash

VERT="/tmp"
TOOLOW="/tmp" 

identify -format "%w:%h:%f\n" *jpg | 
   while IFS=: read w h name; do
      echo "DEBUG: $name, $w, $h"
      [[ $h -gt $(( (8*w)/10 )) ]] &&         { mv "$name" "$VERT";   >&2 echo "Too tall: $name"; continue; }
      [[ ($h -lt 1080) || ($w -lt 1920) ]] && { mv "$name" "$TOOLOW"; >&2 echo "Low res: $name";  continue; }
      echo "$name approved"
   done