Enable audio/video tag in content_shell

Enable audio/video tag in content_shell

Some media initialization work needed to be added to the shell in order for
the ffmpeg stubs to be loaded and for the media tags to be recognized.

AudioManager also needed to be added to the shell, as the Chromium-equivalent
lives in BrowserProcessImpl, and the media code should not assume the
MediaInternals has been created.

BUG=112043, 116906
TEST=content_shell builds and can play the videos in the bug above

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

[vrk (LEFT CHROMIUM), 2012-02-02 22:57:26]

erg/jam: content/shell stuff
scherkus: everything!

[vrk (LEFT CHROMIUM), 2012-02-02 23:17:26]

On 2012/02/02 22:57:26, Victoria Kirst wrote:
> erg/jam: content/shell stuff
> scherkus: everything!

BTW: You may notice that this CL only initializes the media library for POSIX. I
did that because content_shell only builds on linux at the moment.

What should I do about content_shell for other OSes? In Chrome,
InitializeMediaLibrary gets called in ChromeRendererProcessObserver to do the
initialization for non-linux systems, and so for shell, the equivalent calls
would presumably go in ShellRenderProcessObserver. Doesn't look like there's a
way for me to test this though. Should I put the calls there anyway?

[jam, 2012-02-03 00:49:44]

content_shell works on windows btw for a few months now. we didn't advertise it
heavily because we were waiting for it to work on linux first, since that's
where most of the devs are :)

i think my comments in the code should address your question of where to do it
on windows.

http://codereview.chromium.org/9316077/diff/1/content/shell/shell_browser_con...
File content/shell/shell_browser_context.h (right):

http://codereview.chromium.org/9316077/diff/1/content/shell/shell_browser_con...
content/shell/shell_browser_context.h:14: #include "media/audio/audio_manager.h"
nit: this include shouldn't be needed here, just a forward declaration

http://codereview.chromium.org/9316077/diff/1/content/shell/shell_main_delega...
File content/shell/shell_main_delegate.cc (right):

http://codereview.chromium.org/9316077/diff/1/content/shell/shell_main_delega...
content/shell/shell_main_delegate.cc:71: FilePath media_path;
in general, we want embedders to have to do as least amount of work as possible.
so can you move this initialization code from chrome to content (i.e. in the
content zygote/renderer initialization code), that way all embedders get it for
free.

http://codereview.chromium.org/9316077/diff/1/content/shell/shell_render_proc...
File content/shell/shell_render_process_observer.cc (right):

http://codereview.chromium.org/9316077/diff/1/content/shell/shell_render_proc...
content/shell/shell_render_process_observer.cc:28:
WebRuntimeFeatures::enableMediaPlayer(media::IsMediaLibraryInitialized());
ditto

[scherkus (not reviewing), 2012-02-03 23:40:01]

LGTM w/ one q

+tommi/henrika as an FYI (vrk is getting audio working inside of content_shell
-- exciting!)

https://chromiumcodereview.appspot.com/9316077/diff/1/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/9316077/diff/1/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:498: return
media_observer_;
I forget why this was coded like this but I wonder whether it's worth having
this member variable at all -- seems like a good source of potential bugs in the
future!

WDYT?

[vrk (LEFT CHROMIUM), 2012-03-06 02:07:48]

Dusting off this CL! Updated to ToT and I have things working on linux and
windows, I believe. PTAL!

It's not ready to commit yet though -- Mac's base_unittests fail. I added a
comment in base_paths_mac.mm identifying the problematic additions.

jam: Do you know what I'm doing wrong?

Thanks!

https://chromiumcodereview.appspot.com/9316077/diff/1/content/browser/rendere...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/9316077/diff/1/content/browser/rendere...
content/browser/renderer_host/media/audio_renderer_host.cc:498: return
media_observer_;
On 2012/02/03 23:40:01, scherkus wrote:
> I forget why this was coded like this but I wonder whether it's worth having
> this member variable at all -- seems like a good source of potential bugs in
the
> future!
> 
> WDYT?

(as discussed offline) You're right it should be removed, but it's a little
risky to do so right now. Since it's not high-priority, let's wait till M20. I
created crbug.com/116906 and commented in the header file.

https://chromiumcodereview.appspot.com/9316077/diff/1/content/shell/shell_bro...
File content/shell/shell_browser_context.h (right):

