0

I am trying to loop a set of jpg images in a folder. The problem is some of the images have the extension in all lower case letters (.jpg) and others have all capital letters (.JPG).

I am trying to use variable modifiers to change the extensions as the loop progresses.

I figured out how to phrase the for var in LIST part but I can't get the modifiers to replace the extensions correctly.

Essentially I need the jpg files to convert to files with .mpc and .cache extensions.

I end up with files that have names like FILE.jpg.mpc or FILE.JPG.jpg when I need the file to be FILE.mpc and FILE.cache

Here is my function that works great if you only use for i in *.jpg but when you add for i in *.{jpg,JPG} everything falls apart.

Again here is what I have so far.

imow()
{

    clear

    local i DIMENSIONS RANDOM

    # find all jpg files and create temporary cache files from them
    for i in *.{jpg,JPG}
    do
        # create random direcotories in case you are running this function more than once at the same time. it prevents cross-over.
        RANDOM="$(mktemp --directory)"
        "${RANDOM}" 2>/dev/null
        echo -e "\\nCreating two temporary cache files: ${RANDOM}/${i%%.jpg,JPG}.mpc + ${RANDOM}/${i%%.{jpg,JPG}}.cache\\n"
        DIMENSIONS="$(identify -format '%wx%h' "${i}")"
        convert "${i}" -monitor -filter 'Triangle' -define filter:support='2' -thumbnail "${DIMENSIONS}" -strip \
        -unsharp '0.25x0.08+8.3+0.045' -dither None -posterize '136' -quality '82' -define jpeg:fancy-upsampling='off' \
        -define png:compression-filter='5' -define png:compression-level='9' -define png:compression-strategy='1' \
        -define png:exclude-chunk='all' -auto-level -enhance -interlace 'none' -colorspace 'sRGB' "${RANDOM}/${i%%.{jpg,JPG}}.mpc"
        clear
        for i in "${RANDOM}"/*.mpc
        do
            if [ -f "${i}" ]; then
                echo -e "\\nOverwriting original file with optimized self: ${i} >> ${i%%.mpc}.jpg\\n"
                convert "${i}" -monitor "${i%%.mpc}.jpg"
                if [ -f "${i%%.mpc}.jpg" ]; then
                    mv "${i%%.mpc}.jpg" "${PWD}"
                    rm -fr "${RANDOM}"
                    clear
                else
                    clear
                    echo 'Error: Unable to find the optimized image and therefore can'\''t overwrite the original.'
                    echo
                    exit 1
                fi
            fi
        done
    done
}

Like I mentioned if you run it with just the .jpg extension it works great. here is a working example.

imow()
{

    clear

    local i DIMENSIONS RANDOM

    # find all jpg files and create temporary cache files from them
    for i in *.jpg
    do
        # create random directories in case you are running this function more than once at the same time. it prevents cross-over.
        RANDOM="$(mktemp --directory)"
        "${RANDOM}" 2>/dev/null
        echo -e "\\nCreating two temporary cache files: ${RANDOM}/${i%%.jpg}.mpc + ${RANDOM}/${i%%.jpg}.cache\\n"
        DIMENSIONS="$(identify -format '%wx%h' "${i}")"
        convert "${i}" -monitor -filter 'Triangle' -define filter:support='2' -thumbnail "${DIMENSIONS}" -strip \
        -unsharp '0.25x0.08+8.3+0.045' -dither None -posterize '136' -quality '82' -define jpeg:fancy-upsampling='off' \
        -define png:compression-filter='5' -define png:compression-level='9' -define png:compression-strategy='1' \
        -define png:exclude-chunk='all' -auto-level -enhance -interlace 'none' -colorspace 'sRGB' "${RANDOM}/${i%%.jpg}.mpc"
        clear
        for i in "${RANDOM}"/*.mpc
        do
            if [ -f "${i}" ]; then
                echo -e "\\nOverwriting original file with optimized self: ${i} >> ${i%%.mpc}.jpg\\n"
                convert "${i}" -monitor "${i%%.mpc}.jpg"
                if [ -f "${i%%.mpc}.jpg" ]; then
                    mv "${i%%.mpc}.jpg" "${PWD}"
                    rm -fr "${RANDOM}"
                    clear
                else
                    clear
                    echo 'Error: Unable to find the optimized image and therefore can'\''t overwrite the original.'
                    echo
                    exit 1
                fi
            fi
        done
    done
slyfox1186
  • 301
  • 1
  • 13
  • 1
    _everything falls apart_ is not a very accurate description of what happens. Please explain what happens. It could be that you just need to set the `nullglob` option (`shopt -s nullglob`). – Renaud Pacalet Jan 31 '23 at 06:13
  • Yeah i struggle with conveying what I am doing. I'm sorry about that. – slyfox1186 Jan 31 '23 at 06:19
  • @slyfox1186 one of the reasons people ask you to describe your problem concisely is that it actually helps you solve it yourself, because when doing that, you may find a discrepancy between your description and what you're really doing. – daniu Jan 31 '23 at 06:23
  • 1
    You already know the extension is either jpg or JPG, just use `${i%.*}` to remove it. – oguz ismail Jan 31 '23 at 06:27
  • I completely understand what you are trying to tell me daniu.. I will try to rework my question. Thanks for the advice. – slyfox1186 Jan 31 '23 at 06:29
  • Of course you would need _nullglob_, as RenaudPacalet said in his comment, but this would still not catch mixed-case file extensions (such as `Foo.Jpg` or `Bar.jpG`). For being completely caseinsinsitive, `shopt -s nocaseglob` would help, but then you can't distinguish anymore beween `foo.jpg` and `FOO.jpg` either. Don't know whether this is a problem for your use case. . – user1934428 Jan 31 '23 at 07:30
  • 3
    Don't use all uppercase vars as they conflict with shell vars (i.e. RANDOM) – Diego Torres Milano Jan 31 '23 at 07:36
  • See [Correct Bash and shell script variable capitalization](https://stackoverflow.com/q/673055/4154375). – pjh Jan 31 '23 at 12:15
  • See [How can I debug a Bash script?](https://stackoverflow.com/q/951336/4154375). – pjh Jan 31 '23 at 12:28
  • 1
    I don't think that it's the cause of your problems, but using the same variable for outer and inner loops (here `for i in *.{jpg,JPG}` and `for i in "${RANDOM}"/*.mpc`) doesn't work in general, and is likely to cause maintenance problems even if it does work. Much better would be something like `for jpgfile in *.{jpg,JPG}` and `for mpcfile in "${RANDOM}"/*.mpc`. (Ignoring the fact that `RANDOM` is a special Bash variable and shouldn't be used like this.) – pjh Jan 31 '23 at 12:41
  • 1
    [Shellcheck](https://www.shellcheck.net/) reports the overridden loop variable. [Shellcheck](https://www.shellcheck.net/) finds many common (and some not so common) problems in shell code. It's a good idea to run it on all new and modified code. – pjh Jan 31 '23 at 12:53
  • `for i in ./*.[Jj][Pp][Gg]` maybe – Jetchisel Feb 01 '23 at 05:26

3 Answers3

1

${i%%.jpg,JPG} and ${i%%.{jpg,JPG}} don't do what you want. The first tries to remove the exact suffix .jpg,JPG, and the second tries to remove the exact suffix .{jpg,JPG}. Since the suffix is guaranteed to be either .jpg or .JPG at that point in the code, ${i%.*} is a safe alternative.

pjh
  • 6,388
  • 2
  • 16
  • 17
1

As I read it, your function already overwrites original .jpg files, but hardcodes a lowercase .jpg which leaves the old .JPG files lying around. I'd just formalize the conversion to lowercase unless there's some specific reason you shouldn't/can't.

Get rid of those broken uppercase variables, and your inner loop using the same loop var as the outer loop, etc. I moved some stuff around, but don't have imagemagick installed so this is a freehand edit; I trust someone will point out any glaring logic errors as I can't really test it the way I'd like.

I explicitly test the imagemagick return code to simplify a bit, and added a test to make sure the mv succeeded. If the original file's extension is not all lowercase, it now should be removed once the optimized file is staged.

I also removed the clear commands that were erasing output you had just explicitly logged in favor of sending convert spam to logs that get wiped unless there was a problem.

imow() {
  local orig mpc new dim rc tdir boilerplate
  tdir="$(mktemp --directory)" 
  boilerplate=( -monitor -filter 'Triangle' -define filter:support='2' 
    -strip -unsharp '0.25x0.08+8.3+0.045' -dither None -posterize '136'
    -quality '82' -define jpeg:fancy-upsampling='off' 
    -define png:compression-filter='5' -define png:compression-level='9'
    -define png:compression-strategy='1' -define png:exclude-chunk='all'
    -auto-level -enhance -interlace 'none' -colorspace 'sRGB' )
  for orig in *.[Jj][Pp][Gg]
  do dim="$(identify -format '%wx%h' "$orig")"
     if convert "$orig" "${boilerplate[@]}" -thumbnail "$dim" "${tdir}/${orig%.???}.mpc" > "$tdir/convert-mpc.out"
     then for mpc in "${tdir}"/*.mpc
          do new="${mpc%.mpc}.jpg"
             if convert "$mpc" -monitor "$new" > "$tdir/convert-jpg.out"
             then echo "Replacing $orig with optimized version $new"
                  if mv "$new" "$PWD"
                  then [[ "$orig" == "$new" ]] || rm -f "$orig"
                       rm -fr "${tdir:?safety check}/*.*" # can't expand to rm -fr /*
                  else rc=$?;
                       echo "Cannot mv '$new' to '$PWD': error $rc; aborting" >&2
                       exit $rc
                  fi
        else rc=$?
                  echo "convert error $rc on $mpc; c.f. $tdir - aborting." >&2
                  exit $rc
             fi
          done
     else rc=$?
          echo "convert error $rc on $orig; c.f. $tdir - aborting." >&2
          exit $rc
     fi
  done
  rm -fr "$tdir"
}

*.[Jj][Pp][Gg] with match any combination of upper/lower on the extension.

Make sure you test and modify to suit your needs.
Good luck.

addendum

I have discovered that changing the line if convert "${mpc}" -monitor "${new}" > "${tdir}/convert-jpg.out" to if convert "${mpc}" -monitor "${new}" > "${tdir}/convert-jpg.out" &> /dev/null GREATLY speeds up the parsing of each loop. Otherwise cheers.

Please c.f. the GNU bash guide on Redirections

Observe:

$: date >foo
$: cat foo
Fri Feb  3 12:33:13 CST 2023
$:

whereas:

$: date >foo &> /dev/null
$: cat foo
$:

in the second, date's output is redirected to foo, which opens (and creates or truncates) the file, but then &> redirects both stdout and stderr to /dev/null, overriding the first redirection to a logfile, so foo is empty after it runs.

Maybe it was "significantly faster" because it failed?
How would you know?

IMHO, never throw away your logs, especially stderr, even if you are testing your return. When it eventually does fail, you have nothing to debug. I usually clean up the logs after a success, but on a fail I usually cat them, then and abort.

Worst case, even if you do throw them away, remove the redundancy.
Assuming all use

if convert "${mpc}" -monitor "${new}" 

followed by some output control or other, rather than

 > "${tdir}/convert-jpg.out" &> /dev/null # log will always be empty

If you just do not want the stdout at all, try one of these -

Leaving stderr defaulting to the console,

> /dev/null                              # stdout to the bitbucket
>&-                                      # just *close* stdout

redirecting stderr to a log,

2> "${tdir}/convert-jpg.out" > /dev/null # stdout to the bitbucket
2> "${tdir}/convert-jpg.out" >&-         # stdout closed

if stdout has any value -

&> "${tdir}/convert-jpg.out"             # both to a specified log

or worst case, if you just insist,

&>/dev/null                              # BOTH to the bitbucket
&>-                                      # close both
Paul Hodges
  • 13,382
  • 1
  • 17
  • 36
  • This worked well as far as I can tell. Thank you so much. I see what you did there. Thank you very much for the support. – slyfox1186 Feb 01 '23 at 20:54
  • I have discovered that changing the line `if convert "${mpc}" -monitor "${new}" > "${tdir}/convert-jpg.out"` to `if convert "${mpc}" -monitor "${new}" > "${tdir}/convert-jpg.out" &> /dev/null` GREATLY speeds up the parsing of each loop. Otherwise cheers. – slyfox1186 Feb 01 '23 at 21:48
  • That's redundant - the log file will never have anything in it. Just remove it if you don't care... but please see my edited post. Never just throw away stderr - debugging that later is a nightmare. – Paul Hodges Feb 03 '23 at 19:12
  • I wasn't interested in a log file. It does work regardless as I've tested it but thanks for the info. One of the drawbacks of the way you coded it was that it loops every image before going on to the next image that hasn't been processed yet. This may be the best way to do things but it does slow down the entire operation from start to finish. By blocking the output to terminal that it produces it speeds up the looping as I just mentioned. If you are correct I am not sure whey it still works then. – slyfox1186 Feb 04 '23 at 08:38
  • You never suggested you wanted concurrency. That can be arranged, but it's a significant change. In either/*any* case, no logs means reduced confidence, though I suppose you can trust the return code if you're testing it. How many files are we talking about? How long is it taking? do you *have* the resources to run them all at once? – Paul Hodges Feb 07 '23 at 14:38
  • I do not have the resources to run all pictures at once in parallel. The max photo set is around 500 pictures. What approach do you think you would take for this given your previous reply? – slyfox1186 Feb 17 '23 at 08:48
  • Maybe you could use [parallel](https://linux.die.net/man/1/parallel). Other than that, it takes what it takes - a certain number of cycles per task. You can improve efficiencies, but the parts that aren't in your control are hard to tune. You have sequential calls to imagemagick per file, so unless you can parallel those I'm not sure how you'd speed that up - not familiar with the software. – Paul Hodges Feb 17 '23 at 17:04
  • Thanks for all the input. I will continue to search for a leaner solution and use your answer as a reference for possible solutions. – slyfox1186 Feb 18 '23 at 00:46
0

Does this work for you?

shopt -s nullglob
cd
cd desktop/tests
for i in *.jpg *.JPG; do
echo $i
done
fmw42
  • 46,825
  • 10
  • 62
  • 80