mediabench change to decode one extra frame, flushing ffmpeg-mt for the last ...

mediabench change to decode one extra frame, flushing ffmpeg-mt for the last frame.
Allows single frame to be benchmarked with --video-threads=4


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=17385

[fbarchard, 2009-05-27 04:10:19]

This change allows ffmpeg-mt to be benchmarked accurately, by flushing the last
frame.

[scherkus (not reviewing), 2009-05-27 21:26:17]

http://codereview.chromium.org/113888/diff/1/2
File media/bench/bench.cc (right):

http://codereview.chromium.org/113888/diff/1/2#newcode10
Line 10: // This tool requires ffmpeg DLL's built with --enable-protocol=file
nits: ffmpeg -> FFmpeg, period at end

http://codereview.chromium.org/113888/diff/1/2#newcode187
Line 187: int amt = 0;
nit: |amt| isn't a very accurate name since av_read_frame() is actually
returning an error code.  How about |result|?

http://codereview.chromium.org/113888/diff/1/2#newcode189
Line 189: amt = av_read_frame(format_context, &packet);
should check that amt is either >= 0 or equals AVERROR_EOF, perhaps add a check
for:
if (amt < 0 && amt != AVERROR_EOF) {
  // demux error message
  return 1;
}

http://codereview.chromium.org/113888/diff/1/2#newcode191
Line 191: if (packet.stream_index == target_stream) {
I don't the flushing logic is completely correct.

if we have multiple streams only it's possible the EOF packet won't have a
stream_index that matches target_stream

since this program only benchmarks a single stream, you could probably do
something like:

// Forcefully flush our decoder.
if (amt == AVERROR_EOF) {
  DCHECK(!packet.data);
  DCHECK(!packet.size);
  packet.stream_index = target_stream;
}

http://codereview.chromium.org/113888/diff/1/2#newcode220
Line 220: 
could probably ditch the extra whitespace

http://codereview.chromium.org/113888/diff/1/2#newcode223
Line 223: // -1 because we call decode one extra time to flush
Period at end of comment.  Also try to explain "why" rather than "what" the code
is doing, so "Account for flushing the decoder."

http://codereview.chromium.org/113888/diff/1/2#newcode245
Line 245: // Print our results.
Could you unindent this line by two spaces?

[fbarchard, 2009-05-28 00:03:12]

done

http://codereview.chromium.org/113888/diff/1/2
File media/bench/bench.cc (right):

http://codereview.chromium.org/113888/diff/1/2#newcode10
Line 10: // This tool requires ffmpeg DLL's built with --enable-protocol=file
On 2009/05/27 21:26:17, scherkus wrote:
> nits: ffmpeg -> FFmpeg, period at end
Done, but note that I left the . out because the comment is about a literal
command, which does not want a . in it.

http://codereview.chromium.org/113888/diff/1/2#newcode187
Line 187: int amt = 0;
I assumed it was a read amount.  I'll call it result, so it works either way.

http://codereview.chromium.org/113888/diff/1/2#newcode189
Line 189: amt = av_read_frame(format_context, &packet);
I'm purposely decoding one extra frame, when I get the -1

http://codereview.chromium.org/113888/diff/1/2#newcode220
Line 220: 
On 2009/05/27 21:26:17, scherkus wrote:
> could probably ditch the extra whitespace
I'm not sure which extra whitespace you mean, but I've removed the blank lines
nearby.

http://codereview.chromium.org/113888/diff/1/2#newcode223
Line 223: // -1 because we call decode one extra time to flush
On 2009/05/27 21:26:17, scherkus wrote:
> Period at end of comment.  Also try to explain "why" rather than "what" the
code
> is doing, so "Account for flushing the decoder."

Done.

http://codereview.chromium.org/113888/diff/1/2#newcode245
Line 245: // Print our results.
On 2009/05/27 21:26:17, scherkus wrote:
> Could you unindent this line by two spaces?

Done.

[scherkus (not reviewing), 2009-05-28 00:42:26]

http://codereview.chromium.org/113888/diff/1/2
File media/bench/bench.cc (right):

http://codereview.chromium.org/113888/diff/1/2#newcode10
Line 10: // This tool requires ffmpeg DLL's built with --enable-protocol=file
On 2009/05/28 00:03:12, fbarchard wrote:
> On 2009/05/27 21:26:17, scherkus wrote:
> > nits: ffmpeg -> FFmpeg, period at end
> Done, but note that I left the . out because the comment is about a literal
> command, which does not want a . in it.
> 

Cool -- but it looks like you left the period in

http://codereview.chromium.org/113888/diff/1/2#newcode189
Line 189: amt = av_read_frame(format_context, &packet);
On 2009/05/28 00:03:12, fbarchard wrote:
> I'm purposely decoding one extra frame, when I get the -1
> 

I still don't see why we wouldn't want to distinguish between demuxing errors
and an actual end of stream condition

http://codereview.chromium.org/113888/diff/1/2#newcode191
Line 191: if (packet.stream_index == target_stream) {
On 2009/05/27 21:26:17, scherkus wrote:
> I don't the flushing logic is completely correct.
> 
> if we have multiple streams only it's possible the EOF packet won't have a
> stream_index that matches target_stream
> 
> since this program only benchmarks a single stream, you could probably do
> something like:
> 
> // Forcefully flush our decoder.
> if (amt == AVERROR_EOF) {
>   DCHECK(!packet.data);
>   DCHECK(!packet.size);
>   packet.stream_index = target_stream;
> }
> 

What about this?

http://codereview.chromium.org/113888/diff/1003/5#newcode221
Line 221: base::TimeDelta total = base::TimeTicks::HighResNow() - start;
have you testing vanilla vs. mt?  according to docs you always need to flush the
decoders regardless of ffmpeg flavour (refer to ffmpeg.c or ffplay.c for
examples)

[fbarchard, 2009-05-28 01:22:34]

uploaded fixes.
even on error, the last packet should be decoded, allowing  any decoding that
was buffered to be returned.
My initial implementation did this as an extra unconditional decode outside the
loop.
This implementation does it by unconditionally doing the decode inside the loop,
and then testing, via do/while, if the loop should be exited.

yes, flushing is generally needed for ffmpeg.  In practice, its not needed for
h264 mainline, but it might be for other formats.  I've also noticed that x264
compressed videos do not produce an extra frame, but ffmpeg encoded/muxed videos
will decode with an extra frame, which is likely a flushed frame at the end.
ffmpeg-mt does buffering, almost guaranteeing the need for flushing.

[scherkus (not reviewing), 2009-05-28 01:24:39]

fyi: not seeing the uploaded changes

[fbarchard, 2009-05-29 23:00:53]

now its perfect :-)

[scherkus (not reviewing), 2009-06-02 01:39:20]

really tiny nits

http://codereview.chromium.org/113888/diff/1012/13
File media/bench/bench.cc (right):

http://codereview.chromium.org/113888/diff/1012/13#newcode208
Line 208: int got_picture = 0;
any reason why got_picture was moved out here?  it looks like you're still only
using it within the original scope

http://codereview.chromium.org/113888/diff/1012/13#newcode252
Line 252: if (frames == 0) {
if no frames were output does it make much more sense to continue or would an
error message be more appropriate?

[fbarchard, 2009-06-02 02:47:20]

fixed.  Also added support for unknown codecs (OGG Kate subtitle tracks)

http://codereview.chromium.org/113888/diff/1012/13
File media/bench/bench.cc (right):

http://codereview.chromium.org/113888/diff/1012/13#newcode208
Line 208: int got_picture = 0;
oops.  It was my intent to share the same code/data with audio, but I dropped
the idea.  fixed

http://codereview.chromium.org/113888/diff/1012/13#newcode252
Line 252: if (frames == 0) {
I think of zero frames as an edge case, but not an error.
My first attempt was to avoid the divide by zero.
I've moved the code around to now show zero frames, and elapsed time, but defer
the average and standard deviation calcs.

[scherkus (not reviewing), 2009-06-02 03:47:59]

http://codereview.chromium.org/113888/diff/1013/1014
File media/bench/bench.cc (right):

http://codereview.chromium.org/113888/diff/1013/1014#newcode136
Line 136: if (codec_context->codec_type < 0) {
given codec_type is an enum, you should compare to CODEC_TYPE_UNKNOWN

http://codereview.chromium.org/113888/diff/1013/1014#newcode267
Line 267: if (frames >= 0) {
shouldn't this be "frames > 0"? otherwise we get a division by zero on the next
line

http://codereview.chromium.org/113888/diff/1013/1014#newcode274
Line 274: 
nit: get rid of blank line (comments are generally right next to the code they
describe, with a blank line before them EXCEPT when it's the start of a new
block)

[fbarchard, 2009-06-02 03:55:06]

Done.

http://codereview.chromium.org/113888/diff/1013/1014
File media/bench/bench.cc (right):

http://codereview.chromium.org/113888/diff/1013/1014#newcode136
Line 136: if (codec_context->codec_type < 0) {
On 2009/06/02 03:47:59, scherkus wrote:
> given codec_type is an enum, you should compare to CODEC_TYPE_UNKNOWN

Done.

http://codereview.chromium.org/113888/diff/1013/1014#newcode267
Line 267: if (frames >= 0) {
On 2009/06/02 03:47:59, scherkus wrote:
> shouldn't this be "frames > 0"? otherwise we get a division by zero on the
next
> line

Done.

http://codereview.chromium.org/113888/diff/1013/1014#newcode274
Line 274: 
On 2009/06/02 03:47:59, scherkus wrote:
> nit: get rid of blank line (comments are generally right next to the code they
> describe, with a blank line before them EXCEPT when it's the start of a new
> block)

Done.

[scherkus (not reviewing), 2009-06-02 04:04:37]

LGTM -- I'm not sure if this is being compiled on mac/linux at the moment, but
you may have a gcc incompatibility

http://codereview.chromium.org/113888/diff/1017/17
File media/bench/bench.cc (right):

http://codereview.chromium.org/113888/diff/1017/17#newcode267
Line 267: if (frames > 0) {
heads up: you may need to declare this as "0u" for gcc compatability (frames is
a size_t)