Issue 465044: Refactor FFmpegVideoDecoder to try and generalize code common to all video decoders.

Issue 465044: Refactor FFmpegVideoDecoder to try and generalize code common to all video decoders. (Closed)

Created:
11 years ago by awong

Modified:
9 years, 7 months ago

Reviewers:
scherkus (not reviewing)

CC:
chromium-reviews_googlegroups.com, Alpha Left Google, fbarchard

Visibility:
Public.

Description

Refactor FFmpegVideoDecoder to try and generalize code common to all video decoders. This changes the DecoderBase API to be fully asynchronous when invoking its subclass's actions.

Patch Set 2 : Cleaned up a bit. #

Total comments: 21

Patch Set 3 : Unittests fixed. #

Patch Set 4 : Cleanup for review. #

Patch Set 5 : Missed mock_task.h #

Total comments: 58

Patch Set 6 : Fix unittests. #

Patch Set 9 : Fix comments from andrew. #

Total comments: 2

Patch Set 11 : Fix SCOPED_TRACE since VS faults on %zd. #

Created: 11 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+913 lines, -360 lines)			Patch
A	media/base/callback.h	View	4 5 6 7 8	1 chunk	+62 lines, -0 lines	0 comments	Download
A	media/base/mock_task.h	View	5	1 chunk	+110 lines, -0 lines	0 comments	Download
A	media/ffmpeg/ffmpeg_util.h	View		1 chunk	+18 lines, -0 lines	0 comments	Download
A	media/ffmpeg/ffmpeg_util.cc	View		1 chunk	+18 lines, -0 lines	0 comments	Download
M	media/filters/decoder_base.h	View	1 2 3 4 5	8 chunks	+52 lines, -28 lines	0 comments	Download
M	media/filters/ffmpeg_audio_decoder.h	View		1 chunk	+4 lines, -5 lines	0 comments	Download
M	media/filters/ffmpeg_audio_decoder.cc	View	1 2 3	4 chunks	+20 lines, -11 lines	0 comments	Download
M	media/filters/ffmpeg_demuxer.h	View	1 2 3	2 chunks	+3 lines, -1 line	0 comments	Download
M	media/filters/ffmpeg_demuxer.cc	View	1 2 3 4 5	4 chunks	+11 lines, -7 lines	0 comments	Download
A	media/filters/ffmpeg_video_decode_engine_unittest.cc	View	4 5 6 7 8	1 chunk	+184 lines, -0 lines	0 comments	Download
M	media/filters/ffmpeg_video_decoder.h	View	1 2 3 4 5 6 7 8	5 chunks	+50 lines, -28 lines	0 comments	Download
M	media/filters/ffmpeg_video_decoder.cc	View	1 2 3 4 5 6 7 8	6 chunks	+190 lines, -111 lines	0 comments	Download
M	media/filters/ffmpeg_video_decoder_unittest.cc	View	3 4 5 6 7 8 9 10	18 chunks	+128 lines, -169 lines	0 comments	Download
A	media/filters/video_decode_engine.h	View	4 5 6 7 8 9	1 chunk	+57 lines, -0 lines	0 comments	Download
M	media/media.gyp	View	1 2 3	5 chunks	+6 lines, -0 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

scherkus (not reviewing)

http://codereview.chromium.org/465044/diff/2001/3002 File media/ffmpeg/ffmpeg_util.h (right): http://codereview.chromium.org/465044/diff/2001/3002#newcode5 media/ffmpeg/ffmpeg_util.h:5: #ifndef MEDIA_FFMPEG_FFMPEG_UTIL_H_ feel free to check in this change ...

11 years ago (2009-12-04 02:44:21 UTC) #1

awong

Ready for another pass. http://codereview.chromium.org/465044/diff/2001/3002 File media/ffmpeg/ffmpeg_util.h (right): http://codereview.chromium.org/465044/diff/2001/3002#newcode5 media/ffmpeg/ffmpeg_util.h:5: #ifndef MEDIA_FFMPEG_FFMPEG_UTIL_H_ On 2009/12/04 02:44:21, ...

