WebMediaPlayerImpl needs to own the audio source provider.

WebMediaPlayerImpl needs to own the audio source provider.

WebMediaPlayerImpl needs to own the audio source provider to keep it from being destroyed too soon.

BUG=132890
TEST=See test in bug report.


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

[scherkus (not reviewing), 2012-06-27 04:42:42]

this is layering violation free but is a bit unfortunate that we introduce an
extra pointer that is essentially a casted pointer!

Here's where I'm a bit confused. The RenderAudioSourceProvider *should* be kept
alive via the following ownership chain:

WebMediaPlayerImpl::pipeline_ --> Pipeline::audio_renderer_ -->
AudioRendererImpl::sink_

I believe the bug is that Pipeline actually destroys ARI during page navigation
before ~WMPI() executes.

Are we interested in a small/quick fix for a branch while myself or someone else
fixes the root cause? If so, we could either:
  1) Cast audio_source_provider to media::AudioRendererSink*, knowing that it's
the same object and add the ref as a member in WMPI (instead of plumbing extra
arg)
  2) Peek at the AudioRenderer object inside filter_collection and hold onto
that ref in WMPI (knowing that it also holds onto the sink)

WDYT?

[Raymond Toy (Google), 2012-06-27 16:45:38]

On Tue, Jun 26, 2012 at 9:42 PM, <scherkus@chromium.org> wrote:

> this is layering violation free but is a bit unfortunate that we introduce
> an
> extra pointer that is essentially a casted pointer!
>
> Here's where I'm a bit confused. The RenderAudioSourceProvider *should* be
> kept
> alive via the following ownership chain:
>
> WebMediaPlayerImpl::pipeline_ --> Pipeline::audio_renderer_ -->
> AudioRendererImpl::sink_
>
> I believe the bug is that Pipeline actually destroys ARI during page
> navigation
> before ~WMPI() executes.
>

Yes, that is the bug.  WMPI just holds a pointer to audio_source_provider,
so it doesn't keep it alive.

>
> Are we interested in a small/quick fix for a branch while myself or
> someone else
> fixes the root cause? If so, we could either:
>  1) Cast audio_source_provider to media::AudioRendererSink*, knowing that
> it's
> the same object and add the ref as a member in WMPI (instead of plumbing
> extra
> arg)
>  2) Peek at the AudioRenderer object inside filter_collection and hold onto
> that ref in WMPI (knowing that it also holds onto the sink)
>
> WDYT?
>
>
https://chromiumcodereview.**appspot.com/10662030/<https://chromiumcodereview...
>

[Chris Rogers, 2012-06-27 17:35:27]

On 2012/06/27 04:42:42, scherkus wrote:
> this is layering violation free but is a bit unfortunate that we introduce an
> extra pointer that is essentially a casted pointer!
> 
> Here's where I'm a bit confused. The RenderAudioSourceProvider *should* be
kept
> alive via the following ownership chain:
> 
> WebMediaPlayerImpl::pipeline_ --> Pipeline::audio_renderer_ -->
> AudioRendererImpl::sink_
> 
> I believe the bug is that Pipeline actually destroys ARI during page
navigation
> before ~WMPI() executes.
> 
> Are we interested in a small/quick fix for a branch while myself or someone
else
> fixes the root cause? 

Hi Andrew, after thinking this over a bit, I think it might be more fragile to
change Pipeline in order to fix this bug.  That is, unless you think it's a good
idea to create an obligation on Pipeline to keep the filters alive for the
lifetime of the pipeline itself (which you'd be in a better position to judge).

I'm more confident about the WMPI having direct ownership, and think your option
(1) is a good solution.


If so, we could either:
>   1) Cast audio_source_provider to media::AudioRendererSink*, knowing that
it's
> the same object and add the ref as a member in WMPI (instead of plumbing extra
> arg)
>   2) Peek at the AudioRenderer object inside filter_collection and hold onto
> that ref in WMPI (knowing that it also holds onto the sink)
> 
> WDYT?

[Raymond Toy (Google), 2012-06-27 22:23:20]