https://chromiumcodereview.appspot.com/9316077/diff/1/content/shell/shell_bro...
content/shell/shell_browser_context.h:14: #include "media/audio/audio_manager.h"
On 2012/02/03 00:49:44, John Abd-El-Malek wrote:
> nit: this include shouldn't be needed here, just a forward declaration

Done.

https://chromiumcodereview.appspot.com/9316077/diff/1/content/shell/shell_mai...
File content/shell/shell_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/9316077/diff/1/content/shell/shell_mai...
content/shell/shell_main_delegate.cc:71: FilePath media_path;
On 2012/02/03 00:49:44, John Abd-El-Malek wrote:
> in general, we want embedders to have to do as least amount of work as
possible.
> so can you move this initialization code from chrome to content (i.e. in the
> content zygote/renderer initialization code), that way all embedders get it
for
> free.

Done.

https://chromiumcodereview.appspot.com/9316077/diff/1/content/shell/shell_ren...
File content/shell/shell_render_process_observer.cc (right):

https://chromiumcodereview.appspot.com/9316077/diff/1/content/shell/shell_ren...
content/shell/shell_render_process_observer.cc:28:
WebRuntimeFeatures::enableMediaPlayer(media::IsMediaLibraryInitialized());
On 2012/02/03 00:49:44, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9316077/diff/18017/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

https://chromiumcodereview.appspot.com/9316077/diff/18017/base/base_paths_mac...
base/base_paths_mac.mm:84: case base::DIR_MEDIA_LIBS: {
This is failing the base_unittests PathServiceTest.Get() test on Mac. Any ideas
on what I'm doing wrong?

[scherkus (not reviewing), 2012-03-06 02:14:06]

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/DEPS
File content/DEPS (right):

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/DEPS#newcode28
content/DEPS:28: "+media",
nit: could we get a tighter scope?

AFAIK content/renderer/DEPS and content/browser/DEPS contain media entries so
perhaps it's content/app/DEPS that requires a media entry?

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
File content/app/content_main_runner.cc (right):

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
content/app/content_main_runner.cc:12: #include "base/path_service.h"
a->z ordering

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
content/app/content_main_runner.cc:205: // Each Renderer we spawn will
re-attempt initialization of the media
indent

[scherkus (not reviewing), 2012-03-06 02:19:00]

https://chromiumcodereview.appspot.com/9316077/diff/18017/base/base_paths_mac.mm
File base/base_paths_mac.mm (right):

https://chromiumcodereview.appspot.com/9316077/diff/18017/base/base_paths_mac...
base/base_paths_mac.mm:84: case base::DIR_MEDIA_LIBS: {
On 2012/03/06 02:07:50, Victoria Kirst wrote:
> This is failing the base_unittests PathServiceTest.Get() test on Mac. Any
ideas
> on what I'm doing wrong?

my only guess would be that ReturnsValidPath() is returning false for
file_util::PathExists(path)

perhaps base_unittests doesn't have a framework bundle path? (not sure -- not a
mac expert!)

[jam, 2012-03-06 02:20:54]

very cool, thanks for working on this. if audio/video folks can work on
content_shell and not have to build all of chrome, it makes our work more
worthwhile :)

I'm not sure I know why the base tests are failing, you can check with our
content Mac expert Avi (cc'd). it might be related to my next point

It seems like a layering violation that the base module has to know about media.
Why not put the path for media libraries in content_paths.h? that might fix your
base test in case it has to do with the fact that the base unittest target on
mac doesn't have a LIbraries subdir but content does (or will, after
https://chromiumcodereview.appspot.com/9513006/?).

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
File content/app/content_main_runner.cc (right):

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
content/app/content_main_runner.cc:206: // libraries, at which point failure
will be detected and handled, so
nit: spacing

[vrk (LEFT CHROMIUM), 2012-03-06 20:34:14]

Thanks guys!

On 2012/03/06 02:20:54, John Abd-El-Malek wrote:
> It seems like a layering violation that the base module has to know about
media.
> Why not put the path for media libraries in content_paths.h? that might fix
your
> base test in case it has to do with the fact that the base unittest target on
> mac doesn't have a LIbraries subdir but content does (or will, after
> https://chromiumcodereview.appspot.com/9513006/?).

Oh awesome! Yes I agree, content_paths makes a lot more sense. Made the change
and mac trybots appear to be happy with it!

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/DEPS
File content/DEPS (right):

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/DEPS#newcode28
content/DEPS:28: "+media",
On 2012/03/06 02:14:06, scherkus wrote:
> nit: could we get a tighter scope?
> 
> AFAIK content/renderer/DEPS and content/browser/DEPS contain media entries so
> perhaps it's content/app/DEPS that requires a media entry?

Done.

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
File content/app/content_main_runner.cc (right):

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
content/app/content_main_runner.cc:12: #include "base/path_service.h"
On 2012/03/06 02:14:06, scherkus wrote:
> a->z ordering

Done.

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
content/app/content_main_runner.cc:205: // Each Renderer we spawn will
re-attempt initialization of the media
On 2012/03/06 02:14:06, scherkus wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/9316077/diff/18017/content/app/content...
content/app/content_main_runner.cc:206: // libraries, at which point failure
will be detected and handled, so
On 2012/03/06 02:20:54, John Abd-El-Malek wrote:
> nit: spacing

Done.

[scherkus (not reviewing), 2012-03-06 20:36:59]

LGTM w/ one nit

https://chromiumcodereview.appspot.com/9316077/diff/30001/content/app/DEPS
File content/app/DEPS (right):

https://chromiumcodereview.appspot.com/9316077/diff/30001/content/app/DEPS#ne...
content/app/DEPS:3: "+media",
nit: could this be scoped even tighter w/ a comment? :)

