Removed custom FFmpegLock. Removed ffmpeg headers from third_party/ffmpeg/include.

Removed custom FFmpegLock. Removed ffmpeg headers from third_party/ffmpeg/include.
BUG=23271
TEST=none

[Sergey Ulanov, 2010-03-11 02:34:52]

Review please

[fbarchard, 2010-03-11 19:59:19]

Thanks for the patch!

Theres a style nit you should fix.
Test on windows... I think you gyp assumes the source tree is there.  If you
want to pull in source, change src/DEPS and then you could completely delete the
include folder.

http://codereview.chromium.org/859001/diff/5001/6001
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/859001/diff/5001/6001#newcode96
media/base/mock_ffmpeg.cc:96: {
{ should be on line above.  see style of other functions

http://codereview.chromium.org/859001/diff/5001/6002
File media/base/mock_ffmpeg.h (right):

http://codereview.chromium.org/859001/diff/5001/6002#newcode24
media/base/mock_ffmpeg.h:24: MOCK_METHOD1(AVRegisterLockManager, int(int
(*cb)(void **, enum AVLockOp)));
were you able to test this?

http://codereview.chromium.org/859001/diff/5001/6003
File media/ffmpeg/ffmpeg_common.cc (left):

http://codereview.chromium.org/859001/diff/5001/6003#oldcode17
media/ffmpeg/ffmpeg_common.cc:17: }
can you explain?  are we moving away from an ffmpeg global lock?

http://codereview.chromium.org/859001/diff/5001/6004
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/859001/diff/5001/6004#newcode24
media/ffmpeg/ffmpeg_common.h:24: #include
"third_party/ffmpeg/source/patched-ffmpeg-mt/libavutil/log.h"
hmmm... i'm not sure if this is a good idea, because it makes windows and linux
do it differently.
could you make windows src/DEPS pull in the source so the includes can be done
the same way on all platforms?

http://codereview.chromium.org/859001/diff/5001/6005
File media/filters/ffmpeg_audio_decoder.cc (left):

http://codereview.chromium.org/859001/diff/5001/6005#oldcode65
media/filters/ffmpeg_audio_decoder.cc:65: AutoLock
auto_lock(FFmpegLock::get()->lock());
cool... locks are evil.  nice job

http://codereview.chromium.org/859001/diff/5001/6006
File media/filters/ffmpeg_demuxer.cc (left):

http://codereview.chromium.org/859001/diff/5001/6006#oldcode238
media/filters/ffmpeg_demuxer.cc:238: AutoLock
auto_lock(FFmpegLock::get()->lock());
nice.  can you update the todo?  is this still needed?

http://codereview.chromium.org/859001/diff/5001/6007
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/859001/diff/5001/6007#newcode87
media/filters/ffmpeg_glue.cc:87: *lock = new Lock();
new... hmmm..  check with Andrew but I dont think we use new typically

http://codereview.chromium.org/859001/diff/5001/6008
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/859001/diff/5001/6008#newcode52
media/filters/ffmpeg_video_decode_engine.cc:52:
avcodec_thread_init(codec_context_, kDecodeThreads) >= 0 &&
this bit breaks for ffmpeg that does not support threads
i wonder if this is a good time to be more explicite about that part of inits. 
fail or warn if threads fails with a more clear failure compared to open()

http://codereview.chromium.org/859001/diff/5001/6009
File third_party/ffmpeg/avcodec-52.def (right):

http://codereview.chromium.org/859001/diff/5001/6009#newcode15
third_party/ffmpeg/avcodec-52.def:15: av_lockmgr_register
looks good.  in this area, i'd like to see new ffmpeg still work with old
chrome.  should work?

http://codereview.chromium.org/859001/diff/5001/6011
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/859001/diff/5001/6011#newcode64
third_party/ffmpeg/ffmpeg.gyp:64: 'ffmpeg_include_root':
'source/patched-ffmpeg-mt',
same as previous concern.  should be consistent across platforms.

http://codereview.chromium.org/859001/diff/5001/6012
File third_party/ffmpeg/ffmpeg_stub_headers.fragment (right):

http://codereview.chromium.org/859001/diff/5001/6012#newcode7
third_party/ffmpeg/ffmpeg_stub_headers.fragment:7: #include
"third_party/ffmpeg/source/patched-ffmpeg-mt/libavformat/avformat.h"
windows?

http://codereview.chromium.org/859001/diff/5001/6012#newcode9
third_party/ffmpeg/ffmpeg_stub_headers.fragment:9: typedef int
(*AVLockMgrFunc)(void **mutex, enum AVLockOp op);
why is this prototyped?

[scherkus (not reviewing), 2010-03-11 21:20:40]

so far looking good!

we should do the include directory switch as a separate patch, land that, make
sure everything still works, then land this

http://codereview.chromium.org/859001/diff/5001/6002
File media/base/mock_ffmpeg.h (right):

http://codereview.chromium.org/859001/diff/5001/6002#newcode24
media/base/mock_ffmpeg.h:24: MOCK_METHOD1(AVRegisterLockManager, int(int
(*cb)(void **, enum AVLockOp)));
pointers go with types

Good: void** foo
Bad: void **foo

http://codereview.chromium.org/859001/diff/5001/6003
File media/ffmpeg/ffmpeg_common.cc (left):

http://codereview.chromium.org/859001/diff/5001/6003#oldcode17
media/ffmpeg/ffmpeg_common.cc:17: }
On 2010/03/11 19:59:19, fbarchard wrote:
> can you explain?  are we moving away from an ffmpeg global lock?

