merge video_decoder_impl.cc and ffmpeg_video_decoder.cc

merge video_decoder_impl.cc and ffmpeg_video_decoder.cc

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

[jiesun, 2010-06-15 01:01:02]

cut down video layers by merge video_decoder_impl.cc and ffmpeg_video_decoder.cc

[Alpha Left Google, 2010-06-15 18:43:47]

http://codereview.chromium.org/2854002/diff/1/2
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/2854002/diff/1/2#newcode155
media/filters/ffmpeg_video_decoder.cc:155: void
FFmpegVideoDecoder::OnDecodeComplete(scoped_refptr<VideoFrame> video_frame) {
nit: 80 chars

http://codereview.chromium.org/2854002/diff/1/3
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/2854002/diff/1/3#newcode5
media/filters/ffmpeg_video_decoder.h:5: #ifndef
MEDIA_FILTERS_VIDEO_DECODER_IMPL_H_
This file should be FFMPEG_VIDEO_DECODER_H_

http://codereview.chromium.org/2854002/diff/1/3#newcode121
media/filters/ffmpeg_video_decoder.h:121: #endif  //
MEDIA_FILTERS_VIDEO_DECODER_IMPL_H_
This needs to be corrected too.

[jiesun, 2010-06-15 20:25:04]

done.

http://codereview.chromium.org/2854002/diff/1/2
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/2854002/diff/1/2#newcode155
media/filters/ffmpeg_video_decoder.cc:155: void
FFmpegVideoDecoder::OnDecodeComplete(scoped_refptr<VideoFrame> video_frame) {
On 2010/06/15 18:43:47, Alpha wrote:
> nit: 80 chars

Done.

http://codereview.chromium.org/2854002/diff/1/3
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/2854002/diff/1/3#newcode5
media/filters/ffmpeg_video_decoder.h:5: #ifndef
MEDIA_FILTERS_VIDEO_DECODER_IMPL_H_
On 2010/06/15 18:43:47, Alpha wrote:
> This file should be FFMPEG_VIDEO_DECODER_H_

Done.

http://codereview.chromium.org/2854002/diff/1/3#newcode121
media/filters/ffmpeg_video_decoder.h:121: #endif  //
MEDIA_FILTERS_VIDEO_DECODER_IMPL_H_
On 2010/06/15 18:43:47, Alpha wrote:
> This needs to be corrected too.

Done.

[scherkus (not reviewing), 2010-06-15 21:26:59]

some nits

what's interesting is that FFmpegVideoDecoder doesn't really know that
VideoDecodeEngine is FFmpeg :)

http://codereview.chromium.org/2854002/diff/8001/9001
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/2854002/diff/8001/9001#newcode34
media/filters/ffmpeg_video_decoder.cc:34: bool* success,
indentation

http://codereview.chromium.org/2854002/diff/8001/9002
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/2854002/diff/8001/9002#newcode17
media/filters/ffmpeg_video_decoder.h:17: class FilterFactory;
these are in the media namespace, not sure we need these here

http://codereview.chromium.org/2854002/diff/8001/9002#newcode49
media/filters/ffmpeg_video_decoder.h:49: 
remove two extra blank lines

[jiesun, 2010-06-15 22:57:50]

please have another look.

http://codereview.chromium.org/2854002/diff/8001/9001
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/2854002/diff/8001/9001#newcode34
media/filters/ffmpeg_video_decoder.cc:34: bool* success,
On 2010/06/15 21:26:59, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/2854002/diff/8001/9002
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/2854002/diff/8001/9002#newcode17
media/filters/ffmpeg_video_decoder.h:17: class FilterFactory;
On 2010/06/15 21:26:59, scherkus wrote:
> these are in the media namespace, not sure we need these here

Done.

http://codereview.chromium.org/2854002/diff/8001/9002#newcode49
media/filters/ffmpeg_video_decoder.h:49: 
On 2010/06/15 21:26:59, scherkus wrote:
> remove two extra blank lines

Done.

[scherkus (not reviewing), 2010-06-16 01:50:00]

LGTM but please fix the few nits before committing

http://codereview.chromium.org/2854002/diff/1007/10002
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/2854002/diff/1007/10002#newcode178
media/filters/ffmpeg_video_decoder.cc:178: // TODO(jiesun): what
|DecodeBase::OnDecodeComplete| done is just
is this a TODO?  if we like what DecoderBase is doing then perhaps a TODO isn't
needed

http://codereview.chromium.org/2854002/diff/1007/10003
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/2854002/diff/1007/10003#newcode23
media/filters/ffmpeg_video_decoder.h:23: FFmpegVideoDecoder(VideoDecodeEngine*
engine);
add explicit keyword for single argument constructors

http://codereview.chromium.org/2854002/diff/1007/10003#newcode27
media/filters/ffmpeg_video_decoder.h:27: // CreateFactory() and
IsMediaFormatSupported().
this comment is completely out of date

you can remove it