"+media/base",  # For initializing media library.

[vrk (LEFT CHROMIUM), 2012-03-06 21:18:36]

https://chromiumcodereview.appspot.com/9316077/diff/30001/content/app/DEPS
File content/app/DEPS (right):

https://chromiumcodereview.appspot.com/9316077/diff/30001/content/app/DEPS#ne...
content/app/DEPS:3: "+media",
On 2012/03/06 20:37:00, scherkus wrote:
> nit: could this be scoped even tighter w/ a comment? :)
> 
> "+media/base",  # For initializing media library.

SGTM :) Done and thanks!

[jam, 2012-03-06 21:26:17]

lgtm, nice

http://codereview.chromium.org/9316077/diff/29014/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/9316077/diff/29014/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:258: void
ChromeRenderProcessObserver::WebKitInitialized() {
nit: delete this function since it's now empty

http://codereview.chromium.org/9316077/diff/29014/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

http://codereview.chromium.org/9316077/diff/29014/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:388: void
AudioRendererHost::OnSetAudioStreamPlaying(int stream_id, bool playing) {
nit: it seems that these batch of functions aren't necessary.. 

if (GetMedia..)
  GetMedia..->

above seems quicker to read

[vrk (LEFT CHROMIUM), 2012-03-06 22:11:41]

scherkus: Sorry, PTAL at AudioRendererHost! I discovered that the
media_observer_ field was more harmful than I thought... and that actually it
was causing content_shell to occasionally segfault on shutdown :|

The media_observer_ field stores a cached copy of the MediaObserver from
resource_context_, under the (safe-for-now) assumption that MediaObserver
outlives the AudioRendererHost.

However, this makes two additional assumptions that are not safe: 1) The
ResourceContext is valid the first time GetMediaObserver() is called, and 2) the
ResourceContext always returns a non-null MediaObserver. Since assumption 2) is
wrong for content_shell, we end up once again facing the crbug.com/91397 bug. (I
can explain offline if that doesn't make sense.)

My fix described in crbug.com/116906 seems to fix things and I added it to this
CL.

http://codereview.chromium.org/9316077/diff/29014/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/9316077/diff/29014/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:258: void
ChromeRenderProcessObserver::WebKitInitialized() {
On 2012/03/06 21:26:17, John Abd-El-Malek wrote:
> nit: delete this function since it's now empty

Done.

http://codereview.chromium.org/9316077/diff/29014/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

http://codereview.chromium.org/9316077/diff/29014/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.cc:388: void
AudioRendererHost::OnSetAudioStreamPlaying(int stream_id, bool playing) {
On 2012/03/06 21:26:17, John Abd-El-Malek wrote:
> nit: it seems that these batch of functions aren't necessary.. 
> 
> if (GetMedia..)
>   GetMedia..->
> 
> above seems quicker to read

SGTM, thanks! Done.

[scherkus (not reviewing), 2012-03-06 22:14:17]

LGTM++

[jam, 2012-03-06 22:17:38]

slgtm

[commit-bot: I haz the power, 2012-03-07 18:38:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/9316077/34020

[commit-bot: I haz the power, 2012-03-07 20:33:41]

Change committed as 125442