11 years ago (2009-12-08 00:33:00 UTC) #2

Ready for another pass.

http://codereview.chromium.org/465044/diff/2001/3002
File media/ffmpeg/ffmpeg_util.h (right):

http://codereview.chromium.org/465044/diff/2001/3002#newcode5
media/ffmpeg/ffmpeg_util.h:5: #ifndef MEDIA_FFMPEG_FFMPEG_UTIL_H_
On 2009/12/04 02:44:21, scherkus wrote:
> feel free to check in this change

Left it mixed in with the rest.

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

http://codereview.chromium.org/465044/diff/2001/3003#newcode82
media/filters/decoder_base.h:82: // FilterCallbacks.  However, since NewCallback
cannot prebind parameters,
On 2009/12/04 02:44:21, scherkus wrote:
> NewCallback -> NewCallback()

Done.

http://codereview.chromium.org/465044/diff/2001/3003#newcode83
media/filters/decoder_base.h:83: // Curry, we use NewRunnableMethod instead
which causes an unforutnate
On 2009/12/04 02:44:21, scherkus wrote:
> unforutnate -> unfortunate

Done.

http://codereview.chromium.org/465044/diff/2001/3003#newcode83
media/filters/decoder_base.h:83: // Curry, we use NewRunnableMethod instead
which causes an unforutnate
On 2009/12/04 02:44:21, scherkus wrote:
> NewRunnableMethod -> NewRunnableMethod()

Done.

http://codereview.chromium.org/465044/diff/2001/3003#newcode145
media/filters/decoder_base.h:145: scoped_ptr<FilterCallback> c(callback);
On 2009/12/04 02:44:21, scherkus wrote:
> callback will be deleted = bad

Done.

http://codereview.chromium.org/465044/diff/2001/3003#newcode175
media/filters/decoder_base.h:175: bool* success = new bool;
On 2009/12/04 02:44:21, scherkus wrote:
> hmm.. re Task versus NewCallback.. you're interested in pre-binding the bool
and
> done_cb?
> 
> I'm wondering about going down a path refactoring working around lack of
> pre-bound callbacks (then having to do it again).
> 
> Also what about callback-then-inspect instead of using bool*?  If we have the
> callees implementing our interface, we can have them provide an getters for
> whether they properly initialized (i.e., inspect their state)
> 
> UPDATE: oh I see you're doing that INSIDE of FFmpegVideoDecoder..
> 
> Let's get this straight.. long-term goal is:
> FFmpegVideoDecoder --> VideoDecoderImpl + VideoDecodeEngine
> 
> What about getting rid of subclassing DecoderBase<T> and using more
composition?
>  Do you plan on keeping VideoDecoderImpl a subclass in the short or longer
term
> (or at all)?
> 
> There still looks like there's a lot of FFmpeg-specific goo inside of your new
> FFmpegVideoDecoder.  Would what you have now help you implement a different
> engine?  (i.e., is what's left over in FFmpegVideoDecoder useful for all/most
> video engines)
> 
> We might be better off chatting about it tomorrow.

Discussed offline.  I think it's okay as is.  Please comment again if you think
I need to change something.

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

http://codereview.chromium.org/465044/diff/2001/3004#newcode7
media/filters/ffmpeg_audio_decoder.cc:7: #include "media/base/callback.h"
On 2009/12/04 02:44:21, scherkus wrote:
> not seeing this file uploaded

added.

