Move media library path resolution into Chrome path provider.

Move media library path resolution into Chrome path provider.

BUG=None
TEST=None

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

[Wez, 2011-02-17 17:36:24]

Suggested change to have the path from which to load the media libraries
provided by the Chrome PathProvider.

This has the minor benefit of avoiding duplication of the Mac OS X specific path
logic between the render and service process implementations...

[scherkus (not reviewing), 2011-02-17 17:57:10]

drive by!

http://codereview.chromium.org/6537022/diff/1/chrome/browser/zygote_main_linu...
File chrome/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/6537022/diff/1/chrome/browser/zygote_main_linu...
chrome/browser/zygote_main_linux.cc:601: // to use them.  Should we store a flag
so code can check this?
we definitely shouldn't crash!

the renderer processes know if the media libraries haven't loaded

it's tricky but essentially we should self-disable anything media-related from
starting in that case

do you think that's possible for you guys?

http://codereview.chromium.org/6537022/diff/1/chrome/common/chrome_paths.h
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/6537022/diff/1/chrome/common/chrome_paths.h#ne...
chrome/common/chrome_paths.h:42: DIR_MEDIA_LIBS,               // Directory
where the Media libraries reside.
nit: Media -> media

http://codereview.chromium.org/6537022/diff/1/chrome/renderer/render_process_...
File chrome/renderer/render_process_impl.cc (right):

http://codereview.chromium.org/6537022/diff/1/chrome/renderer/render_process_...
chrome/renderer/render_process_impl.cc:137: #if !defined(OS_MACOSX)
I'm curious... I wonder if we can remove this restriction

[Wez, 2011-02-17 18:27:52]

On 17 February 2011 17:57, <scherkus@chromium.org> wrote:

> drive by!


(ducks for cover)


>
>
http://codereview.chromium.org/6537022/diff/1/chrome/browser/zygote_main_linu...
> File chrome/browser/zygote_main_linux.cc (right):
>
>
>
http://codereview.chromium.org/6537022/diff/1/chrome/browser/zygote_main_linu...
> chrome/browser/zygote_main_linux.cc:601: // to use them.  Should we
>
> store a flag so code can check this?
> we definitely shouldn't crash!


> the renderer processes know if the media libraries haven't loaded
>
> it's tricky but essentially we should self-disable anything
> media-related from starting in that case
>

Yes, I agree.

The first thing I'd suggest is having InitializeMediaLibrary() store a
process-wide global that code can query via
media::IsMediaLibraryInitialized(), so that individual components don't need
to handle a failure return-code, just so long as they check that before
doing any media work.

do you think that's possible for you guys?
>

Actually, this CL was one I mashed together because it seemed bad to have
media-library-DLL-path logic duplicated - it's not specific to what we're
doing, nor is it actually necessary as such.


>
>
http://codereview.chromium.org/6537022/diff/1/chrome/renderer/render_process_...
> File chrome/renderer/render_process_impl.cc (right):
>
>
>
http://codereview.chromium.org/6537022/diff/1/chrome/renderer/render_process_...
> chrome/renderer/render_process_impl.cc:137: #if !defined(OS_MACOSX)
> I'm curious... I wonder if we can remove this restriction


According to hclam (sat behind me right now, as it happens) the OpenMax
headers b0rk under the Xcode, and OpenMax is only there right now for
experimentation anyway.


> http://codereview.chromium.org/6537022/
>

[awong, 2011-02-23 21:29:07]

Agreed with Andrew's comments...especially about the CHECK failure.  That can't
go in as is since we need to support having the media libraries not exist.

[Wez, 2011-02-24 19:09:47]

Revised this CL to:
* Have InitializeMediaLibrary() store a flag indicating whether it succeeded.
* Restore the Renderer & Zygote's ability to run without media libraries.

[awong, 2011-02-25 02:41:59]

Got a number of nitpicks.

I'm not 100% convinced it's better for the module to store the status.  All
people who call the init code still have to check the IsMediaInitialized()
state, no?  We've just disassociated the status check from the function call,
which arguably feels more error prone.

What's the benefit you think we're getting?  I think we only save 2 state
variables...

http://codereview.chromium.org/6537022/diff/9002/chrome/service/service_proce...
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/6537022/diff/9002/chrome/service/service_proce...
chrome/service/service_process.cc:176: if
(PathService::Get(chrome::DIR_MEDIA_LIBS, &module_path))
Single-line ifs seem to be wrapped in braces in this file.  Wrap this in a brace
as well?

http://codereview.chromium.org/6537022/diff/9002/chrome/service/service_proce...
chrome/service/service_process.cc:344:
DISABLE_RUNNABLE_METHOD_REFCOUNT(ServiceProcess);
Gha...this needs in the headerfile after the class definition.  Please move?

http://codereview.chromium.org/6537022/diff/9002/content/browser/zygote_main_...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/6537022/diff/9002/content/browser/zygote_main_...
content/browser/zygote_main_linux.cc:602: // be sure to check
media::IsMediaLibraryInitialized() before doing so!
Exclamations are discouraged in comments :)

http://codereview.chromium.org/6537022/diff/9002/media/base/media_posix.cc
File media/base/media_posix.cc (right):

