Plumb media data from renderers up to MediaInternals in the browser process.

Plumb media data from renderers up to MediaInternals in the browser process.

Relanding 95542. Reverted due to issue caused by 95496.

BUG=none
TEST=manually

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

[Scott Franklin, 2011-07-27 00:08:23]

Sorry for the size of this one.

This adds the starts of a MediaLog (similar to NetLog, but in the renderer
process and, well, for media). A new MediaLog is created and attached to each
WebMediaPlayerImpl, and it will pass any events up through the hierarchy of
abstractions, across to the browser process, and up into MediaInternals, where
they can be analyzed and the results sent off to be displayed by
MediaInternalsUI.

media_log.h is in media/base so that WebMediaPlayerImpl can pass its MediaLog
down to filters, etc., which can then post events as well.

Using the pid was the best method I could come up with for uniquely identifying
a MediaLog. I'm sure there's a renderer id of some sort somewhere, but I was
unable to locate it.

[jam, 2011-07-29 16:08:55]

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_log_host.cc (right):

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_log_host.cc:17:
BrowserMessageFilter::OnChannelClosing();
you don't need to override this

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_log_host.cc:21:
BrowserThread::DeleteOnIOThread::Destruct(this);
ditto. there's no reason that I see that makes you care about which thread this
object is destructed on

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_log_host.cc:28:
IPC_MESSAGE_HANDLER(MediaLogMsg_MediaEvent, OnMediaEvent)
given that you have just one message, it seems easiest to just dispatch this in
RenderMessageFilter

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_log_host.h (right):

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_log_host.h:18: public:
nit: spacing

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_log_host.h:31: // Handle
MediaLogMsg_MediaEvent, passing the event off to the MediaObserver.
nit: we usually don't comment message handlers. esp we don't describe what the
code is doing, since someone can see that by looking at the implementation

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_observer.h (right):

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_observer.h:9: #include
"media/base/media_log.h"
the C++ style guide advises against inner classes specifically for this reason,
i.e. they can't be forward declared.