http://codereview.chromium.org/465044/diff/2001/3004#newcode57
media/filters/ffmpeg_audio_decoder.cc:57:
static_cast<size_t>(codec_context_->sample_rate) > Limits::kMaxSampleRate) {
On 2009/12/04 02:44:21, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/465044/diff/2001/3006
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/465044/diff/2001/3006#newcode197
media/filters/ffmpeg_demuxer.cc:197: base::TimeDelta
FFmpegDemuxerStream::ConvertStreamTimestamp(
On 2009/12/04 02:44:21, scherkus wrote:
> static?

Listed as static in the class definition.

http://codereview.chromium.org/465044/diff/2001/3009
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/465044/diff/2001/3009#newcode36
media/filters/ffmpeg_video_decoder.h:36: virtual void Initialize(Task* done_cb);
On 2009/12/04 02:44:21, scherkus wrote:
> no need for virtual?

For mockability you need it to be virtual. Also, this whole has been changed
into inherit from an abstract interface.

scherkus (not reviewing)

I think I covered most of it :) http://codereview.chromium.org/465044/diff/1012/1013 File media/base/callback.h (right): http://codereview.chromium.org/465044/diff/1012/1013#newcode4 media/base/callback.h:4: could ...

11 years ago (2009-12-08 01:52:53 UTC) #3

awong

PTAL http://codereview.chromium.org/465044/diff/1012/1013 File media/base/callback.h (right): http://codereview.chromium.org/465044/diff/1012/1013#newcode4 media/base/callback.h:4: On 2009/12/08 01:52:53, scherkus wrote: > could we ...

11 years ago (2009-12-09 00:34:50 UTC) #4

PTAL

http://codereview.chromium.org/465044/diff/1012/1013
File media/base/callback.h (right):

http://codereview.chromium.org/465044/diff/1012/1013#newcode4
media/base/callback.h:4: 
On 2009/12/08 01:52:53, scherkus wrote:
> could we get a super quick mini description?  1-2 lines should be sufficient

Done.

http://codereview.chromium.org/465044/diff/1012/1013#newcode26
media/base/callback.h:26: Task* release() { return task_.release(); }
On 2009/12/08 01:52:53, scherkus wrote:
> is the lack of symmetry between these two intentional?  for example:
>  - AutoTaskRunner accepts NULL
>  - AutoTaskRunner can release

Forgot to update the other.  Have done so.

We really need to unify the 0-cardinality function pointer abstractions.

http://codereview.chromium.org/465044/diff/1012/1013#newcode30
media/base/callback.h:30: };
On 2009/12/08 01:52:53, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN(AutoTaskRunner);

Done.

http://codereview.chromium.org/465044/diff/1012/1013#newcode46
media/base/callback.h:46: };
On 2009/12/08 01:52:53, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN(AutoCallbackRunner);

Done.

http://codereview.chromium.org/465044/diff/1012/1014
File media/base/mock_task.h (right):

http://codereview.chromium.org/465044/diff/1012/1014#newcode49
media/base/mock_task.h:49: #ifndef MEDIA_BASE_MOCK_TASK_H_
On 2009/12/08 01:52:53, scherkus wrote:
> we've got a bit of a naming issue here unfortunately (file should be
> task_mocker.h reflecting the class) but I'll let it slide to keep our
> /media/base/mock_*.h collection of files

Yeah. Originally it was MockTask.  But then it got complicated, so I renamed it
such that people won't think it's a dumb mock.  Leaving it as it doesn't sound
too bad.

http://codereview.chromium.org/465044/diff/1012/1014#newcode57
media/base/mock_task.h:57: ACTION(InvokeRunnable) { arg0->Run();
On 2009/12/08 01:52:53, scherkus wrote:
> formatting

Just reworded.

http://codereview.chromium.org/465044/diff/1012/1014#newcode67
media/base/mock_task.h:67: CHECK(outstanding_tasks_ == 0) << "If non-0 tasks
have been leaked.";
On 2009/12/08 01:52:53, scherkus wrote:
> nit: non-0 -> non-zero
> (looks really strange)

Done.

http://codereview.chromium.org/465044/diff/1012/1014#newcode70
media/base/mock_task.h:70: Task* GetTask() {
On 2009/12/08 01:52:53, scherkus wrote:
> naming nit: this doesn't so much get a task as it actually creates a task
> 
> NewTask()?
> CreateTask()?

Done.

http://codereview.chromium.org/465044/diff/1012/1014#newcode95
media/base/mock_task.h:95: };
On 2009/12/08 01:52:53, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/465044/diff/1012/1014#newcode98
media/base/mock_task.h:98: };
On 2009/12/08 01:52:53, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/465044/diff/1012/1017
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/465044/diff/1012/1017#newcode174
media/filters/decoder_base.h:174: bool* success = new bool;
On 2009/12/08 01:52:53, scherkus wrote:
> I can't remember.. didn't we think about using a member variable i.e.,
> initialization_succeeded_ (or something less verbose)