On 2012/06/27 04:42:42, scherkus wrote:
> this is layering violation free but is a bit unfortunate that we introduce an
> extra pointer that is essentially a casted pointer!
> 
> Here's where I'm a bit confused. The RenderAudioSourceProvider *should* be
kept
> alive via the following ownership chain:
> 
> WebMediaPlayerImpl::pipeline_ --> Pipeline::audio_renderer_ -->
> AudioRendererImpl::sink_
> 
> I believe the bug is that Pipeline actually destroys ARI during page
navigation
> before ~WMPI() executes.
> 
> Are we interested in a small/quick fix for a branch while myself or someone
else
> fixes the root cause? If so, we could either:
>   1) Cast audio_source_provider to media::AudioRendererSink*, knowing that
it's
> the same object and add the ref as a member in WMPI (instead of plumbing extra
> arg)

In WMPI, audio_source_provider is a WebAudioSourceProvider*, which is not a
media::AudioRendererSink*, so this doesn't work.


>   2) Peek at the AudioRenderer object inside filter_collection and hold onto
> that ref in WMPI (knowing that it also holds onto the sink)
> 
> WDYT?

[Raymond Toy (Google), 2012-06-27 22:46:55]

On 2012/06/27 22:23:20, rtoy wrote:
> On 2012/06/27 04:42:42, scherkus wrote:
> > this is layering violation free but is a bit unfortunate that we introduce
an
> > extra pointer that is essentially a casted pointer!
> > 
> > Here's where I'm a bit confused. The RenderAudioSourceProvider *should* be
> kept
> > alive via the following ownership chain:
> > 
> > WebMediaPlayerImpl::pipeline_ --> Pipeline::audio_renderer_ -->
> > AudioRendererImpl::sink_
> > 
> > I believe the bug is that Pipeline actually destroys ARI during page
> navigation
> > before ~WMPI() executes.
> > 
> > Are we interested in a small/quick fix for a branch while myself or someone
> else
> > fixes the root cause? If so, we could either:
> >   1) Cast audio_source_provider to media::AudioRendererSink*, knowing that
> it's
> > the same object and add the ref as a member in WMPI (instead of plumbing
extra
> > arg)
> 
> In WMPI, audio_source_provider is a WebAudioSourceProvider*, which is not a
> media::AudioRendererSink*, so this doesn't work.

Less briefly, audio_source_provider is a RenderAudioSourceProvider that is
derived from two different interface classes: WebKit::WebAudioSourceProvider and
media::AudioRendererSink.  WMPI has a pointer to WebKit::WebAudioSourceProvider,
so we can't cast that to a pointer to the other interface type.

> 
> 
> >   2) Peek at the AudioRenderer object inside filter_collection and hold onto
> > that ref in WMPI (knowing that it also holds onto the sink)
> > 
> > WDYT?

