BrettSheleski / comchap

Commercial detection script to add chapters into video file
MIT License
138 stars 26 forks source link

Enhanced error and failure detection #9

Closed Reed97123 closed 8 years ago

Reed97123 commented 8 years ago

The last two nights when comchap was launched through my cron jobs I was surprised to find no chapters in the resulting files. I logged in and tested it again and again and analyzed my code again and again and couldn't find the issue.

Finally it occurred to me that since it was launching through cron the paths weren't setup properly and comchap wasn't flagging it as an error as it ran. I made some changes to comchap to address all the corner cases I found when I went into the script.

1) Comskip doesn't use proper exit codes so I add a check to ensure the edl file has been created and isn't of zero length. Comskip can run successfully, but with a zero length edl as a result and that too must be prevented because it causes the script logic to fail.

Either issues should not allow the script to proceed. The rest of the script is wholly dependent upon the edl file being present and of non-zero length. Because the cleanup for the lock file is at the end of the script I had to replicate the lock file cleanup and exit with error status in the middle of the script.

The way I did this isn't what I'd consider optimal. In Perl (my comfort zone) I would create a error subroutine which I would call in the event of an error. The error subroutine would print the error and it would also perform the file cleanup / removal of lock file, etc. That way the exit and cleanup doesn't need to be replicated multiple times. I'm out of my depth in Bash to do something similar.

if [ ! -f "$edlfile" ]; then
  $comskipPath --ini="$comskipini" "$infile"

  if [ ! -f "$edlfile" ] || [ ! -s "$edlfile" ]; then
    echo Error running comskip. 1>&3 2>&4

    if [ ! -z "$lockfile" ]; then
      rm "$lockfile"
    fi

    exit -1
  fi
fi

2) Adding in a 'echo' that is always displayed (including without verbose) if ffmpeg didn't run successfully.

  if [ "$infile" -ef "$outfile" ] ; then

    tempfile=`mktemp --suffix=."$outextension" "$outdir"/XXXXXXXX`

    echo Writing file to temporary file: "$tempfile"
    if $ffmpegPath -loglevel error -hide_banner -nostdin -i "$infile" -i "$metafile" -map_metadata 1 -codec copy -y "$tempfile" 1>&3 2>&4; then
      mv -f "$tempfile" "$outfile"
      echo Saved to: "$outfile"
    else
      echo Error running ffmpeg. 1>&3 2>&4
      exitcode=-1
    fi

3) Return the exit code to the OS. You set the '$exitcode' variable, but you never actually exit with that exit code anywhere. I added an exit with the exit code at the end.

exit $exitcode

4) Added a check that the output file that should have been created is actually present.

  if [ ! -f "$outfile" ]; then
    echo Error, "$outfile" does not exist. 1>&3 2>&4
    exitcode=-1
  fi

5) Removed encoding when no commercials are present. If no commercials are present it should be counted as an error because the main point of running is to detect commercials. Previously it was simply running ffmpeg and copying the streams and acting like it actually successfully ran.

6) Minor code cleanup, removed a redundant outfile assignment. There's no need to do the initial 'outfile' assignment because the conditional that follows assigns it in either case, making the original redundant.

infile=$1

if [[ -z "$2" ]]; then
  outfile="$infile"
else
  outfile="$2"
fi
Reed97123 commented 8 years ago
#!/usr/bin/env bash

#LD_LIBRARY_PATH is set and will mess up ffmpeg, unset it, then re-set it when done
ldPath=${LD_LIBRARY_PATH}
unset LD_LIBRARY_PATH

exitcode=0

ffmpegPath="ffmpeg"
comskipPath="comskip"