sergey is replacing our ad-hoc global lock with FFmpeg's API

http://code.google.com/p/chromium/issues/detail?id=23271

http://codereview.chromium.org/859001/diff/5001/6004
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/859001/diff/5001/6004#newcode24
media/ffmpeg/ffmpeg_common.h:24: #include
"third_party/ffmpeg/source/patched-ffmpeg-mt/libavutil/log.h"
On 2010/03/11 19:59:19, fbarchard wrote:
> hmmm... i'm not sure if this is a good idea, because it makes windows and
linux
> do it differently.
> could you make windows src/DEPS pull in the source so the includes can be done
> the same way on all platforms?

that should probably be done in a separate change before this one lands

http://codereview.chromium.org/859001/diff/5001/6005
File media/filters/ffmpeg_audio_decoder.cc (left):

http://codereview.chromium.org/859001/diff/5001/6005#oldcode65
media/filters/ffmpeg_audio_decoder.cc:65: AutoLock
auto_lock(FFmpegLock::get()->lock());
On 2010/03/11 19:59:19, fbarchard wrote:
> cool... locks are evil.  nice job

they're not going away.. avcodec_open() will call the lock instead :)

http://codereview.chromium.org/859001/diff/5001/6006
File media/filters/ffmpeg_demuxer.cc (left):

http://codereview.chromium.org/859001/diff/5001/6006#oldcode238
media/filters/ffmpeg_demuxer.cc:238: AutoLock
auto_lock(FFmpegLock::get()->lock());
On 2010/03/11 19:59:19, fbarchard wrote:
> nice.  can you update the todo?  is this still needed?
> 

no need, TODO applies to the following code not the locking

http://codereview.chromium.org/859001/diff/5001/6007
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/859001/diff/5001/6007#newcode84
media/filters/ffmpeg_glue.cc:84: int LockManagerFunc(void **lock, enum AVLockOp
op) {
pointer goes with type

LockManagerFunc is a little short on description .. maybe LockManagerOperation?

http://codereview.chromium.org/859001/diff/5001/6007#newcode86
media/filters/ffmpeg_glue.cc:86: case AV_LOCK_CREATE:
indent inside of switch {} by two spaces

http://codereview.chromium.org/859001/diff/5001/6007#newcode87
media/filters/ffmpeg_glue.cc:87: *lock = new Lock();
On 2010/03/11 19:59:19, fbarchard wrote:
> new... hmmm..  check with Andrew but I dont think we use new typically

in this instance we'll be using new and expecting FFmpeg to call AV_LOCK_DESTROY
during shutdown, which will call delete

http://codereview.chromium.org/859001/diff/5001/6008
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/859001/diff/5001/6008#newcode52
media/filters/ffmpeg_video_decode_engine.cc:52:
avcodec_thread_init(codec_context_, kDecodeThreads) >= 0 &&
On 2010/03/11 19:59:19, fbarchard wrote:
> this bit breaks for ffmpeg that does not support threads
> i wonder if this is a good time to be more explicite about that part of inits.

> fail or warn if threads fails with a more clear failure compared to open()

we can handle that in a separate change -- the code hasn't changed so if it was
broken before it'll still be broken :)

http://codereview.chromium.org/859001/diff/5001/6009
File third_party/ffmpeg/avcodec-52.def (right):

http://codereview.chromium.org/859001/diff/5001/6009#newcode15
third_party/ffmpeg/avcodec-52.def:15: av_lockmgr_register
I actually don't think this file is used anymore

I'll delete them ;)

http://codereview.chromium.org/859001/diff/5001/6010
File third_party/ffmpeg/avcodec-52.sigs (right):

http://codereview.chromium.org/859001/diff/5001/6010#newcode1
third_party/ffmpeg/avcodec-52.sigs:1: # Copyright (c) 2009 The Chromium Authors.
All rights reserved.
could you update copyright year to 2010?

http://codereview.chromium.org/859001/diff/5001/6010#newcode25
third_party/ffmpeg/avcodec-52.sigs:25: int av_lockmgr_register(AVLockMgrFunc
cb);
I'm guessing generate_sigs.py doesn't work with function pointers?

http://codereview.chromium.org/859001/diff/5001/6012
File third_party/ffmpeg/ffmpeg_stub_headers.fragment (right):

http://codereview.chromium.org/859001/diff/5001/6012#newcode9
third_party/ffmpeg/ffmpeg_stub_headers.fragment:9: typedef int
(*AVLockMgrFunc)(void **mutex, enum AVLockOp op);
On 2010/03/11 19:59:19, fbarchard wrote:
> why is this prototyped?
> 

pointer goes with type

I'm guessing generate_stubs.py doesn't handle function pointer syntax, hence the
typedef

[programadoresargentinos_yahoo.com.ar, 2010-03-11 21:22:02]

BlackBerry® de Personal, tu mundo en tus manos.

-----Original Message-----
From: scherkus@chromium.org
Date: Thu, 11 Mar 2010 21:20:40 
To: <sergeyu@chromium.org>; <fbarchard@chromium.org>
Cc: <chromium-reviews@googlegroups.com>; <ajwong@chromium.org>;
<hclam@chromium.org>
Subject: [chromium-reviews] Re: Removed custom FFmpegLock. Removed ffmpeg 
	headers from third_party/ffmpeg/include. (issue859001)

so far looking good!

we should do the include directory switch as a separate patch, land that,  
make
sure everything still works, then land this


http://codereview.chromium.org/859001/diff/5001/6002
File media/base/mock_ffmpeg.h (right):

http://codereview.chromium.org/859001/diff/5001/6002#newcode24
media/base/mock_ffmpeg.h:24: MOCK_METHOD1(AVRegisterLockManager, int(int
(*cb)(void **, enum AVLockOp)));
pointers go with types

Good: void** foo
Bad: void **foo

http://codereview.chromium.org/859001/diff/5001/6003
File media/ffmpeg/ffmpeg_common.cc (left):

http://codereview.chromium.org/859001/diff/5001/6003#oldcode17
media/ffmpeg/ffmpeg_common.cc:17: }
On 2010/03/11 19:59:19, fbarchard wrote:
> can you explain?  are we moving away from an ffmpeg global lock?