[scherkus (not reviewing), 2012-07-09 23:13:23]

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/rendere...
File content/public/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/rendere...
content/public/renderer/DEPS:3: "+media/base/audio_renderer_sink.h",
I think you can get rid of this if you fix fwd decl

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/rendere...
content/public/renderer/content_renderer_client.h:15: #include
"media/base/audio_renderer_sink.h"
don't include -- just typedef in media namespace below

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
File webkit/media/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:136: media_log_->AddEvent(
can you DCHECK_EQ() that the two pointers are equal + comment why?

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
File webkit/media/webmediaplayer_impl.h (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.h:77: class AudioRendererSink;
a->z ordering

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.h:107: // audio_renderer_sink arguments should
be the same object.
add || around variable names referenced in comments

add a TODO(scherkus) saying "Remove WebAudioSourceProvider parameter once we
refactor RenderAudioSourceProvider to live under webkit/media/ instead of
content/renderer/, see http://crbug.com/136442"

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.h:346: // the same as audio_source_provider_.
I'd remove the comment -- it's OK to assume this class should hold onto a
pointer (i.e,. this comment will quickly become stale)

In fact it's better to DCHECK this inside the .cc + comment there

[Raymond Toy, 2012-07-10 02:51:39]

Review comments taken care of, new patch will be uploaded shortly.

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/rendere...
File content/public/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/rendere...
content/public/renderer/DEPS:3: "+media/base/audio_renderer_sink.h",
On 2012/07/09 23:13:23, scherkus wrote:
> I think you can get rid of this if you fix fwd decl

Done.

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/rendere...
content/public/renderer/content_renderer_client.h:15: #include
"media/base/audio_renderer_sink.h"
On 2012/07/09 23:13:23, scherkus wrote:
> don't include -- just typedef in media namespace below

Done.

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
File webkit/media/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:136: media_log_->AddEvent(
On 2012/07/09 23:13:23, scherkus wrote:
> can you DCHECK_EQ() that the two pointers are equal + comment why?

No, I can't do that because they're pointers derived from two different classes.
 I even tried casting each to void*, but they're different, due to how classes
are laid out.

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
File webkit/media/webmediaplayer_impl.h (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.h:77: class AudioRendererSink;
On 2012/07/09 23:13:23, scherkus wrote:
> a->z ordering

Done.

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.h:107: // audio_renderer_sink arguments should
be the same object.
On 2012/07/09 23:13:23, scherkus wrote:
> add || around variable names referenced in comments
> 
> add a TODO(scherkus) saying "Remove WebAudioSourceProvider parameter once we
> refactor RenderAudioSourceProvider to live under webkit/media/ instead of
> content/renderer/, see http://crbug.com/136442%22

Done.

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.h:346: // the same as audio_source_provider_.
On 2012/07/09 23:13:23, scherkus wrote:
> I'd remove the comment -- it's OK to assume this class should hold onto a
> pointer (i.e,. this comment will quickly become stale)
> 
> In fact it's better to DCHECK this inside the .cc + comment there

Comment removed.  But see comment for .cc where we can't DCHECK_EQ the pointers.

[ivan_cobs.com.sg, 2012-07-10 03:47:00]

Pls remove the following email adds from your list NOW

ivan@cobs.com.sg
lynn@cobs.com.sg

-----Original Message-----
From: rtoy@chromium.org [mailto:rtoy@chromium.org] 
Sent: Tuesday, 10 July, 2012 10:52 AM
To: rtoy@google.com; crogers@google.com; scherkus@chromium.org
Cc: chromium-reviews@chromium.org; jochen+watch-content@chromium.org;
jam@chromium.org; joi+watch-content@chromium.org;
feature-media-reviews@chromium.org; darin-cc@chromium.org;
brettw-cc@chromium.org; pam+watch@chromium.org
Subject: [chromium-reviews] Re: WebMediaPlayerImpl needs to own the audio
source provider. (issue 10662030)

Review comments taken care of, new patch will be uploaded shortly.


https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/render
er/DEPS
File content/public/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/render
er/DEPS#newcode3
content/public/renderer/DEPS:3: "+media/base/audio_renderer_sink.h",
On 2012/07/09 23:13:23, scherkus wrote:
> I think you can get rid of this if you fix fwd decl

Done.

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/render
er/content_renderer_client.h
File content/public/renderer/content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/content/public/render
er/content_renderer_client.h#newcode15
content/public/renderer/content_renderer_client.h:15: #include
"media/base/audio_renderer_sink.h"
On 2012/07/09 23:13:23, scherkus wrote:
> don't include -- just typedef in media namespace below

Done.

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmedia
player_impl.cc
File webkit/media/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmedia
player_impl.cc#newcode136
webkit/media/webmediaplayer_impl.cc:136: media_log_->AddEvent( On 2012/07/09
23:13:23, scherkus wrote:
> can you DCHECK_EQ() that the two pointers are equal + comment why?

No, I can't do that because they're pointers derived from two different
classes.  I even tried casting each to void*, but they're different, due to
how classes are laid out.

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmedia
player_impl.h
File webkit/media/webmediaplayer_impl.h (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmedia
player_impl.h#newcode77
webkit/media/webmediaplayer_impl.h:77: class AudioRendererSink; On
2012/07/09 23:13:23, scherkus wrote:
> a->z ordering

Done.

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmedia
player_impl.h#newcode107
webkit/media/webmediaplayer_impl.h:107: // audio_renderer_sink arguments
should be the same object.
On 2012/07/09 23:13:23, scherkus wrote:
> add || around variable names referenced in comments

> add a TODO(scherkus) saying "Remove WebAudioSourceProvider parameter
once we
> refactor RenderAudioSourceProvider to live under webkit/media/ instead
of
> content/renderer/, see http://crbug.com/136442%22

Done.

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmedia
player_impl.h#newcode346
webkit/media/webmediaplayer_impl.h:346: // the same as
audio_source_provider_.
On 2012/07/09 23:13:23, scherkus wrote:
> I'd remove the comment -- it's OK to assume this class should hold
onto a
> pointer (i.e,. this comment will quickly become stale)

> In fact it's better to DCHECK this inside the .cc + comment there

Comment removed.  But see comment for .cc where we can't DCHECK_EQ the
pointers.

https://chromiumcodereview.appspot.com/10662030/

[scherkus (not reviewing), 2012-07-10 17:13:50]

LGTM w/ one nit for fwd decl instead of #include

thanks raymond!

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
File webkit/media/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/10662030/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:136: media_log_->AddEvent(
On 2012/07/10 02:51:39, rtoy1 wrote:
> On 2012/07/09 23:13:23, scherkus wrote:
> > can you DCHECK_EQ() that the two pointers are equal + comment why?
> 
> No, I can't do that because they're pointers derived from two different
classes.
>  I even tried casting each to void*, but they're different, due to how classes
> are laid out.

Right -- that makes sense :\

https://chromiumcodereview.appspot.com/10662030/diff/3003/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/10662030/diff/3003/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.h:16: #include
"media/base/audio_renderer_sink.h"
fwd decl instead

[Raymond Toy, 2012-07-10 17:38:17]

All review comments fixed.

http://codereview.chromium.org/10662030/diff/3003/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/10662030/diff/3003/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.h:16: #include
"media/base/audio_renderer_sink.h"
On 2012/07/10 17:13:51, scherkus wrote:
> fwd decl instead

Done.

[commit-bot: I haz the power, 2012-07-10 21:00:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/10662030/3005

[commit-bot: I haz the power, 2012-07-10 21:00:08]

Presubmit check for 10662030-3005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit/support
    webkit
    content/public
    content/renderer

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[piman, 2012-07-10 22:40:40]

lgtm

[Jói, 2012-07-10 22:50:17]

LGTM for content/public.

On Tue, Jul 10, 2012 at 10:40 PM,  <piman@chromium.org> wrote:
> lgtm
>
>
>
> https://chromiumcodereview.appspot.com/10662030/

[tkent, 2012-07-10 23:37:17]

LGTM for webkit/support.

> TEST=See test in bug report. 

Will you add a LayoutTest?

[Raymond Toy, 2012-07-11 16:16:26]

On 2012/07/10 23:37:17, Kent Tamura wrote:
> LGTM for webkit/support.
> 
> > TEST=See test in bug report. 
> 
> Will you add a LayoutTest?

Thanks for reviewing.  I had not planned on doing a LayoutTest.

[commit-bot: I haz the power, 2012-07-16 20:13:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/10662030/3005

[commit-bot: I haz the power, 2012-07-16 20:13:08]

Presubmit check for 10662030-3005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

Presubmit checks took 1.3s to calculate.

[scherkus (not reviewing), 2012-07-16 20:18:00]

tony can we get OWNERS for webkit/ ?

[tony, 2012-07-16 20:24:08]

webkit LGTM

[Raymond Toy, 2012-07-16 20:26:52]

On 2012/07/16 20:24:08, tony wrote:
> webkit LGTM

Thanks!

[commit-bot: I haz the power, 2012-07-16 20:27:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/10662030/3005

[commit-bot: I haz the power, 2012-07-16 21:46:15]

Change committed as 146897