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

[Matthias Wieser]

Hi Guillaume, hi ivo,

Am Dienstag, 13. September 2005 09:49 schrieb Guillaume POIRIER:
> Hi Matthias,
>
> I don't know if you're subscribed to MPlayer-cvslog, so here is a
> message about your script, which, BTW, had been committed.
>
> If you could address the problems that Ivo has pointed out, that would
> be superb!

Unfortunately it took quite some time but here it is:

> ---------- Forwarded message ----------
> From: Ivo <ivop at euronet.nl>
>
> I have a few comments. It's maybe not that important since it's only a
> tool, but here they are anyway :)
>
> > TEMPDIR="/tmp/psnr_video"
> > WORKDIR=`pwd`/
>
> I'm not that fond of adding the trailing / to the variable. It gets
> confusing later on if a filename is added.

OK, changed that.

> > mkdir -p ${TEMPDIR}/FILE1
> > mkdir -p ${TEMPDIR}/FILE2
>
> -vo pnm support writing to directories. Maybe that can be used?

I don't see the advantage. I think it's confusing if I mix Mplayer video 
options with script logic (directories,..) in the Mplayer command line.


> > if [[ `ls -1 ${TEMPDIR}/FILE1/*ppm|wc -l` = `ls -1
>
> ${TEMPDIR}/FILE2/*ppm|wc -l` ]] then
>
> I'm not sure I like wc -l. What about tail -n 1?

wc -l theoretically catches all cases of differing frame numbers. I think 
it's a bit more general than only comparing the filenames of the last 
frames. But if you want to change, do are welcome to do so.

> >         pnmpsnr ../FILE1/$FILE $FILE 2> del.del
> >         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
> >         grep "Cb" del.del | dd bs=1c count=5 skip=29 of=del2.del
> > 2>/dev/null CB=`cat del2.del`
> >                echo -n "$CB;">>../psnr.dat
> >         grep "Cr" del.del | dd bs=1c count=5 skip=29 of=del2.del
> > 2>/dev/null CR=`cat del2.del`
> >                echo -n "$CR;">>../psnr.dat
>
> I don't like the usage of dd here. Can you be sure it's always 5
> characters? The above could be replaced by something like:
>
> YCBCR=`pnmpsnr ../FILE1/$FILE $FILE 2>&1 | tail -n 3 | cut -f 3 -d ':'
> | \ ( read Y X; read CB X; read CR X; echo "$Y;$CB;$CR;")`
> Y=`echo $YCBCR | cut -f 1 -d ';'`
> CB=`echo $YCBCR | cut -f 2 -d ';'`
> CR=`echo $YCBCR | cut -f 3 -d ';'`
> echo $YCBCR >>../psnr.dat

Done. Your version is much nicer!

> >          ALL=`echo
> > "(-10)*l((e(-$Y/10*l(10))+e(-$CB/10*l(10))/4+e(-$CR/10*l(10))/4)/1.5)
> >/l(1 0)"|bc -l` echo "$ALL">>../psnr.dat
>
> I don't have bc on my system, so this will fail. Can't this be done
> with expr (emulate fixed point arithmetic) or maybe a small C program?

I'm happy I managed to do the bc version. I think bc is quite common on 
linux distributions. I have added a check for bc and pnmpsnr.


> > if [[ `ls -1 ${TEMPDIR}/FILE1/*ppm|wc -l` = `ls -1
> > ${TEMPDIR}/FILE2/*ppm|wc -l` ]] then
> >         echo
> > else
> >         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
> > fi
>
> Code duplication.

Added a function that prints this warning.

> > +              Be aware that psnr-video.sh needs a lot of temporal
> > space +              inside /temp/.
>
> I'm no English grammar and writing guru myself, so I leave the docs to
> those who are, but temporal should be temporary imho.

You are right!

Thanks for your suggestions!

Matthias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: video-psnr.diff
Type: text/x-diff
Size: 5475 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-cvslog/attachments/20051002/a58082a7/attachment.diff>