if [[ $# -lt 1 ]]; then

  exename=$(basename "$0")

  echo "Add chapters to video file using EDL file"
  echo "     (If no EDL file is found, comskip will be used to generate one)"
  echo ""
  echo "Usage: $exename infile [outfile]"

  exit 1
fi

comskipini=$HOME/.comskip.ini

deleteedl=true
deletemeta=true
deletelog=true
deletelogo=true
deletetxt=true
verbose=false
lockfile=""

while [[ $# -gt 1 ]]
do
key="$1"
case $key in
    --keep-edl)
    deleteedl=false
    shift
    ;;
    --keep-meta)
    deletemeta=false
    shift
    ;;
    --verbose)
    verbose=true
    shift
    ;;
    --ffmpeg=*)
    ffmpegPath="${key#*=}"
    shift
    ;;
    --comskip=*)
    comskipPath="${key#*=}"
    shift
    ;;
    --comskip-ini=*)
    comskipini="${key#*=}"
    shift
    ;;
    --lockfile=*)
    lockfile="${key#*=}"
    shift
    ;;
    *)
    break
    ;;
esac
done

# Setup for verbose
exec 3>&1
exec 4>&2

if ! $verbose; then
  exec 1>/dev/null
  exec 2>/dev/null
fi

if [ ! -z "$lockfile" ]; then

  echo "lockfile: $lockfile"
  while [[ -f "$lockfile" ]]; do
    echo "Waiting"
    sleep 5
  done

  touch "$lockfile"
fi

if [ ! -f "$comskipini" ]; then
  echo "output_edl=1" > "$comskipini"
elif ! grep -q "output_edl=1" "$comskipini"; then
  echo "output_edl=1" >> "$comskipini"
fi

infile=$1

if [[ -z "$2" ]]; then
  outfile="$infile"
else
  outfile="$2"
fi

outdir=$(dirname "$outfile")

outextension="${outfile##*.}"

edlfile="${infile%.*}.edl"
metafile="${infile%.*}.ffmeta"
logfile="${infile%.*}.log"
logofile="${infile%.*}.logo.txt"
txtfile="${infile%.*}.txt"

if [ ! -f "$edlfile" ]; then
  $comskipPath --ini="$comskipini" "$infile"

  if [ ! -f "$edlfile" ] || [ ! -s "$edlfile" ]; then
    echo Error running comskip. 1>&3 2>&4

    if [ ! -z "$lockfile" ]; then
      rm "$lockfile"
    fi

    exit -1
  fi
fi

start=0
i=0
hascommercials=false

echo ";FFMETADATA1" > "$metafile"
# Reads in from $edlfile, see end of loop.
while IFS=$'\t' read -r -a line
do
  ((i++))

  end="${line[0]}"
  startnext="${line[1]}"
  hascommercials=true

  echo [CHAPTER] >> "$metafile"
  echo TIMEBASE=1/1 >> "$metafile"
  echo START="$start" >> "$metafile"
  echo END="$end" >> "$metafile"
  echo "title=Chapter $i" >> "$metafile"

  echo [CHAPTER] >> "$metafile"
  echo TIMEBASE=1/1 >> "$metafile"
  echo START="$end" >> "$metafile"
  echo END="$startnext" >> "$metafile"
  echo "title=Commercial $i" >> "$metafile"

  start=$startnext
done < "$edlfile"

if $hascommercials ; then
  ((i++))
  echo [CHAPTER] >> "$metafile"
  echo TIMEBASE=1/1 >> "$metafile"
  echo START="$start" >> "$metafile"
  echo END=`$ffmpegPath -i "$infile" 2>&1 | grep Duration | awk '{print $2}' | tr -d , | awk -F: '{ print ($1 * 3600) + ($2 * 60) + $3 }'` >> "$metafile"
  echo "title=Chapter $i" >> "$metafile"

  if [ "$infile" -ef "$outfile" ] ; then

    tempfile=`mktemp --suffix=."$outextension" "$outdir"/XXXXXXXX`

    echo Writing file to temporary file: "$tempfile"
    if $ffmpegPath -loglevel error -hide_banner -nostdin -i "$infile" -i "$metafile" -map_metadata 1 -codec copy -y "$tempfile" 1>&3 2>&4; then
      mv -f "$tempfile" "$outfile"
      echo Saved to: "$outfile"
    else
      echo Error running ffmpeg. 1>&3 2>&4
      exitcode=-1
    fi
  else
    if $ffmpegPath -loglevel error -hide_banner -nostdin -i "$infile" -i "$metafile" -map_metadata 1 -codec copy -y "$outfile" 1>&3 2>&4; then
      echo Saved to: "$outfile"
    else
      echo Error running ffmpeg. 1>&3 2>&4
      exitcode=-1
    fi
  fi

  if [ ! -f "$outfile" ]; then
    echo Error, "$outfile" does not exist. 1>&3 2>&4
    exitcode=-1
  fi