http://codereview.chromium.org/6537022/diff/9002/media/base/media_posix.cc#ne...
media/base/media_posix.cc:69: static bool g_media_library_is_initialized =
false;
If it's in an anonymous namespace, don't bother with static.

Pick one or the other. :)

http://codereview.chromium.org/6537022/diff/9002/media/base/media_win.cc
File media/base/media_win.cc (right):

http://codereview.chromium.org/6537022/diff/9002/media/base/media_win.cc#newc...
media/base/media_win.cc:90: g_media_library_is_initialized =
(libs[arraysize(libs)-1] != NULL);
Remove outer parens.  Not necessary.

http://codereview.chromium.org/6537022/diff/9002/media/tools/media_bench/medi...
File media/tools/media_bench/media_bench.cc (right):

http://codereview.chromium.org/6537022/diff/9002/media/tools/media_bench/medi...
media/tools/media_bench/media_bench.cc:126: if
(media::IsMediaLibraryInitialized() == false) {
if (!media::IsMediaLibraryInitialized()) {

[Wez, 2011-02-25 09:26:12]

On 2011/02/25 02:41:59, awong wrote:
> I'm not 100% convinced it's better for the module to store the status.  All
> people who call the init code still have to check the IsMediaInitialized()
> state, no?  We've just disassociated the status check from the function call,
> which arguably feels more error prone.
> 
> What's the benefit you think we're getting?  I think we only save 2 state
> variables...

I think these are two separate points. :)

I agree that separating the initialization call from the check does make it
easier to do one without the other.  I do think it's better for
InitializeMediaLibrary() to at least keep an initialized flag internally, rather
than relying on re-initialization being idempotent.  (I did mistakenly think
that the Zygote was already missing an initialization failure case, but of
course the failure is just picked up later by the Renderer when it
re-initialized the media library.)

I was originally trying to address the need for the path to the media library
files to be specified to InitializeMediaLibrary() - it feels like the call (or a
wrapper around it?) should be able to initialize based on
PathService::Get(chrome::DIR_MEDIA_LIBS, ...) so that calling code just has to
call that function, check the result (see above) and know that the world is a
sane place.  Tests can just override chrome::DIR_MEDIA_LIBS.

However, chrome::DIR_MEDIA_LIBS is currently a Chrome path in my patch (which I
think is right), whereas media is more generally-applicable, so placing that
dependency in InitializeMediaLibrary() doesn't make sense.

>
http://codereview.chromium.org/6537022/diff/9002/chrome/service/service_proce...
> File chrome/service/service_process.cc (right):
> 
>
http://codereview.chromium.org/6537022/diff/9002/chrome/service/service_proce...
> chrome/service/service_process.cc:176: if
> (PathService::Get(chrome::DIR_MEDIA_LIBS, &module_path))
> Single-line ifs seem to be wrapped in braces in this file.  Wrap this in a
brace
> as well?

Done (albeit implicitly).

>
http://codereview.chromium.org/6537022/diff/9002/chrome/service/service_proce...
> chrome/service/service_process.cc:344:
> DISABLE_RUNNABLE_METHOD_REFCOUNT(ServiceProcess);
> Gha...this needs in the headerfile after the class definition.  Please move?

Done.

>
http://codereview.chromium.org/6537022/diff/9002/content/browser/zygote_main_...
> File content/browser/zygote_main_linux.cc (right):
> 
>
http://codereview.chromium.org/6537022/diff/9002/content/browser/zygote_main_...
> content/browser/zygote_main_linux.cc:602: // be sure to check
> media::IsMediaLibraryInitialized() before doing so!
> Exclamations are discouraged in comments :)

I've updated the entire comment with my hopefully better-informed understanding
of what's going on here.

> http://codereview.chromium.org/6537022/diff/9002/media/base/media_posix.cc
> File media/base/media_posix.cc (right):
> 
>
http://codereview.chromium.org/6537022/diff/9002/media/base/media_posix.cc#ne...
> media/base/media_posix.cc:69: static bool g_media_library_is_initialized =
> false;
> If it's in an anonymous namespace, don't bother with static.
> 
> Pick one or the other. :)

Done.

> http://codereview.chromium.org/6537022/diff/9002/media/base/media_win.cc
> File media/base/media_win.cc (right):
> 
>
http://codereview.chromium.org/6537022/diff/9002/media/base/media_win.cc#newc...
> media/base/media_win.cc:90: g_media_library_is_initialized =
> (libs[arraysize(libs)-1] != NULL);
> Remove outer parens.  Not necessary.

This one I disagree with - "a = b != c" is harder to read than "a = (b != c)",
IMHO.

>
http://codereview.chromium.org/6537022/diff/9002/media/tools/media_bench/medi...
> File media/tools/media_bench/media_bench.cc (right):
> 
>
http://codereview.chromium.org/6537022/diff/9002/media/tools/media_bench/medi...
> media/tools/media_bench/media_bench.cc:126: if
> (media::IsMediaLibraryInitialized() == false) {
> if (!media::IsMediaLibraryInitialized()) {

Agreed - this was done in keeping with the way the original author had it.  That
said, I've reverted all the tests now, having restored the return value from
InitializeMediaLibrary().

[awong, 2011-03-16 15:10:52]

LGTM