move from omx_codec to new omx_video_decode_engine...

Remove omx_codec since OMX client implementation is in omx_video_decode_engine. Update omx_test to use omx_video_decode_engine.

Contributed by wjia@chromium.org

BUG=none
TEST=tested on tegra

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

[wjia(left Chromium), 2010-05-27 17:29:58]

omx_test has been updated with this patch. It needs the pending patch which
merged omx_codec and engine.

[wjia(left Chromium), 2010-05-27 17:30:53]

omx_test has been updated with this patch. It needs the pending patch which
merged omx_codec and engine.

[jiesun, 2010-05-27 17:43:55]

http://codereview.chromium.org/2255005/diff/1/2
File media/media.gyp (right):

http://codereview.chromium.org/2255005/diff/1/2#newcode337
media/media.gyp:337: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
I was thinking if we could remove av_stream from Engine interface later.

http://codereview.chromium.org/2255005/diff/1/3
File media/tools/omx_test/omx_test.cc (left):

http://codereview.chromium.org/2255005/diff/1/3#oldcode37
media/tools/omx_test/omx_test.cc:37: using media::Buffer;
alphabeta order

http://codereview.chromium.org/2255005/diff/1/3
File media/tools/omx_test/omx_test.cc (right):

http://codereview.chromium.org/2255005/diff/1/3#newcode22
media/tools/omx_test/omx_test.cc:22: #include "media/base/data_buffer.h"
alphabeta order

http://codereview.chromium.org/2255005/diff/1/3#newcode159
media/tools/omx_test/omx_test.cc:159: av_stream_.get(),
indent?

I think we should also decouple Initialize() and Play() for engine_ interface.
but that could be done in another CL.

[wjia(left Chromium), 2010-05-27 18:07:42]

http://codereview.chromium.org/2255005/diff/1/2
File media/media.gyp (right):

http://codereview.chromium.org/2255005/diff/1/2#newcode337
media/media.gyp:337: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
On 2010/05/27 17:43:56, jiesun wrote:
> I was thinking if we could remove av_stream from Engine interface later.

Let's do it one by one. Changing engine API is another task.

http://codereview.chromium.org/2255005/diff/1/3
File media/tools/omx_test/omx_test.cc (left):

http://codereview.chromium.org/2255005/diff/1/3#oldcode37
media/tools/omx_test/omx_test.cc:37: using media::Buffer;
On 2010/05/27 17:43:56, jiesun wrote:
> alphabeta order

Done.

http://codereview.chromium.org/2255005/diff/1/3
File media/tools/omx_test/omx_test.cc (right):

http://codereview.chromium.org/2255005/diff/1/3#newcode22
media/tools/omx_test/omx_test.cc:22: #include "media/base/data_buffer.h"
On 2010/05/27 17:43:56, jiesun wrote:
> alphabeta order

Done.

http://codereview.chromium.org/2255005/diff/1/3#newcode159
media/tools/omx_test/omx_test.cc:159: av_stream_.get(),
On 2010/05/27 17:43:56, jiesun wrote:
> indent?
> 
> I think we should also decouple Initialize() and Play() for engine_ interface.
> but that could be done in another CL.

Done.

[jiesun, 2010-05-27 18:13:09]

Could we remove omx_codec.*. since it is not used. and disable
omx_codec_unittest until we had omx_decoder_engine_unittest?

[wjia(left Chromium), 2010-05-27 18:16:43]

omx_codec_unittest has been disabled in the tree. I will remove omx_codec.

On Thu, May 27, 2010 at 11:13 AM, <jiesun@chromium.org> wrote:

> Could we remove omx_codec.*. since it is not used. and disable
> omx_codec_unittest until we had omx_decoder_engine_unittest?
>
>
> http://codereview.chromium.org/2255005/show
>

[Alpha Left Google, 2010-05-27 18:32:39]

The CL description is not so accurate.

Should say removed OmxCodec as the implementation is in OmxVideoDecodeEngine.

[Alpha Left Google, 2010-05-27 19:58:13]

My biggest concern is that VideoDecodeEngine depends on FFmpeg, should really
remove that dependency.

http://codereview.chromium.org/2255005/diff/11001/12001
File media/media.gyp (right):

http://codereview.chromium.org/2255005/diff/11001/12001#newcode337
media/media.gyp:337: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
omx_wrapper depends on FFmpeg makes me feel uneasy. The problem is that
VideoDecodeEngine's interface depends on AVStream which should not be there.

