Find the answer to your Linux question:
Results 1 to 3 of 3
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. ...
Enjoy an ad free experience by logging in. Not a member yet? Register.
  1. #1
    Just Joined!
    Join Date
    Jan 2011
    Posts
    4

    [Bash]Critique my script


    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!

    Code:
    #!/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;

  2. #2
    Linux User
    Join Date
    Nov 2008
    Location
    Tokyo, Japan
    Posts
    260
    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.)
    Code:
    #!/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!

  3. #3
    Just Joined!
    Join Date
    Jan 2011
    Posts
    4
    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

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •