[MPlayer-cvslog] CVS: main/TOOLS psnr-video.sh, NONE, 1.1 README, 1.9, 1.10

[Diego Biurrun]

On Mon, Sep 12, 2005 at 06:36:01PM +0200, Guillaume Poirier CVS wrote:
> 
> 
> #!/bin/sh
> # Helper script to ease comparing two video files

Comparing what?  The quality?

> # Copyleft 2001 by Matthias Wieser

It's really that old?

> # This file comes under GPL, see http://www.gnu.org/copyleft/gpl.html for more
> # information on it's licensing.

its

>    echo " <file1> and <file2> are the video files for which the PSNR should be calculated."

This line exceeds 80 characters, just leave out the word "video".

>    echo " [<frames>]          is the number of frames to process, starting from frame 1."
>    echo " [<options1>]        are additional MPlayer options for <file1>"
>    echo " [<options2>]        are additional MPlayer options for <file2>"

Sometimes you have a period at the end, sometimes you don't...

>    echo " Be aware that `basename $0` needs a lot of temporal space inside /temp/."

What is /tEmp/ ?

s/temporal/temporary/

> 	echo "Will process $LastFrame frames"

Nit: Add a period at the end.

> if [ $# -ge 4 ]; then
> 	FILE1Opts=$4
> 	echo "Mplayer options for ${FILE1}: $FILE1Opts"
> fi
> 
> if [ $# -ge 5 ]; then
> 	FILE2Opts=$5
> 	echo "Mplayer options for ${FILE2}: $FILE2Opts"

Misspelling MPlayer is a criminal offense IMNSHO.

> rm *ppm 2> /dev/null
> rm *del 2> /dev/null

Just use rm -f.

> 	mplayer $FILE1Opts -frames $LastFrame -nosound -vo pnm ${WORKDIR}$FILE1 >/dev/null
> else
> 	mplayer $FILE1Opts -nosound -vo pnm ${WORKDIR}$FILE1 >/dev/null

Hmm, I read $FILE1Opts as "$file10 pts".  This is admittedly a small
problem, but ugly.  Also, I'd put a space after > for better
readability.

> ###  File 2
> 
> echo
> echo "############## $FILE2 #################"
> 
> cd ${TEMPDIR}/FILE2
> 
> rm *ppm 2> /dev/null
> 
> if [ $LastFrame -ge 0 ]; then
> 	mplayer $FILE2Opts -frames $LastFrame -nosound -vo pnm ${WORKDIR}$FILE2 >/dev/null
> else
> 	mplayer $FILE2Opts -nosound -vo pnm ${WORKDIR}$FILE2 >/dev/null

dito

> echo "############## Calculation PSNR #################"

PSNR Calculation

> if [[ `ls -1 ${TEMPDIR}/FILE1/*ppm|wc -l` = `ls -1 ${TEMPDIR}/FILE2/*ppm|wc -l` ]]

Please put spaces around the | for better readability, same below.

>         grep "Y" del.del | dd bs=1c count=5 skip=29 of=del2.del 2>/dev/null
>                 Y=`cat del2.del`
>                echo -n "$Y;">>../psnr.dat

WTH?  Try using cut instead of messing around with dd and multiple
files, same below, maybe even make a function for it..

>         echo "Files have differing numbers of frames!"
>         echo "$FILE1 has `ls -1 ${TEMPDIR}/FILE1/*ppm|wc -l` frames,"
>         echo "$FILE2 has `ls -1 ${TEMPDIR}/FILE2/*ppm|wc -l` frames."
>         echo "Processed the first `ls -1 ${TEMPDIR}/FILE2/*ppm|wc -l` frames."
>         echo

Huh, again?

> --- README	24 Aug 2005 00:24:11 -0000	1.9
> +++ README	12 Sep 2005 16:35:58 -0000	1.10
> @@ -254,6 +254,51 @@
>  
> +Description:  Calculates the PSNR between two existing video files.

Hmm, this is wrong or at least awkward on multiple levels.

PSNR is a property of a video if I am not badly mistaken.  So what you
do is calculate the PSNR difference or compare the two PSNRs.

> +              Prints also the overall PSNR.

Also prints the overall PSNR.

> +               * compare different softwarescalers (should I use
> +                 -sws 1 or -sws 2) ?
> +               * compare different resolutions (is it better to scale
> +                 down to 640x360 or to 560x320)
> +               * compare video filters (is it better to use -vf hqdn3d
> +                 or lavcopts:nr=400)

Put the question mark inside the parentheses and capitalize the
sentences.

> +              A file called psnr.dat will be created with the following

./psnr.dat ?

> +              Be aware that psnr-video.sh needs a lot of temporal space
> +              inside /temp/.

See above.

Diego