MediaLog::Event should be in its own header and named MediaLogEvent

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
File content/renderer/media/content_media_log.cc (right):

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
content/renderer/media/content_media_log.cc:19: if (MessageLoop::current() ==
render_loop_) {
you don't need to store the message loop pointer. You can check
RenderThread::current() to know if you're on the main thread or not

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
File content/renderer/media/content_media_log.h (right):

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
content/renderer/media/content_media_log.h:15: class ContentMediaLog : public
media::MediaLog {
we don't name any classes starting with "Content". This should just be
RenderMediaLog

http://codereview.chromium.org/7480032/diff/2001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/7480032/diff/2001/ipc/ipc_message_utils.h#newc...
ipc/ipc_message_utils.h:85: MediaLogMsgStart,
nit: people usually just add at the end

http://codereview.chromium.org/7480032/diff/2001/media/base/media_event_types.h
File media/base/media_event_types.h (right):

http://codereview.chromium.org/7480032/diff/2001/media/base/media_event_types...
media/base/media_event_types.h:5: // A list of media events. This file is
included by media_log.h multiple times,
given that there are 5 entries, and all you want is an enum and a string,
there's no need to have this multiple-include business. in cases like this we
just have a function that has a switch around the enum, and the default has a
NOTREACHED to catch the case when people add a new one.

http://codereview.chromium.org/7480032/diff/2001/media/base/media_log.h
File media/base/media_log.h (right):

http://codereview.chromium.org/7480032/diff/2001/media/base/media_log.h#newco...
media/base/media_log.h:25: base::ProcessId renderer;
media module shouldn't know about different chrome process types, so this should
just be "pid".

[Scott Franklin, 2011-07-29 22:30:25]

Thanks, this is quite a bit simpler without the dedicated message handler.

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_log_host.cc (right):

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_log_host.cc:17:
BrowserMessageFilter::OnChannelClosing();
On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> you don't need to override this

This appeared to be standard practice in every other *host file I looked at,
though I suppose they all had something to clean up.

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_log_host.cc:21:
BrowserThread::DeleteOnIOThread::Destruct(this);
On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> ditto. there's no reason that I see that makes you care about which thread
this
> object is destructed on

Ditto.

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_log_host.cc:28:
IPC_MESSAGE_HANDLER(MediaLogMsg_MediaEvent, OnMediaEvent)
On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> given that you have just one message, it seems easiest to just dispatch this
in
> RenderMessageFilter

Yeah, I was expecting many messages, but then one message with an enum turned
out to be much simpler.

It is much easier. Done.

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_observer.h (right):

http://codereview.chromium.org/7480032/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/media/media_observer.h:9: #include
"media/base/media_log.h"
On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> the C++ style guide advises against inner classes specifically for this
reason,
> i.e. they can't be forward declared.
> 
> MediaLog::Event should be in its own header and named MediaLogEvent

Done.

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
File content/renderer/media/content_media_log.cc (right):

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
content/renderer/media/content_media_log.cc:19: if (MessageLoop::current() ==
render_loop_) {
On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> you don't need to store the message loop pointer. You can check
> RenderThread::current() to know if you're on the main thread or not

Right, but I then have no way of posting the task to the main thread. Unless
there is some other way to get a hold of its message loop?

If so, I can stop storing it, and the DCHECK above can go.

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
File content/renderer/media/content_media_log.h (right):

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
content/renderer/media/content_media_log.h:15: class ContentMediaLog : public
media::MediaLog {
On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> we don't name any classes starting with "Content". This should just be
> RenderMediaLog

Done.

http://codereview.chromium.org/7480032/diff/2001/media/base/media_event_types.h
File media/base/media_event_types.h (right):

http://codereview.chromium.org/7480032/diff/2001/media/base/media_event_types...
media/base/media_event_types.h:5: // A list of media events. This file is
included by media_log.h multiple times,
On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> given that there are 5 entries, and all you want is an enum and a string,
> there's no need to have this multiple-include business. in cases like this we
> just have a function that has a switch around the enum, and the default has a
> NOTREACHED to catch the case when people add a new one.

Sure, I'll fill it in manually. It can always be split off easily if the number
of events grows too large (I'm expecting only 20 at most) or appears in more
places (probably unlikely).

The NOTREACHED() below the switch instead of as a default seems to be better, as
gcc throws an error if you fail to handle a case.

http://codereview.chromium.org/7480032/diff/2001/media/base/media_log.h
File media/base/media_log.h (right):

http://codereview.chromium.org/7480032/diff/2001/media/base/media_log.h#newco...
media/base/media_log.h:25: base::ProcessId renderer;
On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> media module shouldn't know about different chrome process types, so this
should
> just be "pid". 

Or, since the messages are now being handled by RenderMessageFilter (which has a
process identifier), it can just be removed altogether.

[jam, 2011-08-01 06:03:25]

lgtm with nits

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
File content/renderer/media/content_media_log.cc (right):

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
content/renderer/media/content_media_log.cc:19: if (MessageLoop::current() ==
render_loop_) {
On 2011/07/29 22:30:25, Scott Franklin wrote:
> On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> > you don't need to store the message loop pointer. You can check
> > RenderThread::current() to know if you're on the main thread or not
> 
> Right, but I then have no way of posting the task to the main thread. Unless
> there is some other way to get a hold of its message loop?
> 
> If so, I can stop storing it, and the DCHECK above can go.

of course, nm them :) although please use MessageLoopProxy instead of
MessageLoop, which is safer in case the MessageLoop object is destructing at
process shutdown. you can get it through
MessgeLoopProxy::CreateForCurrentThread()

http://codereview.chromium.org/7480032/diff/10001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_observer.h (right):

http://codereview.chromium.org/7480032/diff/10001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_observer.h:36: virtual void
OnMediaEvent(int source, const media::MediaLogEvent& event) = 0;
nit: please rename source to render_process_id, to be consistent with other
places which is what we always name it

http://codereview.chromium.org/7480032/diff/10001/content/renderer/media/rend...
File content/renderer/media/render_media_log.h (right):

http://codereview.chromium.org/7480032/diff/10001/content/renderer/media/rend...
content/renderer/media/render_media_log.h:16: public:
nit: spacing (and below)

http://codereview.chromium.org/7480032/diff/10001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7480032/diff/10001/content/renderer/render_vie...
content/renderer/render_view.cc:52: #include
"content/renderer/media/render_media_log.h"
nit: order

http://codereview.chromium.org/7480032/diff/10001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7480032/diff/10001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:16: #include "media/base/media_log.h"
nit: order

[Scott Franklin, 2011-08-01 18:41:03]

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
File content/renderer/media/content_media_log.cc (right):

http://codereview.chromium.org/7480032/diff/2001/content/renderer/media/conte...
content/renderer/media/content_media_log.cc:19: if (MessageLoop::current() ==
render_loop_) {
On 2011/08/01 06:03:25, John Abd-El-Malek wrote:
> On 2011/07/29 22:30:25, Scott Franklin wrote:
> > On 2011/07/29 16:08:55, John Abd-El-Malek wrote:
> > > you don't need to store the message loop pointer. You can check
> > > RenderThread::current() to know if you're on the main thread or not
> > 
> > Right, but I then have no way of posting the task to the main thread. Unless
> > there is some other way to get a hold of its message loop?
> > 
> > If so, I can stop storing it, and the DCHECK above can go.
> 
> of course, nm them :) although please use MessageLoopProxy instead of
> MessageLoop, which is safer in case the MessageLoop object is destructing at
> process shutdown. you can get it through
> MessgeLoopProxy::CreateForCurrentThread()

Done.

http://codereview.chromium.org/7480032/diff/10001/content/browser/renderer_ho...
File content/browser/renderer_host/media/media_observer.h (right):

http://codereview.chromium.org/7480032/diff/10001/content/browser/renderer_ho...
content/browser/renderer_host/media/media_observer.h:36: virtual void
OnMediaEvent(int source, const media::MediaLogEvent& event) = 0;
On 2011/08/01 06:03:25, John Abd-El-Malek wrote:
> nit: please rename source to render_process_id, to be consistent with other
> places which is what we always name it

Done.

http://codereview.chromium.org/7480032/diff/10001/content/renderer/media/rend...
File content/renderer/media/render_media_log.h (right):

http://codereview.chromium.org/7480032/diff/10001/content/renderer/media/rend...
content/renderer/media/render_media_log.h:16: public:
On 2011/08/01 06:03:25, John Abd-El-Malek wrote:
> nit: spacing (and below)

Done.

http://codereview.chromium.org/7480032/diff/10001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7480032/diff/10001/content/renderer/render_vie...
content/renderer/render_view.cc:52: #include
"content/renderer/media/render_media_log.h"
On 2011/08/01 06:03:25, John Abd-El-Malek wrote:
> nit: order

Done.

http://codereview.chromium.org/7480032/diff/10001/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7480032/diff/10001/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:16: #include "media/base/media_log.h"
On 2011/08/01 06:03:25, John Abd-El-Malek wrote:
> nit: order

Done.

[acolwell GONE FROM CHROMIUM, 2011-08-03 16:31:37]

LGTM

http://codereview.chromium.org/7480032/diff/12007/media/base/media_log.cc
File media/base/media_log.cc (right):

http://codereview.chromium.org/7480032/diff/12007/media/base/media_log.cc#new...
media/base/media_log.cc:12: static int32 media_log_count = 0;
Do we need to worry about thread safety here? If so then perhaps you should use
this instead.
static base::AtomicSequenceNumber media_log_count(base::LINKER_INITIALIZED);

http://codereview.chromium.org/7480032/diff/12007/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7480032/diff/12007/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:428:
media_log_->AddEventOfType(media::MediaLogEvent::DESTROY);
Is this intended to indicate when destruction has started or after it has
completed. I'm only asking because Destroy() may cause log events to be
generated during pipeline shutdown.

[Scott Franklin, 2011-08-03 20:55:03]

http://codereview.chromium.org/7480032/diff/12007/media/base/media_log.cc
File media/base/media_log.cc (right):

http://codereview.chromium.org/7480032/diff/12007/media/base/media_log.cc#new...
media/base/media_log.cc:12: static int32 media_log_count = 0;
On 2011/08/03 16:31:37, acolwell wrote:
> Do we need to worry about thread safety here? If so then perhaps you should
use
> this instead.
> static base::AtomicSequenceNumber media_log_count(base::LINKER_INITIALIZED);

In practice, no, MediaLogs are only created on the render thread; there's a
DCHECK to that effect in RenderMediaLog.

That said, I suppose it could become an issue in the future (I'll need instances
for tests, etc.), and it doesn't really hurt anything to add a bit of
synchronization.

Done.

http://codereview.chromium.org/7480032/diff/12007/webkit/glue/webmediaplayer_...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7480032/diff/12007/webkit/glue/webmediaplayer_...
webkit/glue/webmediaplayer_impl.cc:428:
media_log_->AddEventOfType(media::MediaLogEvent::DESTROY);
On 2011/08/03 16:31:37, acolwell wrote:
> Is this intended to indicate when destruction has started or after it has
> completed. I'm only asking because Destroy() may cause log events to be
> generated during pipeline shutdown.

Starting destruction (I'll change it to DESTROYING).

Getting a final DESTRUCTED message will be a somewhat difficult process because
the teardown is happening across multiple threads. Perhaps I can force the
destruction of RenderMediaLog on the main render thread and log it there?
Either way, that's a topic for a different patch.

[commit-bot: I haz the power, 2011-08-04 16:44:54]

Can't process patch for file chrome/browser/media/DEPS.
A +

[commit-bot: I haz the power, 2011-08-04 21:36:57]

Can't apply patch for file media/media.gyp.
While running patch -p1 --forward --force;
patching file media/media.gyp
Hunk #1 FAILED at 104.
1 out of 1 hunk FAILED -- saving rejects to file media/media.gyp.rej

[commit-bot: I haz the power, 2011-08-05 00:11:52]

Change committed as 95542

[commit-bot: I haz the power, 2011-08-05 23:26:41]

Change committed as 95685

[scherkus (not reviewing), 2011-08-08 21:20:51]

tiny tiny post-commit drive by stuff

http://codereview.chromium.org/7480032/diff/37001/content/common/DEPS
File content/common/DEPS (right):

http://codereview.chromium.org/7480032/diff/37001/content/common/DEPS#newcode2
content/common/DEPS:2: "+media/base", # For media_log_event.h.
I actually found out you can specify an exact .h file if that helps at all

http://codereview.chromium.org/7480032/diff/37001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7480032/diff/37001/content/common/view_message...
content/common/view_messages.h:176: IPC_ENUM_TRAITS(media::MediaLogEvent::Type)
nit: this looks like it should be in AZ order

http://codereview.chromium.org/7480032/diff/37001/content/common/view_message...
content/common/view_messages.h:413:
IPC_STRUCT_TRAITS_BEGIN(media::MediaLogEvent)
possibly ditto for this