sergey is replacing our ad-hoc global lock with FFmpeg's API

http://code.google.com/p/chromium/issues/detail?id=23271

http://codereview.chromium.org/859001/diff/5001/6004
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/859001/diff/5001/6004#newcode24
media/ffmpeg/ffmpeg_common.h:24: #include
"third_party/ffmpeg/source/patched-ffmpeg-mt/libavutil/log.h"
On 2010/03/11 19:59:19, fbarchard wrote:
> hmmm... i'm not sure if this is a good idea, because it makes windows
and linux
> do it differently.
> could you make windows src/DEPS pull in the source so the includes can
be done
> the same way on all platforms?

that should probably be done in a separate change before this one lands

http://codereview.chromium.org/859001/diff/5001/6005
File media/filters/ffmpeg_audio_decoder.cc (left):

http://codereview.chromium.org/859001/diff/5001/6005#oldcode65
media/filters/ffmpeg_audio_decoder.cc:65: AutoLock
auto_lock(FFmpegLock::get()->lock());
On 2010/03/11 19:59:19, fbarchard wrote:
> cool... locks are evil.  nice job

they're not going away.. avcodec_open() will call the lock instead :)

http://codereview.chromium.org/859001/diff/5001/6006
File media/filters/ffmpeg_demuxer.cc (left):

http://codereview.chromium.org/859001/diff/5001/6006#oldcode238
media/filters/ffmpeg_demuxer.cc:238: AutoLock
auto_lock(FFmpegLock::get()->lock());
On 2010/03/11 19:59:19, fbarchard wrote:
> nice.  can you update the todo?  is this still needed?


no need, TODO applies to the following code not the locking