We didn't conclude.  I wouldn't mind doing what you're suggesting, but it's not
safe with our current API because it is theoretically possible to Initialize()
called multiple times and we'd have a race condition.

http://codereview.chromium.org/465044/diff/1012/1020
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/465044/diff/1012/1020#newcode197
media/filters/ffmpeg_demuxer.cc:197: base::TimeDelta
FFmpegDemuxerStream::ConvertStreamTimestamp(
On 2009/12/08 01:52:53, scherkus wrote:
> chrome has this code style thing where in the .cc file we've been adding "//
> static" to the line before static method implementations

static clung the comment.

http://codereview.chromium.org/465044/diff/1012/1022
File media/filters/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/465044/diff/1012/1022#newcode55
media/filters/ffmpeg_video_decode_engine_unittest.cc:55:
test_engine_->DecodeFrame(*buffer_, &yuv_frame_, &got_result,
done_cb.GetTask());
On 2009/12/08 01:52:53, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/465044/diff/1012/1022#newcode71
media/filters/ffmpeg_video_decode_engine_unittest.cc:71:
test_engine_->DecodeFrame(*buffer_, &yuv_frame_, &got_result,
done_cb.GetTask());
On 2009/12/08 01:52:53, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/465044/diff/1012/1022#newcode86
media/filters/ffmpeg_video_decode_engine_unittest.cc:86:
test_engine_->DecodeFrame(*buffer_, &yuv_frame_, &got_result,
done_cb.GetTask());
On 2009/12/08 01:52:53, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/465044/diff/1012/1023
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/465044/diff/1012/1023#newcode102
media/filters/ffmpeg_video_decoder.cc:102: done_cb->Run();
On 2009/12/08 01:52:53, scherkus wrote:
> AutoTaskRunner?
> 
> what's your heuristic for AutoTaskRunner versus none :P

I only used AutoTaskRunner in situations where there was an early return.  I
guess that's bug prone.  I'll change things to be consistent.

http://codereview.chromium.org/465044/diff/1012/1023#newcode308
media/filters/ffmpeg_video_decoder.cc:308: done_cb->Run();
On 2009/12/08 01:52:53, scherkus wrote:
> AutoTaskRunner?

Done.

http://codereview.chromium.org/465044/diff/1012/1023#newcode429
media/filters/ffmpeg_video_decoder.cc:429: void
FFmpegVideoDecoder::SetVideoDecodeEngineForTest(VideoDecodeEngine* engine) {
On 2009/12/08 01:52:53, scherkus wrote:
> >80 chars

Done.

http://codereview.chromium.org/465044/diff/1012/1024
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/465044/diff/1012/1024#newcode44
media/filters/ffmpeg_video_decoder.h:44: };
On 2009/12/08 01:52:53, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/465044/diff/1012/1024#newcode114
media/filters/ffmpeg_video_decoder.h:114: virtual void
SetVideoDecodeEngineForTest(VideoDecodeEngine* engine);
On 2009/12/08 01:52:53, scherkus wrote:
> this could be alternatively be done using the constructor and a filter factory
> that passes the engine in
> 
> I think that'd be cleaner (getting FFmpegVideoDecoder -> VideoDecoderImpl +
pass
> in abstract engine) but an involved change -- add a TODO?

Yes.  I just didn't want to mess with the factories.

Had to do it anyways (and still need this function right now) so we have the
factory change and this set function.

http://codereview.chromium.org/465044/diff/1012/1025
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/465044/diff/1012/1025#newcode344
media/filters/ffmpeg_video_decoder_unittest.cc:344: // Inject mock engine. 
Ownership is transfered to mock_decoder, but we hold a
On 2009/12/08 01:52:53, scherkus wrote:
> mock_decoder -> |mock_decoder|

Done.