else
  echo Error, no Commercials found, not saving to new location.  Nothing to do. 1>&3 2>&4
  exitcode=-1
fi

if $deleteedl ; then
  if [ -f "$edlfile" ] ; then
    rm "$edlfile";
  fi
fi

if $deletemeta ; then
  if [ -f "$metafile" ]; then
    rm "$metafile";
  fi
fi

if $deletelog ; then
  if [ -f "$logfile" ]; then
    rm "$logfile";
  fi
fi

if $deletelogo ; then
  if [ -f "$logofile" ]; then
    rm "$logofile";
  fi
fi

if $deletetxt ; then
  if [ -f "$txtfile" ]; then
    rm "$txtfile";
  fi
fi

if [ ! -z $ldPath ] ; then
  #re-set LD_LIBRARY_PATH
  export LD_LIBRARY_PATH="$ldPath"
fi

if [ ! -z "$lockfile" ]; then
  rm "$lockfile"
fi

exit $exitcode
BrettSheleski commented 8 years ago

I'm looking forward to reviewing this. It would be tons easier if it were made into a pull request.

However I disagree on point number 5. I don't think it should be an error when no commercials were detected.

Thanks for reviewing and cleaning up the code, feedback like this is always helpful.

Reed97123 commented 8 years ago

I tried creating a pull request in the past, but I ended up accidentally forking the code to my area. Any simple tips to creating a pull request?

BrettSheleski commented 8 years ago

There may be easier ways, but I find forking the repo, committing to the fork easiest. GitHub then makes it easy to submit a pull request from your fork to the original.

On Tue, Nov 22, 2016, 12:26 PM Reed97123 notifications@github.com wrote:

I tried creating a pull request in the past, but I ended up accidentally forking the code to my area. Any simple tips to creating a pull request?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/BrettSheleski/comchap/issues/9#issuecomment-262323420, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjwuvPVHx_xoFFnMlBEAjDn9u0BToETks5rAzPcgaJpZM4K5wU6 .

Reed97123 commented 8 years ago

I found a typo in what I submitted above. I'm testing the fix now and I'll update the above and create a pull request once I confirm it's good.

On point 5 my logic is the following:

BrettSheleski commented 8 years ago

I see your point, but just don't necessarily agree it should be seen as an error. I realize in your situation you'd like to treat it as such, but I don't think this would be the general case.

With that said, I wouldn't see a problem making some sort of command line argument which would exit with error status when no commercials found.

I'm unsure exactly what that would look like at the moment though.

On Tue, Nov 22, 2016, 12:40 PM Reed97123 notifications@github.com wrote:

I found a typo in what I submitted above. I'm testing the fix now and I'll update the above and create a pull request once I confirm it's good.

On point 5 my logic is the following:

  • I run comchap because I believe there are commercials in the video and I want it processed.
  • If no commercials are found it is likely a symptom of an error such as: video file is somehow truncated/corrupt, .comskip.ini has an issue, etc.
  • In every case I can imagine I want the issue brought to my attention so I can fix it.
  • Flagging it as an error brings it to my attention and allows me to fix it.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/BrettSheleski/comchap/issues/9#issuecomment-262326969, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjwushMa72q9Ok2hT-TahlsEzE6Jb5Lks5rAzcJgaJpZM4K5wU6 .

BrettSheleski commented 8 years ago

On the topic of creating a pull request, I think the proper way of doing so is to create your own branch in this repository then commit to that. Then create a pull request from your branch to master.

Reed97123 commented 8 years ago

Okay, I think I created the pull request correctly. Thanks for the tips.

BrettSheleski commented 8 years ago

I see it. I'll review when I get time to sit at a computer.

Reed97123 commented 8 years ago

Created a new pull request to address the issues mentioned in the pull request discussion.