My suggestion is make that AVStream parameter be a void pointer and be something
like a decoder_private variable so that not all video decode engine
implementation has to know FFmpeg.

http://codereview.chromium.org/2255005/diff/11001/12004
File media/tools/omx_test/omx_test.cc (right):

http://codereview.chromium.org/2255005/diff/11001/12004#newcode229
media/tools/omx_test/omx_test.cc:229: av_stream->codec->codec_id =
CODEC_ID_NONE;
We really should have VideoDecoderEngine not depend on AVStream, just make that
parameter a void* and we can pass in OmxConfigurator.

[wjia(left Chromium), 2010-05-27 21:41:55]

CL description has been updated. Let's change engine API in next patch.

http://codereview.chromium.org/2255005/diff/11001/12001
File media/media.gyp (right):

http://codereview.chromium.org/2255005/diff/11001/12001#newcode337
media/media.gyp:337: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
I completely agree to remove AVStream. I'd replace with another structure which
is neutral to any decoder. We do need some info (such as codec type) from
AVStream. So eventually it will be a copy some info from AVStream into a new
structure. Removing AVStream results in engine API change. I'd do it in a new
patch.

On 2010/05/27 19:58:13, Alpha wrote:
> omx_wrapper depends on FFmpeg makes me feel uneasy. The problem is that
> VideoDecodeEngine's interface depends on AVStream which should not be there.
> 
> My suggestion is make that AVStream parameter be a void pointer and be
something
> like a decoder_private variable so that not all video decode engine
> implementation has to know FFmpeg.

[Alpha Left Google, 2010-05-27 21:52:37]

LGTM but remember to add the TODO.

http://codereview.chromium.org/2255005/diff/11001/12001
File media/media.gyp (right):

http://codereview.chromium.org/2255005/diff/11001/12001#newcode337
media/media.gyp:337: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
On 2010/05/27 21:41:55, wjia wrote:
> I completely agree to remove AVStream. I'd replace with another structure
which
> is neutral to any decoder. We do need some info (such as codec type) from
> AVStream. So eventually it will be a copy some info from AVStream into a new
> structure. Removing AVStream results in engine API change. I'd do it in a new
> patch.
> 
> On 2010/05/27 19:58:13, Alpha wrote:
> > omx_wrapper depends on FFmpeg makes me feel uneasy. The problem is that
> > VideoDecodeEngine's interface depends on AVStream which should not be there.
> > 
> > My suggestion is make that AVStream parameter be a void pointer and be
> something
> > like a decoder_private variable so that not all video decode engine
> > implementation has to know FFmpeg.
> 
> 

I suggest it just be a void* since ffmpeg requires more data than just the
format, it actually needs codec context from demuxer. But yes, let's do that in
another patch. Just add a TODO to remove ffmpeg.gyp here.

[wjia(left Chromium), 2010-05-27 23:08:00]

Done. 

On 2010/05/27 21:52:37, Alpha wrote:
> LGTM but remember to add the TODO.
> 
> http://codereview.chromium.org/2255005/diff/11001/12001
> File media/media.gyp (right):
> 
> http://codereview.chromium.org/2255005/diff/11001/12001#newcode337
> media/media.gyp:337: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
> On 2010/05/27 21:41:55, wjia wrote:
> > I completely agree to remove AVStream. I'd replace with another structure
> which
> > is neutral to any decoder. We do need some info (such as codec type) from
> > AVStream. So eventually it will be a copy some info from AVStream into a new
> > structure. Removing AVStream results in engine API change. I'd do it in a
new
> > patch.
> > 
> > On 2010/05/27 19:58:13, Alpha wrote:
> > > omx_wrapper depends on FFmpeg makes me feel uneasy. The problem is that
> > > VideoDecodeEngine's interface depends on AVStream which should not be
there.
> > > 
> > > My suggestion is make that AVStream parameter be a void pointer and be
> > something
> > > like a decoder_private variable so that not all video decode engine
> > > implementation has to know FFmpeg.
> > 
> > 
> 
> I suggest it just be a void* since ffmpeg requires more data than just the
> format, it actually needs codec context from demuxer. But yes, let's do that
in
> another patch. Just add a TODO to remove ffmpeg.gyp here.

[jiesun, 2010-05-27 23:12:35]

LGTM