http://codereview.chromium.org/465044/diff/1012/1025#newcode448
media/filters/ffmpeg_video_decoder_unittest.cc:448: // Inject mock engine. 
Ownership is transfered to mock_decoder, but we hold a
On 2009/12/08 01:52:53, scherkus wrote:
> mock_decoder -> |mock_decoder|

Done.

http://codereview.chromium.org/465044/diff/1012/1025#newcode519
media/filters/ffmpeg_video_decoder_unittest.cc:519: // Inject mock engine. 
Ownership is transfered to mock_decoder, but we
On 2009/12/08 01:52:53, scherkus wrote:
> mock_decoder -> |mock_decoder|

Done.

http://codereview.chromium.org/465044/diff/1012/1026
File media/filters/video_decode_engine.h (right):

http://codereview.chromium.org/465044/diff/1012/1026#newcode8
media/filters/video_decode_engine.h:8: #include
"media/filters/video_decode_engine.h"
On 2009/12/08 01:52:53, scherkus wrote:
> no need to include ourselves :)

Done.

http://codereview.chromium.org/465044/diff/1012/1026#newcode19
media/filters/video_decode_engine.h:19: class VideoDecodeEngine  {
On 2009/12/08 01:52:53, scherkus wrote:
> too many spaces before {

Done.

http://codereview.chromium.org/465044/diff/1012/1026#newcode39
media/filters/video_decode_engine.h:39: virtual void DecodeFrame(const Buffer&
buffer, AVFrame* yuv_frame,
On 2009/12/08 01:52:53, scherkus wrote:
> AVFrame!!! yuck :(
> 
> ideally we should fill out a VideoFrame (non-FFmpeg output type)
> 
> this probably involves moving more logic from FFmpegVideoDecoder into the
engine
> (i.e., parts of EnqueueVideoFrame() where we do the conversion to
> VideoFrameImpl)

That's a bigger change.  Punting.

http://codereview.chromium.org/465044/diff/1012/1026#newcode45
media/filters/video_decode_engine.h:45: // Returns the current state of the
deocde engine.
On 2009/12/08 01:52:53, scherkus wrote:
> deocde -> decode

Done.

http://codereview.chromium.org/465044/diff/1012/1026#newcode48
media/filters/video_decode_engine.h:48: // Converts a AVCodecContext |pix_fmt|
to a VideoSurface::Format.
On 2009/12/08 01:52:53, scherkus wrote:
> this interface should have no mention of FFmpeg at all (i.e., AVCodecContext
or
> pix_fmt)
> 
> update comment

Done.

scherkus (not reviewing)

http://codereview.chromium.org/465044/diff/1012/1026 File media/filters/video_decode_engine.h (right): http://codereview.chromium.org/465044/diff/1012/1026#newcode39 media/filters/video_decode_engine.h:39: virtual void DecodeFrame(const Buffer& buffer, AVFrame* yuv_frame, On 2009/12/09 ...

11 years ago (2009-12-09 00:48:32 UTC) #5

awong

fixed. http://codereview.chromium.org/465044/diff/6002/7001 File media/base/callback.h (right): http://codereview.chromium.org/465044/diff/6002/7001#newcode44 media/base/callback.h:44: CHECK(callback_.get()); On 2009/12/09 00:48:32, scherkus wrote: > probably ...

11 years ago (2009-12-09 01:02:58 UTC) #6

scherkus (not reviewing)

LGTM one extra line snuck in I think http://codereview.chromium.org/465044/diff/8004/9015 File media/filters/video_decode_engine.h (right): http://codereview.chromium.org/465044/diff/8004/9015#newcode8 media/filters/video_decode_engine.h:8: remove ...

11 years ago (2009-12-09 01:12:18 UTC) #7

awong

11 years ago (2009-12-09 01:24:44 UTC) #8

Fixed.  Windows try died, but I think I found the bug. Once that's fixed, will
submit.

http://codereview.chromium.org/465044/diff/8004/9015
File media/filters/video_decode_engine.h (right):

http://codereview.chromium.org/465044/diff/8004/9015#newcode8
media/filters/video_decode_engine.h:8: 
On 2009/12/09 01:12:18, scherkus wrote:
> remove extra line

Done.

Expand Messages | Collapse Messages