Make VideoDecoder use (kAborted, NULL) to signify an aborted decode, instead of (kOk, NULL).

Make VideoDecoder use (kAborted, NULL) to signify an aborted decode, instead of (kOk, NULL).

This makes it match its sibling, AudioDecoder.

BUG=329379
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243163

[rileya (GONE FROM CHROMIUM), 2014-01-03 00:06:32]

This is a small first step towards making the decoder interfaces match up
better.

[DaleCurtis, 2014-01-03 01:01:10]

lgtm

[xhwang, 2014-01-03 01:05:57]

Thanks for taking this!

In GpuVideoDecoder, we return EOS frame when reset() is called, which should be
fixed as well (not necessarily in this CL though):

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_...

https://codereview.chromium.org/123213006/diff/30001/media/base/video_decoder.h
File media/base/video_decoder.h (right):

https://codereview.chromium.org/123213006/diff/30001/media/base/video_decoder...
media/base/video_decoder.h:25: kAborted,  // Decode was aborted.
Can we have something like:

Decode was aborted as a result of Reset() being called.

I think it's cleaner to mention Reset() here somehow.

https://codereview.chromium.org/123213006/diff/30001/media/base/video_decoder...
media/base/video_decoder.h:56: // - NULL frame indicates an aborted decode. This
can happen if Reset() or
please update this as well

https://codereview.chromium.org/123213006/diff/30001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

https://codereview.chromium.org/123213006/diff/30001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder_unittest.cc:152: case VideoDecoder::kAborted:
NOTREACHED()? The check on l.142 means that we should not see kAborted in this
test.

https://codereview.chromium.org/123213006/diff/30001/media/filters/video_fram...
File media/filters/video_frame_stream.cc (right):

https://codereview.chromium.org/123213006/diff/30001/media/filters/video_fram...
media/filters/video_frame_stream.cc:248: 
DCHECK_EQ(status == VideoDecoder::kOk, frame != NULL);

then we can drop the DCHECK(!frame.get()) in various places in this function.

Since this function (callback) is used by ALL video decoders, this check also
helps you make sure you updated all video decoders.

https://codereview.chromium.org/123213006/diff/30001/media/filters/video_fram...
media/filters/video_frame_stream.cc:269: }
VideoDecoder::kAborted can only be a result of a previous VideoDecoder::Reset()
call, which is handled on l.280. This is why we didn't check for (kOk, NULL) in
the original impl. Sorry for not making this obvious :( Feel free to
refactor/clean-up/comment the code to make it cleaner.

Note that kAborted -> !reset_cb_.is_null() but not vice versa. For example, we
could have:
- Decode()
- Decode finished and OnFrameReady posted.
- Reset()
- OnFrameReady(kOk, valid_frame)  // !reset_cb_.is_null()
- Reset callback fired..

[rileya (GONE FROM CHROMIUM), 2014-01-03 20:33:37]

Alright, I addressed most everything I think. I'll save the GpuVideoDecoder fix
for another CL.

[xhwang, 2014-01-03 22:28:20]

lgtm, thanks!

[commit-bot: I haz the power, 2014-01-06 18:24:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/123213006/250001

[commit-bot: I haz the power, 2014-01-06 20:57:43]

Change committed as 243163