http://codereview.chromium.org/859001/diff/5001/6007
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/859001/diff/5001/6007#newcode84
media/filters/ffmpeg_glue.cc:84: int LockManagerFunc(void **lock, enum
AVLockOp op) {
pointer goes with type

LockManagerFunc is a little short on description .. maybe
LockManagerOperation?

http://codereview.chromium.org/859001/diff/5001/6007#newcode86
media/filters/ffmpeg_glue.cc:86: case AV_LOCK_CREATE:
indent inside of switch {} by two spaces

http://codereview.chromium.org/859001/diff/5001/6007#newcode87
media/filters/ffmpeg_glue.cc:87: *lock = new Lock();
On 2010/03/11 19:59:19, fbarchard wrote:
> new... hmmm..  check with Andrew but I dont think we use new typically

in this instance we'll be using new and expecting FFmpeg to call
AV_LOCK_DESTROY during shutdown, which will call delete

http://codereview.chromium.org/859001/diff/5001/6008
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/859001/diff/5001/6008#newcode52
media/filters/ffmpeg_video_decode_engine.cc:52:
avcodec_thread_init(codec_context_, kDecodeThreads) >= 0 &&
On 2010/03/11 19:59:19, fbarchard wrote:
> this bit breaks for ffmpeg that does not support threads
> i wonder if this is a good time to be more explicite about that part
of inits.
> fail or warn if threads fails with a more clear failure compared to
open()

we can handle that in a separate change -- the code hasn't changed so if
it was broken before it'll still be broken :)

http://codereview.chromium.org/859001/diff/5001/6009
File third_party/ffmpeg/avcodec-52.def (right):

http://codereview.chromium.org/859001/diff/5001/6009#newcode15
third_party/ffmpeg/avcodec-52.def:15: av_lockmgr_register
I actually don't think this file is used anymore

I'll delete them ;)

http://codereview.chromium.org/859001/diff/5001/6010
File third_party/ffmpeg/avcodec-52.sigs (right):

http://codereview.chromium.org/859001/diff/5001/6010#newcode1
third_party/ffmpeg/avcodec-52.sigs:1: # Copyright (c) 2009 The Chromium
Authors. All rights reserved.
could you update copyright year to 2010?

http://codereview.chromium.org/859001/diff/5001/6010#newcode25
third_party/ffmpeg/avcodec-52.sigs:25: int
av_lockmgr_register(AVLockMgrFunc cb);
I'm guessing generate_sigs.py doesn't work with function pointers?

http://codereview.chromium.org/859001/diff/5001/6012
File third_party/ffmpeg/ffmpeg_stub_headers.fragment (right):

http://codereview.chromium.org/859001/diff/5001/6012#newcode9
third_party/ffmpeg/ffmpeg_stub_headers.fragment:9: typedef int
(*AVLockMgrFunc)(void **mutex, enum AVLockOp op);
On 2010/03/11 19:59:19, fbarchard wrote:
> why is this prototyped?


pointer goes with type

I'm guessing generate_stubs.py doesn't handle function pointer syntax,
hence the typedef

http://codereview.chromium.org/859001

[Sergey Ulanov, 2010-03-12 00:46:20]

http://codereview.chromium.org/859001/diff/5001/6002
File media/base/mock_ffmpeg.h (right):

http://codereview.chromium.org/859001/diff/5001/6002#newcode24
media/base/mock_ffmpeg.h:24: MOCK_METHOD1(AVRegisterLockManager, int(int
(*cb)(void **, enum AVLockOp)));
On 2010/03/11 19:59:19, fbarchard wrote:
> were you able to test this?
Yes, media_unittests pass with this change.

http://codereview.chromium.org/859001/diff/5001/6010
File third_party/ffmpeg/avcodec-52.sigs (right):

http://codereview.chromium.org/859001/diff/5001/6010#newcode25
third_party/ffmpeg/avcodec-52.sigs:25: int av_lockmgr_register(AVLockMgrFunc
cb);
On 2010/03/11 21:20:40, scherkus wrote:
> I'm guessing generate_sigs.py doesn't work with function pointers?
Yes. I tried to put function type here, and generate_stubs wasn't able to parse
it correctly. Fixing generate_stubs  to parse all cases properly would be rather
difficult: essentially it would require implementing C parser.

[scherkus (not reviewing), 2010-03-12 00:58:03]

LGTM w/ some nits

since they're mostly cosmetic nits I'll send your patch to the try servers

http://codereview.chromium.org/859001/diff/19001/20001
File DEPS (right):

http://codereview.chromium.org/859001/diff/19001/20001#newcode125
DEPS:125: "src/third_party/ffmpeg/source/patched-ffmpeg-mt":
instead of having this duplicated for mac/linux/win I think we can safely move
it to the os-independent section above

http://codereview.chromium.org/859001/diff/19001/20002
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/859001/diff/19001/20002#newcode97
media/base/mock_ffmpeg.cc:97: // mock may be NULL when this function is called
from ~FFmpegGlue
this isn't mentioned in the google style guide, but we generally format our
comments to keep the codebase consistent:
  - comments start with capitial letters
  - comments end with periods
  - functions are suffixed with () to make them easier to spot
  - variables are surrounded with | to make them easier to spot

we've had problems with this code before leaking memory (singletons + teardown =
mystery!!) so for fun and learning you may want to run media_unittests through
valgrind

I think the command is:
/src/tools/valgrind/chrome_tests.sh -t media

[scherkus (not reviewing), 2010-03-12 01:05:01]

watch for your try results here:
http://build.chromium.org/buildbot/try-server/waterfall?reload=120

build is named "sergey-34300"

[scherkus (not reviewing), 2010-03-24 00:40:52]

Committed (again) as r42415