Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(480033)

Issue 295: Added new feature to rd_collector.sh including h261, h263, and theora.

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by silverdev
Modified:
4 years, 6 months ago
Reviewers:
negge
Visibility:
Public.

Patch Set 1 : This diff is finally based off on unlords #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M tools/rd_collect.sh View 3 chunks +35 lines, -1 line 2 comments Download
M tools/rd_collect_daala.sh View 2 chunks +14 lines, -12 lines 0 comments Download
A tools/rd_collect_ffmpeg.sh View 1 chunk +42 lines, -0 lines 6 comments Download
M tools/rd_collect_libvpx.sh View 1 chunk +1 line, -1 line 0 comments Download
A tools/rd_collect_theora.sh View 1 chunk +35 lines, -0 lines 4 comments Download
M tools/rd_collect_x264.sh View 1 chunk +1 line, -1 line 0 comments Download
M tools/rd_collect_x265.sh View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 3
silverdev
4 years, 6 months ago #1
silverdev
The vp9 run is done graphs are built and that was a long merge considering. ...
4 years, 6 months ago #2
negge
4 years, 6 months ago #3
r- see my comments below

http://review.xiph.org/295/diff/436/tools/rd_collect.sh
File tools/rd_collect.sh (right):

http://review.xiph.org/295/diff/436/tools/rd_collect.sh#newcode176
tools/rd_collect.sh:176: if [ -x "/usr/bin/ffmpeg" ]; then
This will not work on platforms that install ffmpeg in other locations, e.g., on
a Mac

Maybe use FFMPEG=$(which ffmpeg) and then test that.

http://review.xiph.org/295/diff/436/tools/rd_collect.sh#newcode179
tools/rd_collect.sh:179: echo "FFMPEG not found FFMPEG=$ffMPEG/"
Many typo's on this line.

http://review.xiph.org/295/diff/436/tools/rd_collect_ffmpeg.sh
File tools/rd_collect_ffmpeg.sh (right):

http://review.xiph.org/295/diff/436/tools/rd_collect_ffmpeg.sh#newcode5
tools/rd_collect_ffmpeg.sh:5: echo "Please use: $(dirname $0)/rd_collect.sh
<h261|h263> *.y4m"
What about h264?

http://review.xiph.org/295/diff/436/tools/rd_collect_ffmpeg.sh#newcode18
tools/rd_collect_ffmpeg.sh:18: FRAMES=$(grep -c FRAME $FILE)
This is no longer the preferred way of counting the number of FRAME's in a y4m
file.  See rd_collect_daala.sh which parses the output of dump_psnr to count the
frames.

http://review.xiph.org/295/diff/436/tools/rd_collect_ffmpeg.sh#newcode21
tools/rd_collect_ffmpeg.sh:21: if [ "$CODEC" == "h261" ]; then
If you use a case statement here it is easy to add other ffmpeg codecs, e.g.,
flv, mjpeg

http://review.xiph.org/295/diff/436/tools/rd_collect_ffmpeg.sh#newcode22
tools/rd_collect_ffmpeg.sh:22: RANGE=$(seq 1 32)
Should this be $(seq 1 31) ?

http://review.xiph.org/295/diff/436/tools/rd_collect_ffmpeg.sh#newcode28
tools/rd_collect_ffmpeg.sh:28: if [ "$CODEC" != "h264" ]; then
Instead of having a separate encoding line for h264, add a QSTR inside the case
statement above where RANGE is set.

This is how the problem is solved in rd_collect_libvpx.sh

http://review.xiph.org/295/diff/436/tools/rd_collect_ffmpeg.sh#newcode29
tools/rd_collect_ffmpeg.sh:29: yes | ffmpeg -i $FILE -f $CODEC -qscale $x
$FULLNAME.mpg >/dev/null  2>$FULLNAME-$x-enc.error
Why is this prefixed with yes | ?

http://review.xiph.org/295/diff/436/tools/rd_collect_theora.sh
File tools/rd_collect_theora.sh (right):

http://review.xiph.org/295/diff/436/tools/rd_collect_theora.sh#newcode5
tools/rd_collect_theora.sh:5: echo "Please use: $(dirname $0)/rd_collect.sh
<vp8|vp9> *.y4m"
Wrong error message.

http://review.xiph.org/295/diff/436/tools/rd_collect_theora.sh#newcode14
tools/rd_collect_theora.sh:14: FULLNAME=$BASENAME.$CODEC
BASENAME is not used below this line, why not change it to

BASENAME=$(basename $FILE)-$CODEC and leave the rest of the lines untouched?

http://review.xiph.org/295/diff/436/tools/rd_collect_theora.sh#newcode19
tools/rd_collect_theora.sh:19: FRAMES=$(grep -c FRAME $FILE)
This has been deprecated in favor of counting the number of frames by parsing
the output of dump_psnr.

http://review.xiph.org/295/diff/436/tools/rd_collect_theora.sh#newcode22
tools/rd_collect_theora.sh:22: RANGE=$(seq 1 .1 10)
Theora computes their internal q value as:

  video_q=(int)rint(atof(optarg)*6.3);

Shouldn't this be

  RANGE=$(seq 0 0.15873015873015 10)
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld