[Bash]Critique my script

[afulldevnull]

Hi All,

I just wrote my first bash script, and am requesting some constructive criticism on it. The script simply uses ffmpeg to get a clip from a video file. The reason I wrote it is because typically ffmpeg takes the starting position and then you specify how long you want the clip to be. I prefer to enter the starting and ending times. Granted it is not a complicated script but I wanted to learn how to write bash scripts so I thought it would be a good starting spot.

TL;DR wrote my first bash script. it uses ffmpeg to create a video clip. looking for criticisms

Thanks guys!

#!/bin/bash
REGEX="[0-9][0-9]:[0-9][0-9]:[0-9][0-9]"
REGEXTXT="##:##:##"
USAGE="./cutVideo.sh pathtofile startTime endTime outfile";


#
#This script requires ffmpeg
#
type -P ffmpeg &> /dev/null || { echo "Error: ffmpeg required, but not found.. aborting" >&2; exit 1; }




#
#If no arguments are given, echo the usage and exit 1
#
if [[ $# -lt 1 ]];
	then { 
		echo "Usage: "$USAGE >&2; 
		exit 1; 
	}
fi




#
#Check if all the parameters are there
#
if [ -z "$1" ]; 
	 then { 
		echo "Error: No path supplied." >&2; 
		exit 1; 
	}
fi

if [ -z "$2" ]; 
	 then { 
		echo "Error: No startTime supplied." >&2; 
		exit 1; 
	}
fi

if [ -z "$3" ]; 
	then { 
		echo "Error: No endTime supplied." >&2; 
		exit 1; 
	}
fi

if [ -z "$4" ]; 
	then { 
		echo "Error: No outfile supplied." >&2; 
		exit 1; 
	}
fi




#
#Check if the file path is valid
#
if [ `file $1 | grep -c "cannot open"` == 1 ];
	 then echo "Error: File not found." 
fi




#
#Check if starttime and endtime are of a valid format
#
if [[ $2 =~ $REGEX ]];
	then {
		echo "$2 matches $REGEXTXT...";
	}
	else {
		echo "$2 does not match $REGEXTXT. Aborting." >&2;
		exit 1;
	}
fi

if [[ $3 =~ $REGEX ]];
	then {
		echo "$3 matches $REGEXTXT...";
	}
	else {
		echo "$3 does not match $REGEXTXT. Aborting." >&2;
		exit 1;
	}
fi




#
#Calculate the difference between between endtime and starttime for ffmpeg
#
ENDHOUR=`echo $3 | cut -d : -f 1`;
ENDMIN=`echo $3 | cut -d : -f 2`;
ENDSEC=`echo $3 | cut -d : -f 3`;
ENDSECONDS=`expr $ENDHOUR \* 3600 + $ENDMIN \* 60 + $ENDSEC`;

STARTHOUR=`echo $2 | cut -d : -f 1`;
STARTMIN=`echo $2 | cut -d : -f 2`;
STARTSEC=`echo $2 | cut -d : -f 3`;
STARTSECONDS=`expr $STARTHOUR \* 3600 + $STARTMIN \* 60 + $STARTSEC`;

DIFFSECONDS=`expr $ENDSECONDS - $STARTSECONDS`;

if [ $DIFFSECONDS -lt 0 ];
	then {
		echo "Ending time cannot be before starting time. Aborting." >&2;
		exit 1;
	}
	else {
		echo "Starting offset in seconds: $STARTSECONDS";
		echo "Ending offset in seconds: $ENDSECONDS";
		echo "Seconds between: $DIFFSECONDS";
	}
fi



#Execute. Exit.
echo "Executing command: ffmpeg -i $1 -sameq -ss $STARTSECONDS -t $DIFFSECONDS $4";
ffmpeg -i $1 -sameq -ss $STARTSECONDS -t $DIFFSECONDS  $4;
exit 0;

[ramin.honary]

You have definitely done your homework! This is a good script!

I would just make a few minor changes. (I screwed with the whitespace to make the script shorter, you can ignore my stylistic changes.)

#!/bin/bash
REGEX="[0-9][0-9]:[0-9][0-9]:[0-9][0-9]"
REGEXTXT="##:##:##"
USAGE="./cutVideo.sh pathtofile startTime endTime outfile";

#This script requires ffmpeg
if type -P ffmpeg &> /dev/null
then
  echo "Error: ffmpeg required, but not found.. aborting" >&2
  exit 1
fi

#If no arguments are given, echo the usage and exit 1
#if [[ $# -lt 1 ]]
#then echo "Usage: "$USAGE >&2; exit 1
#fi
# This is nitpicking, but I wouldn't bother with checking the number of input
# arguments. The inputs are either correct or not.

# It is better to assign $1, $2, $3, etc to meaningful variable
# names to avoid confusion.
INFILE="$1"
START="$2"
END="$3"
OUTFILE="$4"
# Then, use a shell function to check each input parameter.
check() {
  if [ -z "$1" ]
  then
    echo "Error: No $2 supplied" >&2
    echo $USAGE >&2
    exit 1
  fi
}
check "$INFILE" "path"
check "$START" "startTime"
check "$END" "endTime"
check "$OUTFILE" "outfile"

#Check if the file path is valid
#if [ `file $1 | grep -c "cannot open"` == 1 ];
#	 then echo "Error: File not found." 
#fi
# I would do this instead:
if [ ! -f "$INFILE" ]
then echo "Error: File not found \"$INFILE\"." >&2; exit 1
fi

#Check if starttime and endtime are of a valid format
# I would do a shell function again.
check_time() {
  if [[ "$1" =~ $REGEX ]]
  then echo "$2: \"$1\" matches $REGEXTXT..."
  else echo "$2: \"$1\" does not match $REGEXTXT. Aborting." >&2; exit 1
  fi
}
check_time "$START" "Start time"
check_time "$END" "End time"

#Calculate the difference between between endtime and starttime for ffmpeg
#ENDHOUR=`echo $3 | cut -d : -f 1`;
#ENDMIN=`echo $3 | cut -d : -f 2`;
#ENDSEC=`echo $3 | cut -d : -f 3`;
#ENDSECONDS=`expr $ENDHOUR \* 3600 + $ENDMIN \* 60 + $ENDSEC`;
#
#STARTHOUR=`echo $2 | cut -d : -f 1`;
#STARTMIN=`echo $2 | cut -d : -f 2`;
#STARTSEC=`echo $2 | cut -d : -f 3`;
#STARTSECONDS=`expr $STARTHOUR \* 3600 + $STARTMIN \* 60 + $STARTSEC`;
#
#DIFFSECONDS=`expr $ENDSECONDS - $STARTSECONDS`;

# instead of doing lots of cuts, I would use "sed" and output to "bc".
# The disadvantage, of course, is that your script now depends on "bc" as well.
get_time() {
  TIME=$(echo "$1" \
      | sed -e 's,\([0-9][0-9]\):\([0-9][0-9]\):\([0-9][0-9]\),(3600 * \1 + 60 * \2 + \3),g' \
      | bc)
  # This sed script is setup to convert all substrings that match the regular expression
  # into an expression that can be computed by "bc". This allows you to pass multiple
  # time arguments separated by "+ - * / %" operators as a single string and still have
  # it compute the correct answer. This is useful for computing the time difference.
}
get_time $END; ENDSECONDS=$TIME
get_time $START; STARTSECONDS=$TIME
get_time "$END - $START"; DIFFSECONDS=$TIME

if [ $DIFFSECONDS -lt 0 ];
then echo "Ending time cannot be before starting time. Aborting." >&2; exit 1
else
  echo "Starting offset in seconds: $STARTSECONDS";
  echo "Ending offset in seconds: $ENDSECONDS";
  echo "Seconds between: $DIFFSECONDS";
fi

#Execute. Exit.
echo "Executing command: ffmpeg -i $FILE -sameq -ss $STARTSECONDS -t $DIFFSECONDS $OUTFILE";
ffmpeg -i "$INFILE" -sameq -ss $STARTSECONDS -t $DIFFSECONDS  "$OUTFILE"
exit 0

So, I hope you liked the changes I made!

[afulldevnull]

Thanks ramin!

This is exactly what I was hoping for! I do enjoy your use of functions and also the comments on better scripting practice! I'm going to make some updates!

Thanks again

-devnull