remove AVFrame Dependency

Remove AVFrame dependency.

Currently AVFrame is used in both OmxVideoDecodeEngine and FFmpegVideoDecodeEngine.  This causes an unnecessary memory copy from AVFrame to VideoFrame.  We can remove the additional copy by using VideoFrame.

Also in the hardware (OpenMAX) path, VideoFrames are allocated inside decode engines, therefore the interface was changed such that VideoFrames are created inside the decode engines but buffered in VideoDecoderImpl.

BUGS=none
TEST=none

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

[jiesun, 2010-04-17 01:04:05]

add some comments

http://codereview.chromium.org/1669002/diff/2001/3002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3002#newcode84
media/filters/ffmpeg_video_decode_engine.cc:84: // TODO(jiesun): This is error
case, but handled as normal case.
how to signal?

http://codereview.chromium.org/1669002/diff/2001/3004
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3004#newcode96
media/filters/omx_video_decode_engine.cc:96: Task* done_cb) {
should we pass a structure like DecodeRequest?

http://codereview.chromium.org/1669002/diff/2001/3004#newcode211
media/filters/omx_video_decode_engine.cc:211: request.frame->data[1] =
u_buffer_.get();
remove [y|u|v]_buffer

[jiesun, 2010-04-19 17:12:23]

some comments

http://codereview.chromium.org/1669002/diff/2001/3002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3002#newcode117
media/filters/ffmpeg_video_decode_engine.cc:117: return;
this is also a error case , return as normal

http://codereview.chromium.org/1669002/diff/2001/3002#newcode125
media/filters/ffmpeg_video_decode_engine.cc:125: *got_frame = false;
return error here.

http://codereview.chromium.org/1669002/diff/2001/3004
File media/filters/omx_video_decode_engine.cc (left):

http://codereview.chromium.org/1669002/diff/2001/3004#oldcode225
media/filters/omx_video_decode_engine.cc:225: 
where is this done_cb get deleted?

[wjia(left Chromium), 2010-04-19 17:30:35]

http://codereview.chromium.org/1669002/diff/2001/3002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3002#newcode43
media/filters/ffmpeg_video_decode_engine.cc:43: 
are width and height available at this stage?

http://codereview.chromium.org/1669002/diff/2001/3004
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3004#newcode201
media/filters/omx_video_decode_engine.cc:201: // |frame| carries the decoded
frame.
extra same comment here?

http://codereview.chromium.org/1669002/diff/2001/3004#newcode234
media/filters/omx_video_decode_engine.cc:234: 
Hopefully, this memcpy() path will be removed after EGLImage support is added.

[Alpha Left Google, 2010-04-19 19:27:02]

http://codereview.chromium.org/1669002/diff/2001/3001
File media/base/buffers.h (right):

http://codereview.chromium.org/1669002/diff/2001/3001#newcode77
media/base/buffers.h:77: void SetRepeatCount(int repeat_count) {
Don't think this is needed as this is not used anywhere.

http://codereview.chromium.org/1669002/diff/2001/3002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3002#newcode40
media/filters/ffmpeg_video_decode_engine.cc:40: 
Should probably move the width and height to after avcodec_open().

http://codereview.chromium.org/1669002/diff/2001/3002#newcode82
media/filters/ffmpeg_video_decode_engine.cc:82: scoped_ptr_malloc<AVFrame,
ScopedPtrAVFree> av_frame(avcodec_alloc_frame());
Can we use the same av_frame for all decodes? Say putting it to a member
variable?

http://codereview.chromium.org/1669002/diff/2001/3002#newcode122
media/filters/ffmpeg_video_decode_engine.cc:122:
VideoFrame::CreateFrame(GetSurfaceFormat(), width_, height_,
If you changed the method declaration to:
DecodeFrame(..., scoped_refptr<VideoFrame> *video_frame, ...)

This line will be:
VideoFrame::CreateFrame(GetSurfaceFormat(), width_, height_,
                        zero, zero, video_frame);

http://codereview.chromium.org/1669002/diff/2001/3002#newcode134
media/filters/ffmpeg_video_decode_engine.cc:134: CopyPlane(VideoFrame::kYPlane,
output_frame.get(), av_frame.get());
Here you are copying data from |av_frame| to |output_frame|. But this should be
written to |video_frame| instead, I don't see that code anywhere, I think this
is copying to a wrong place.

http://codereview.chromium.org/1669002/diff/2001/3002#newcode139
media/filters/ffmpeg_video_decode_engine.cc:139:
output_frame->SetRepeatCount(av_frame->repeat_pict);
I don't think this repeat_pict is ever used, and you have the DCHECK too, do we
really need that?

http://codereview.chromium.org/1669002/diff/2001/3003
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/2001/3003#newcode27
media/filters/ffmpeg_video_decode_engine.h:27: virtual void DecodeFrame(const
Buffer& buffer, VideoFrame** video_frame,
This should be scoped_refptr<VideoFrame> *video_frame, so |video_frame| is a
pointer to scoped_refptr<VideoFrame>.

http://codereview.chromium.org/1669002/diff/2001/3004
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3004#newcode94
media/filters/omx_video_decode_engine.cc:94: VideoFrame** video_frame,
Should be scoped_refptr<VideoFrame> *video_frame.

http://codereview.chromium.org/1669002/diff/2001/3004#newcode110
media/filters/omx_video_decode_engine.cc:110: input_buffer = new
OmxInputBuffer(data, buffer.GetDataSize());
I think you need to sync and merge, wjia@ changed this code.

http://codereview.chromium.org/1669002/diff/2001/3004#newcode207
media/filters/omx_video_decode_engine.cc:207: // TODO(ajwong): This leaks
memory. Fix by not using AVFrame.
Remove this comment if it doesn't apply.

http://codereview.chromium.org/1669002/diff/2001/3004#newcode209
media/filters/omx_video_decode_engine.cc:209: #if 0
If this code is not used then remove it.

http://codereview.chromium.org/1669002/diff/2001/3004#newcode217
media/filters/omx_video_decode_engine.cc:217: scoped_refptr<VideoFrame>
video_frame;
This is incorrect, |video_frame| is a scoped_refptr, after
VideoFrame::CrateFrame, it will have refcount 1, the pointer value is assigned
to |*request.frame| but that doesn't increase the refcount, so when
|video_frame| is out of scope, the memory will be deleted, and |*request.frame|
will hold an invalid reference.

http://codereview.chromium.org/1669002/diff/2001/3005
File media/filters/omx_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/2001/3005#newcode37
media/filters/omx_video_decode_engine.h:37: virtual void DecodeFrame(const
Buffer& buffer, VideoFrame** yuv_frame,
Should be scoped_refptr<VideoFrame> *yuv_frame.

http://codereview.chromium.org/1669002/diff/2001/3005#newcode86
media/filters/omx_video_decode_engine.h:86: DecodeRequest(VideoFrame** f, bool*
b, Task* cb)
Should be scoped_refptr<VideoFrame> *f.

http://codereview.chromium.org/1669002/diff/2001/3005#newcode92
media/filters/omx_video_decode_engine.h:92: VideoFrame** frame;
Should be scoped_refptr<VideoFrame> *frame.

http://codereview.chromium.org/1669002/diff/2001/3006
File media/filters/video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/2001/3006#newcode43
media/filters/video_decode_engine.h:43: virtual void DecodeFrame(const Buffer&
buffer, VideoFrame** video_frame,
Should be scoped_refptr<VideoFrame> *video_frame.

[Alpha Left Google, 2010-04-20 18:43:24]

http://codereview.chromium.org/1669002/diff/12001/13002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/12001/13002#newcode41
media/filters/ffmpeg_video_decode_engine.cc:41: width_ = stream->codec->width;
Sometimes this is not know until you decoded the first frame, so may move this
to DecodeFrame()?

http://codereview.chromium.org/1669002/diff/12001/13002#newcode81
media/filters/ffmpeg_video_decode_engine.cc:81: done_cb->Run(false, false,
video_frame, done_cb_context);
Why do you need to call DecodeFrameCallback? I think Task should be able to
accomplish what you need to do..

You need to assign the output parameters and make the callback.

http://codereview.chromium.org/1669002/diff/12001/13003
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/12001/13003#newcode28
media/filters/ffmpeg_video_decode_engine.h:28: DecodeFrameCallback* done_cb,
Indentation?

Also why is DecodeFrameCallback needed here? Can Task do what you need already?

http://codereview.chromium.org/1669002/diff/12001/13004
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/12001/13004#newcode121
media/filters/omx_video_decode_engine.cc:121:
decode_request_queue_.push_back(DecodeRequest(done_cb, done_cb_context));
Can you save the parameter as a VideoFrame instead of DecodeFrameCallback?

http://codereview.chromium.org/1669002/diff/12001/13004#newcode227
media/filters/omx_video_decode_engine.cc:227: request.done_cb_->Run(true,
got_result, video_frame,
If you have saved the parameter as VideoFrame, you can assign it's value here
and call the Task.

http://codereview.chromium.org/1669002/diff/12001/13005
File media/filters/omx_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/12001/13005#newcode38
media/filters/omx_video_decode_engine.h:38: DecodeFrameCallback* done_cb,
I still don't understand why you need an additional callback.

[jiesun, 2010-04-21 21:26:37]

passed build-bots

http://codereview.chromium.org/1669002/diff/2001/3002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3002#newcode40
media/filters/ffmpeg_video_decode_engine.cc:40: 
On 2010/04/19 19:27:02, Alpha wrote:
> Should probably move the width and height to after avcodec_open().

Done.

http://codereview.chromium.org/1669002/diff/2001/3002#newcode43
media/filters/ffmpeg_video_decode_engine.cc:43: 
On 2010/04/19 17:30:35, wjia wrote:
> are width and height available at this stage?

Done.

http://codereview.chromium.org/1669002/diff/2001/3004
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/2001/3004#newcode110
media/filters/omx_video_decode_engine.cc:110: input_buffer = new
OmxInputBuffer(data, buffer.GetDataSize());
On 2010/04/19 19:27:02, Alpha wrote:
> I think you need to sync and merge, wjia@ changed this code.
> 

Done.

http://codereview.chromium.org/1669002/diff/2001/3004#newcode201
media/filters/omx_video_decode_engine.cc:201: // |frame| carries the decoded
frame.
On 2010/04/19 17:30:35, wjia wrote:
> extra same comment here?

Done.

http://codereview.chromium.org/1669002/diff/2001/3005
File media/filters/omx_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/2001/3005#newcode37
media/filters/omx_video_decode_engine.h:37: virtual void DecodeFrame(const
Buffer& buffer, VideoFrame** yuv_frame,
On 2010/04/19 19:27:02, Alpha wrote:
> Should be scoped_refptr<VideoFrame> *yuv_frame.
> 

Done.

http://codereview.chromium.org/1669002/diff/2001/3005#newcode86
media/filters/omx_video_decode_engine.h:86: DecodeRequest(VideoFrame** f, bool*
b, Task* cb)
On 2010/04/19 19:27:02, Alpha wrote:
> Should be scoped_refptr<VideoFrame> *f.
> 

Done.

http://codereview.chromium.org/1669002/diff/2001/3005#newcode92
media/filters/omx_video_decode_engine.h:92: VideoFrame** frame;
On 2010/04/19 19:27:02, Alpha wrote:
> Should be scoped_refptr<VideoFrame> *frame.
> 

Done.

http://codereview.chromium.org/1669002/diff/2001/3006
File media/filters/video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/2001/3006#newcode43
media/filters/video_decode_engine.h:43: virtual void DecodeFrame(const Buffer&
buffer, VideoFrame** video_frame,
On 2010/04/19 19:27:02, Alpha wrote:
> Should be scoped_refptr<VideoFrame> *video_frame.
> 

Done.

[Alpha Left Google, 2010-04-21 22:45:50]

Thanks! Nice work!

This is some extra requests (since this is a big change..):

1. Please run valgrind against media_unittests, see this page for more details:
http://www.chromium.org/developers/how-tos/using-valgrind

I see memory leak in the test, just to be safe.

2. Please make sure you can build this and run through a video on the desktop.
This is a good site for testing:
http://fbarchard-kir.ad.corp.google.com/testmatrix/index.html

http://codereview.chromium.org/1669002/diff/39001/40002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/39001/40002#newcode67
media/filters/ffmpeg_video_decode_engine.cc:67: scoped_refptr<VideoFrame>*
video_frame,
nit: 80 chars

http://codereview.chromium.org/1669002/diff/39001/40002#newcode117
media/filters/ffmpeg_video_decode_engine.cc:117:
VideoFrame::CreateFrame(GetSurfaceFormat(),
Please add DCHECKS to make sure width and height are non null and they are not
too large, see media/base/limits.h

http://codereview.chromium.org/1669002/diff/39001/40003
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/39001/40003#newcode10
media/filters/ffmpeg_video_decode_engine.h:10: #include
"media/ffmpeg/ffmpeg_common.h"
This header is not needed.

http://codereview.chromium.org/1669002/diff/39001/40003#newcode46
media/filters/ffmpeg_video_decode_engine.h:46: scoped_ptr_malloc<AVFrame,
ScopedPtrAVFree> av_frame;
nit: av_frame_

http://codereview.chromium.org/1669002/diff/39001/40004
File media/filters/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/1669002/diff/39001/40004#newcode30
media/filters/ffmpeg_video_decode_engine_unittest.cc:30: frame_buffer_.reset(new
uint8[kWidth*kHeight]);
nit: indentation.
nit: kWidth * kHeight

http://codereview.chromium.org/1669002/diff/39001/40004#newcode32
media/filters/ffmpeg_video_decode_engine_unittest.cc:32: // DecodeFrame will
check there pointer as non-NULL value
nit: s/there pointer/these pointers/

http://codereview.chromium.org/1669002/diff/39001/40004#newcode36
media/filters/ffmpeg_video_decode_engine_unittest.cc:36: yuv_frame_.linesize[1]
= yuv_frame_.linesize[2] = kWidth>>1;
nit: kWidth >> 1;

http://codereview.chromium.org/1669002/diff/39001/40004#newcode58
media/filters/ffmpeg_video_decode_engine_unittest.cc:58: void
Initialize_Normal() {
nit: maybe just call it Initialize()?

http://codereview.chromium.org/1669002/diff/39001/40004#newcode77
media/filters/ffmpeg_video_decode_engine_unittest.cc:77: scoped_ptr<uint8_t>
frame_buffer_;
scoped_array or you have memory leak.

Better run valgrind against the media unittests to make sure.

http://codereview.chromium.org/1669002/diff/39001/40005
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/39001/40005#newcode178
media/filters/omx_video_decode_engine.cc:178: MergeBytesFrameQueue(buffer,
size);
We can eliminate the memory copy occured here in the next patch since we can
safely assume all frames received are full frames.

http://codereview.chromium.org/1669002/diff/39001/40005#newcode214
media/filters/omx_video_decode_engine.cc:214:
memcpy((*request.frame)->data(VideoFrame::kYPlane), frame->data, pixels);
nit: Add a comment here that you assumes color space format.

http://codereview.chromium.org/1669002/diff/39001/40008
File media/filters/video_decoder_impl.cc (right):

http://codereview.chromium.org/1669002/diff/39001/40008#newcode160
media/filters/video_decoder_impl.cc:160: scoped_ptr<scoped_refptr<VideoFrame> >
video_frame_deleter(video_frame);
This is tricky! :)

http://codereview.chromium.org/1669002/diff/39001/40008#newcode184
media/filters/video_decoder_impl.cc:184: EnqueueVideoFrame(*video_frame);
Should call EnqueueResult() directly.

http://codereview.chromium.org/1669002/diff/39001/40008#newcode195
media/filters/video_decoder_impl.cc:195: void
VideoDecoderImpl::EnqueueVideoFrame(
This method is pretty empty, should remove it and call EnqueueResult directly.

http://codereview.chromium.org/1669002/diff/39001/40009
File media/filters/video_decoder_impl.h (right):

http://codereview.chromium.org/1669002/diff/39001/40009#newcode63
media/filters/video_decoder_impl.h:63: virtual void
EnqueueVideoFrame(scoped_refptr<VideoFrame> video_frame);
maybe const scoped_refptr<VideoFrame>&?

[jiesun, 2010-04-22 18:29:28]

Tested with Frank's VideoStack Test Matrix.
Tested media_unittests with Valgrind with 
sh tools/valgrind/valgrind.sh --leak-check=full out/Debug/media_unittests
find some irrelevant "possible" leak.

==25090== 
==25090== HEAP SUMMARY:
==25090==     in use at exit: 127,672 bytes in 2,081 blocks
==25090==   total heap usage: 109,456 allocs, 107,375 frees, 48,963,983 bytes
allocated
==25090== 
==25090== LEAK SUMMARY:
==25090==    definitely lost: 0 bytes in 0 blocks
==25090==    indirectly lost: 0 bytes in 0 blocks
==25090==      possibly lost: 866 bytes in 21 blocks
==25090==    still reachable: 9,830 bytes in 298 blocks
==25090==         suppressed: 116,976 bytes in 1,762 blocks
==25090== Rerun with --leak-check=full to see details of leaked memory
==25090== 
==25090== For counts of detected and suppressed errors, rerun with: -v
==25090== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 127 from 11)

http://codereview.chromium.org/1669002/diff/39001/40002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/39001/40002#newcode67
media/filters/ffmpeg_video_decode_engine.cc:67: scoped_refptr<VideoFrame>*
video_frame,
On 2010/04/21 22:45:50, Alpha wrote:
> nit: 80 chars
> 

Done.

http://codereview.chromium.org/1669002/diff/39001/40002#newcode117
media/filters/ffmpeg_video_decode_engine.cc:117:
VideoFrame::CreateFrame(GetSurfaceFormat(),
On 2010/04/21 22:45:50, Alpha wrote:
> Please add DCHECKS to make sure width and height are non null and they are not
> too large, see media/base/limits.h
> 

Done.

http://codereview.chromium.org/1669002/diff/39001/40003
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/39001/40003#newcode10
media/filters/ffmpeg_video_decode_engine.h:10: #include
"media/ffmpeg/ffmpeg_common.h"
On 2010/04/21 22:45:50, Alpha wrote:
> This header is not needed.
for ScopedPtrAVFree().

http://codereview.chromium.org/1669002/diff/39001/40003#newcode46
media/filters/ffmpeg_video_decode_engine.h:46: scoped_ptr_malloc<AVFrame,
ScopedPtrAVFree> av_frame;
On 2010/04/21 22:45:50, Alpha wrote:
> nit: av_frame_

Done.

http://codereview.chromium.org/1669002/diff/39001/40004
File media/filters/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/1669002/diff/39001/40004#newcode30
media/filters/ffmpeg_video_decode_engine_unittest.cc:30: frame_buffer_.reset(new
uint8[kWidth*kHeight]);
On 2010/04/21 22:45:50, Alpha wrote:
> nit: indentation.
> nit: kWidth * kHeight
> 

Done.

http://codereview.chromium.org/1669002/diff/39001/40004#newcode32
media/filters/ffmpeg_video_decode_engine_unittest.cc:32: // DecodeFrame will
check there pointer as non-NULL value
On 2010/04/21 22:45:50, Alpha wrote:
> nit: s/there pointer/these pointers/
> 

Done.

http://codereview.chromium.org/1669002/diff/39001/40004#newcode36
media/filters/ffmpeg_video_decode_engine_unittest.cc:36: yuv_frame_.linesize[1]
= yuv_frame_.linesize[2] = kWidth>>1;
On 2010/04/21 22:45:50, Alpha wrote:
> nit: kWidth >> 1;
> 

Done.

http://codereview.chromium.org/1669002/diff/39001/40004#newcode58
media/filters/ffmpeg_video_decode_engine_unittest.cc:58: void
Initialize_Normal() {
On 2010/04/21 22:45:50, Alpha wrote:
> nit: maybe just call it Initialize()?
> 

Done.

http://codereview.chromium.org/1669002/diff/39001/40004#newcode77
media/filters/ffmpeg_video_decode_engine_unittest.cc:77: scoped_ptr<uint8_t>
frame_buffer_;
On 2010/04/21 22:45:50, Alpha wrote:
> scoped_array or you have memory leak.
> 
> Better run valgrind against the media unittests to make sure.

Done.

http://codereview.chromium.org/1669002/diff/39001/40005
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/39001/40005#newcode178
media/filters/omx_video_decode_engine.cc:178: MergeBytesFrameQueue(buffer,
size);
On 2010/04/21 22:45:50, Alpha wrote:
> We can eliminate the memory copy occured here in the next patch since we can
> safely assume all frames received are full frames.
> 
true, will do!

http://codereview.chromium.org/1669002/diff/39001/40005#newcode214
media/filters/omx_video_decode_engine.cc:214:
memcpy((*request.frame)->data(VideoFrame::kYPlane), frame->data, pixels);
On 2010/04/21 22:45:50, Alpha wrote:
> nit: Add a comment here that you assumes color space format.
> 

Done.

http://codereview.chromium.org/1669002/diff/39001/40008
File media/filters/video_decoder_impl.cc (right):

http://codereview.chromium.org/1669002/diff/39001/40008#newcode160
media/filters/video_decoder_impl.cc:160: scoped_ptr<scoped_refptr<VideoFrame> >
video_frame_deleter(video_frame);
On 2010/04/21 22:45:50, Alpha wrote:
> This is tricky! :)
Now you know why I had hard feeling about this.

http://codereview.chromium.org/1669002/diff/39001/40008#newcode184
media/filters/video_decoder_impl.cc:184: EnqueueVideoFrame(*video_frame);
On 2010/04/21 22:45:50, Alpha wrote:
> Should call EnqueueResult() directly.

right, but this is kind of convenient for unittests.
gMock had issues with EncodeResult template.

http://codereview.chromium.org/1669002/diff/39001/40009
File media/filters/video_decoder_impl.h (right):

http://codereview.chromium.org/1669002/diff/39001/40009#newcode63
media/filters/video_decoder_impl.h:63: virtual void
EnqueueVideoFrame(scoped_refptr<VideoFrame> video_frame);
On 2010/04/21 22:45:50, Alpha wrote:
> maybe const scoped_refptr<VideoFrame>&?
> 

Done.

[Alpha Left Google, 2010-04-22 18:36:30]

It looks pretty good now.

One last thing, how do you solve the problem that
VideoDecoderImpl::OnDecodeComplete is called from arbitrary thread?

[jiesun, 2010-04-22 19:59:51]

Andrew: alpha said you are working on the same set of files. so maybe you want
to comment.

[scherkus (not reviewing), 2010-04-22 20:47:14]

please write a more descriptive changelist description, including BUG= and TEST=
lines

http://codereview.chromium.org/1669002/diff/52001/53001
File media/base/buffers.h (right):

http://codereview.chromium.org/1669002/diff/52001/53001#newcode62
media/base/buffers.h:62: int GetRepeatCount() const {
can this go in VideoFrame?

http://codereview.chromium.org/1669002/diff/52001/53002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/52001/53002#newcode83
media/filters/ffmpeg_video_decode_engine.cc:83: // We don't allocate AVFrame on
the stack since different versions of FFmpeg
this comment is out of place, should go up by avcodec_alloc_frame()

http://codereview.chromium.org/1669002/diff/52001/53002#newcode88
media/filters/ffmpeg_video_decode_engine.cc:88:
avcodec_decode_video2(codec_context_,
no need to drop all this code onto the next line

http://codereview.chromium.org/1669002/diff/52001/53002#newcode118
media/filters/ffmpeg_video_decode_engine.cc:118: base::TimeDelta zero;
zero is a valid timestamp

please use StreamSample::kInvalidTimestamp

http://codereview.chromium.org/1669002/diff/52001/53003
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/52001/53003#newcode10
media/filters/ffmpeg_video_decode_engine.h:10: #include
"media/ffmpeg/ffmpeg_common.h"
alphabetize includes (ffmpeg is before filters)

http://codereview.chromium.org/1669002/diff/52001/53003#newcode48
media/filters/ffmpeg_video_decode_engine.h:48: virtual void CopyPlane(size_t
plane,
does this need to virtual?

looks like it could be a static method inside the .cc

I looked through existing source code and I believe we can safely make this
static

http://codereview.chromium.org/1669002/diff/52001/53004
File media/filters/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/1669002/diff/52001/53004#newcode32
media/filters/ffmpeg_video_decode_engine_unittest.cc:32: // DecodeFrame will
check these pointers as non-NULL value
comments end with periods

http://codereview.chromium.org/1669002/diff/52001/53005
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/52001/53005#newcode193
media/filters/omx_video_decode_engine.cc:193: // Notify the read request to this
object has been fulfilled.
nit: blank lines before comments

http://codereview.chromium.org/1669002/diff/52001/53005#newcode202
media/filters/omx_video_decode_engine.cc:202: base::TimeDelta zero;
zero is a valid timestamp

please use StreamSample::kInvalidTimestamp

http://codereview.chromium.org/1669002/diff/52001/53008
File media/filters/video_decoder_impl.cc (right):

http://codereview.chromium.org/1669002/diff/52001/53008#newcode213
media/filters/video_decoder_impl.cc:213: // First search the AVFrame for the
pts. This is the most authoritative.
comment is out of date

http://codereview.chromium.org/1669002/diff/52001/53008#newcode221
media/filters/video_decoder_impl.cc:221: if (frame) {
since we only call FindPtsAndDuration() when got_frame == true, then this
condition should always be true

I'd rather have passing a frame into this condition be required (i.e., crash
when frame is NULL)

the unit test + comments will need to be updated

http://codereview.chromium.org/1669002/diff/52001/53008#newcode223
media/filters/video_decoder_impl.cc:223: if (timestamp !=
static_cast<int64_t>(AV_NOPTS_VALUE) && timestamp != 0) {
AV_NOPTS_VALUE is an FFmpeg constant

please use StreamSample::kInvalidTimestamp

http://codereview.chromium.org/1669002/diff/52001/53008#newcode231
media/filters/video_decoder_impl.cc:231: // If the frame did not have pts, try
to get the pts from the
nit: this comment can fit on one line

http://codereview.chromium.org/1669002/diff/52001/53008#newcode244
media/filters/video_decoder_impl.cc:244: pts.duration =
ConvertTimestamp(time_base, 1);
why wouldn't we use GetRepeatCount() here?

seems like we can move this line of code up in the function and have it shared

[jiesun, 2010-04-22 21:24:27]

Done

http://codereview.chromium.org/1669002/diff/52001/53002
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/52001/53002#newcode83
media/filters/ffmpeg_video_decode_engine.cc:83: // We don't allocate AVFrame on
the stack since different versions of FFmpeg
On 2010/04/22 20:47:15, scherkus wrote:
> this comment is out of place, should go up by avcodec_alloc_frame()

Done.

http://codereview.chromium.org/1669002/diff/52001/53002#newcode88
media/filters/ffmpeg_video_decode_engine.cc:88:
avcodec_decode_video2(codec_context_,
On 2010/04/22 20:47:15, scherkus wrote:
> no need to drop all this code onto the next line

Done.

http://codereview.chromium.org/1669002/diff/52001/53002#newcode118
media/filters/ffmpeg_video_decode_engine.cc:118: base::TimeDelta zero;
On 2010/04/22 20:47:15, scherkus wrote:
> zero is a valid timestamp
> 
> please use StreamSample::kInvalidTimestamp

Done.

http://codereview.chromium.org/1669002/diff/52001/53003
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/52001/53003#newcode10
media/filters/ffmpeg_video_decode_engine.h:10: #include
"media/ffmpeg/ffmpeg_common.h"
On 2010/04/22 20:47:15, scherkus wrote:
> alphabetize includes (ffmpeg is before filters)

Done.

http://codereview.chromium.org/1669002/diff/52001/53003#newcode48
media/filters/ffmpeg_video_decode_engine.h:48: virtual void CopyPlane(size_t
plane,
On 2010/04/22 20:47:15, scherkus wrote:
> does this need to virtual?
> 
> looks like it could be a static method inside the .cc
> 
> I looked through existing source code and I believe we can safely make this
> static

Done.

http://codereview.chromium.org/1669002/diff/52001/53004
File media/filters/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/1669002/diff/52001/53004#newcode32
media/filters/ffmpeg_video_decode_engine_unittest.cc:32: // DecodeFrame will
check these pointers as non-NULL value
On 2010/04/22 20:47:15, scherkus wrote:
> comments end with periods

Done.

http://codereview.chromium.org/1669002/diff/52001/53005
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/52001/53005#newcode193
media/filters/omx_video_decode_engine.cc:193: // Notify the read request to this
object has been fulfilled.
On 2010/04/22 20:47:15, scherkus wrote:
> nit: blank lines before comments

Done.

http://codereview.chromium.org/1669002/diff/52001/53005#newcode202
media/filters/omx_video_decode_engine.cc:202: base::TimeDelta zero;
On 2010/04/22 20:47:15, scherkus wrote:
> zero is a valid timestamp
> 
> please use StreamSample::kInvalidTimestamp

Done.

http://codereview.chromium.org/1669002/diff/52001/53008
File media/filters/video_decoder_impl.cc (right):

http://codereview.chromium.org/1669002/diff/52001/53008#newcode213
media/filters/video_decoder_impl.cc:213: // First search the AVFrame for the
pts. This is the most authoritative.
On 2010/04/22 20:47:15, scherkus wrote:
> comment is out of date

Done.

http://codereview.chromium.org/1669002/diff/52001/53008#newcode221
media/filters/video_decoder_impl.cc:221: if (frame) {
On 2010/04/22 20:47:15, scherkus wrote:
> since we only call FindPtsAndDuration() when got_frame == true, then this
> condition should always be true
> 
> I'd rather have passing a frame into this condition be required (i.e., crash
> when frame is NULL)
> 
> the unit test + comments will need to be updated
> 
> 

Done.

http://codereview.chromium.org/1669002/diff/52001/53008#newcode223
media/filters/video_decoder_impl.cc:223: if (timestamp !=
static_cast<int64_t>(AV_NOPTS_VALUE) && timestamp != 0) {
On 2010/04/22 20:47:15, scherkus wrote:
> AV_NOPTS_VALUE is an FFmpeg constant
> 
> please use StreamSample::kInvalidTimestamp

Done.

http://codereview.chromium.org/1669002/diff/52001/53008#newcode231
media/filters/video_decoder_impl.cc:231: // If the frame did not have pts, try
to get the pts from the
On 2010/04/22 20:47:15, scherkus wrote:
> nit: this comment can fit on one line

Done.

http://codereview.chromium.org/1669002/diff/52001/53008#newcode244
media/filters/video_decoder_impl.cc:244: pts.duration =
ConvertTimestamp(time_base, 1);
On 2010/04/22 20:47:15, scherkus wrote:
> why wouldn't we use GetRepeatCount() here?
> 
> seems like we can move this line of code up in the function and have it shared

if ffmpeg did not set timestamp correctly, I am not sure we should trust this
value either.

[scherkus (not reviewing), 2010-04-23 00:42:56]

few tiny nits

thanks for doing this!!  something that we were hoping to get done a while ago

http://codereview.chromium.org/1669002/diff/56001/29005
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/1669002/diff/56001/29005#newcode48
media/filters/ffmpeg_video_decode_engine.h:48: static void CopyPlane(size_t
plane,
there's no need for this to be in the .h file nor part of
FFmpegVideoDecodeEngine

might as well remove the declaration from the header file and place the whole
thing inside the .cc

http://codereview.chromium.org/1669002/diff/56001/29007
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/56001/29007#newcode203
media/filters/omx_video_decode_engine.cc:203: base::TimeDelta zero;
I think this variable is now unused

http://codereview.chromium.org/1669002/diff/56001/29010
File media/filters/video_decoder_impl.cc (right):

http://codereview.chromium.org/1669002/diff/56001/29010#newcode218
media/filters/video_decoder_impl.cc:218: // Oh, and we have to cast
AV_NOPTS_VALUE since it ends up becoming unsigned
you can remove the whole part of the comment about AV_NOPTS_VALUE since we now
use StreamSample::kInvalidTimestamp

http://codereview.chromium.org/1669002/diff/56001/29010#newcode223
media/filters/video_decoder_impl.cc:223: int64_t invalid_ts =
StreamSample::kInvalidTimestamp.ToInternalValue();
it'd be best to avoid int64s.  can just do the following:
if (timestamp != StreamSample::kInvalidTimestamp && timestamp.InternalValue() !=
0)

http://codereview.chromium.org/1669002/diff/56001/29010#newcode242
media/filters/video_decoder_impl.cc:242: // TODO(ajwong): Make sure this formula
is correct.
you can remove this TODO as the calculation is no longer performed

[jiesun, 2010-04-23 01:06:50]

http://codereview.chromium.org/1669002/diff/56001/29007
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/1669002/diff/56001/29007#newcode203
media/filters/omx_video_decode_engine.cc:203: base::TimeDelta zero;
On 2010/04/23 00:42:56, scherkus wrote:
> I think this variable is now unused

Done.

http://codereview.chromium.org/1669002/diff/56001/29010
File media/filters/video_decoder_impl.cc (right):

http://codereview.chromium.org/1669002/diff/56001/29010#newcode218
media/filters/video_decoder_impl.cc:218: // Oh, and we have to cast
AV_NOPTS_VALUE since it ends up becoming unsigned
On 2010/04/23 00:42:56, scherkus wrote:
> you can remove the whole part of the comment about AV_NOPTS_VALUE since we now
> use StreamSample::kInvalidTimestamp

Done.

http://codereview.chromium.org/1669002/diff/56001/29010#newcode223
media/filters/video_decoder_impl.cc:223: int64_t invalid_ts =
StreamSample::kInvalidTimestamp.ToInternalValue();
On 2010/04/23 00:42:56, scherkus wrote:
> it'd be best to avoid int64s.  can just do the following:
> if (timestamp != StreamSample::kInvalidTimestamp && timestamp.InternalValue()
!=
> 0)

Done.

http://codereview.chromium.org/1669002/diff/56001/29010#newcode242
media/filters/video_decoder_impl.cc:242: // TODO(ajwong): Make sure this formula
is correct.
On 2010/04/23 00:42:56, scherkus wrote:
> you can remove this TODO as the calculation is no longer performed

Done.

[Alpha Left Google, 2010-04-23 01:11:34]

Awsome! LGTM.

[scherkus (not reviewing), 2010-04-23 01:21:15]

LGTM!  thanks for tackling this jie!