Decouple 'give me more data' and 'rendered end of stream' audio callbacks.

Decouple 'give me more data' and 'rendered end of stream' audio callbacks.
Make 'rendered end of stream' cllback to be called by the delayed task.
That is in the preparation of software audio mixer, it will ask for data
at the time unrelated to the actual playback.

[enal1, 2012-02-07 19:42:06]

And here is promised CL, with (hopefully) correct handling of seek/pause after
we scheduled the task but before it actually worked.

[scherkus (not reviewing), 2012-02-08 02:10:40]

I'm a bit confused. Why do we need to execute SignalEndOfStream() on the IO
thread?

The .h claims OnRenderEndOfStream() is called on pipeline thread and
documentation for SignalEndOfStream() does not state that it must be called on
the IO thread.

Is the .h incorrect?

https://chromiumcodereview.appspot.com/9347029/diff/3002/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9347029/diff/3002/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:275: delay_ms);
PostDelayedTask() now supports base::TimeDelta and new code should be written
using that version

[enal1, 2012-02-08 02:43:22]

Pipeline message loop is not readily available, and it is not available to
caller as well. You'll have to pass it through several levels of objects.

Callback is lightweight, and code is thread-safe -- much more than I initially
feared, so it is safe to use different message loop.

https://chromiumcodereview.appspot.com/9347029/diff/3002/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9347029/diff/3002/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:275: delay_ms);
Not MessageLoopProxy::PostDelayedTask(). It is for milliseconds only.

[scherkus (not reviewing), 2012-02-09 04:52:50]

apologies as I'm still a bit confused here!

is there a bug/issue that this CL addresses? with the branch cut does this need
to be merged seeing as your other change made it into the branch?

also while this code appears to be thread safe, it looks like
AudioRendererBase::SignalEndOfStream() isn't locking when reading/writing
rendered_end_of_stream_

is this a temporary fix or something that should go away with additional mixer
changes?

https://chromiumcodereview.appspot.com/9347029/diff/3002/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9347029/diff/3002/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:275: delay_ms);
On 2012/02/08 02:43:22, enal1 wrote:
> Not MessageLoopProxy::PostDelayedTask(). It is for milliseconds only.

Looks like MLProxy slipped through the cracks!
http://code.google.com/p/chromium/issues/detail?id=108171#c59

[enal1, 2012-02-09 19:13:22]

On 2012/02/09 04:52:50, scherkus wrote:
> apologies as I'm still a bit confused here!
> 
> is there a bug/issue that this CL addresses? with the branch cut does this
need
> to be merged seeing as your other change made it into the branch?

We absolutely need that change for software mixer, because callback for data
becomes absolutely unrelated to the playback.

It is very useful CL even now (especially on Mac, where we don't use length of
data in buffer), because we would signal 'ended' much earlier. Right now browser
would ask for data from time to time, and during one of those callbacks we would
figure out it's time, and  signal. That easily can be 20ms or more after we stop
playback (there is no audible problems because we are feeding zeroes to the OS).

On my favorite test -- playing 146ms file 100 times, re-starting playback on
'ended' event -- new code works ~15.1 seconds, vs. ~17-18 for old ones. You can
easily hear the difference.

I'd prefer the code not to go into new branch immediately, so we can figure out
all potential problems on the canary builds.

> also while this code appears to be thread safe, it looks like
> AudioRendererBase::SignalEndOfStream() isn't locking when reading/writing
> rendered_end_of_stream_

Fixed that (and other) potential threading issues.

> is this a temporary fix or something that should go away with additional mixer
> changes?
> 
>
https://chromiumcodereview.appspot.com/9347029/diff/3002/content/renderer/med...
> File content/renderer/media/audio_renderer_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9347029/diff/3002/content/renderer/med...
> content/renderer/media/audio_renderer_impl.cc:275: delay_ms);
> On 2012/02/08 02:43:22, enal1 wrote:
> > Not MessageLoopProxy::PostDelayedTask(). It is for milliseconds only.
> 
> Looks like MLProxy slipped through the cracks!
> http://code.google.com/p/chromium/issues/detail?id=108171#c59

Will use them when they appear...

[scherkus (not reviewing), 2012-02-23 05:54:51]

my biggest concern with this CL going in as is is that if I'm having trouble
understanding the atomic-word-based-thread-safe-task-cancellation now, how
understandable will this be a year from now? there's a reason it's under the
subtle namespace :)

I'd really like to understand what the "proper fix" is in this case -- I can
think of two ideas:
  1) Add a ThreadSafeCancelableCallback to src/base/ and re-implement this
change using that so it's clearer what's going on
  2) Determine proper threading semantics and invest the time to detangle this
code so we can use the existing cancelable callback under base

[Ami GONE FROM CHROMIUM, 2012-02-23 20:18:25]

drive-by triggered by scherkus@' mention of "subtle"

https://chromiumcodereview.appspot.com/9347029/diff/10003/content/renderer/me...
File content/renderer/media/audio_renderer_impl.h (right):

https://chromiumcodereview.appspot.com/9347029/diff/10003/content/renderer/me...
content/renderer/media/audio_renderer_impl.h:165: base::subtle::Atomic32
stream_id_;
I might be missing something, but ISTM this member is terribly misused.  
Atomic32 is just an int32 under the covers.

++'ing an Atomic32 is not atomic (two concurrent ++'s of a 0-valued Atomic32 can
result in a final value of 1, for example).

The later test of this member:
if (stream_id == stream_id_)
looks like another race since:
a) it's just an int32 to the compiler, so there are no memory fences around that
read, nor volatile semantics, so the value read may not reflect updates in other
threads
b) nothing guarantees that stream_id_ won't change in another thread after
reading it and before acting on it.

Is there a reason a lock can't be used to guard a simple state-tracking int32?

(or, better yet, obviously, make the threading story explicit in this class
hierarchy and avoid touching state from multiple threads, but that's a much
bigger undertaking :))

[enal1, 2012-02-23 20:28:04]

https://chromiumcodereview.appspot.com/9347029/diff/10003/content/renderer/me...
File content/renderer/media/audio_renderer_impl.h (right):

https://chromiumcodereview.appspot.com/9347029/diff/10003/content/renderer/me...
content/renderer/media/audio_renderer_impl.h:165: base::subtle::Atomic32
stream_id_;
On 2012/02/23 20:18:25, Ami Fischman wrote:
> I might be missing something, but ISTM this member is terribly misused.  
> Atomic32 is just an int32 under the covers.
> 
> ++'ing an Atomic32 is not atomic (two concurrent ++'s of a 0-valued Atomic32
can
> result in a final value of 1, for example).
> 
> The later test of this member:
> if (stream_id == stream_id_)
> looks like another race since:
> a) it's just an int32 to the compiler, so there are no memory fences around
that
> read, nor volatile semantics, so the value read may not reflect updates in
other
> threads
> b) nothing guarantees that stream_id_ won't change in another thread after
> reading it and before acting on it.
> 
> Is there a reason a lock can't be used to guard a simple state-tracking int32?
> 
> (or, better yet, obviously, make the threading story explicit in this class
> hierarchy and avoid touching state from multiple threads, but that's a much
> bigger undertaking :))

++stream_id_ happens only on the pipeline thread, so there is no race condition
here.

There was race condition between seek/signal end of stream anyways, lack of
memory barrier/lock does not make it worse.

Anyways, I am rewriting the code to make Andrew happy and make code more robust
and simpler to understand...

[enal1, 2012-02-24 19:03:27]

Rewrote the code to
(1) Use cancelable callback, 
(2) Use pipeline thread instead of I/O one,
(3) Use locks instead of atomics,
(4) Added new unit test.

[scherkus (not reviewing), 2012-03-03 02:24:32]

I'm really wary of landing this patch. It looks like a lot of the trickiness
stems from AudioRendererBase having ill-defined threading policy.

How urgent is it for this to land in M19? It seems like fixing
http://crbug.com/116645 first as well as cleaning up the locking/threading
policy prior to landing this fix would be better (I think this is on vrk's
plate)

https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/me...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/me...
content/renderer/media/audio_renderer_impl.cc:193:
base::Bind(&AudioRendererImpl::DoSignalEndOfStream, AsWeakPtr()));
are weak pointers enforced by cancellable callbacks?

seems strange considering AudioRendererImpl is refcounted

https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/me...
content/renderer/media/audio_renderer_impl.cc:200:
DoSetOrCheckMessageLoopProxy();
I would strongly prefer having pipeline set the message loop as opposed to
hijacking it via these methods.

https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/me...
content/renderer/media/audio_renderer_impl.cc:288: if (delay_ms <= 0) {
if the delay time was calculated to be <=0 wouldn't we want to call PDT with a
time of 0? otherwise the task will get put behind whatever other tasks are
queued when using PostTask

in which case using std::max(0, delay) would work

https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/me...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/me...
content/renderer/media/audio_renderer_impl_unittest.cc:228:
TEST_F(AudioRendererImplTest, OnRenderEndOfStream) {
These tests rely way too much on internal knowledge of the class.

The fact that we're adding virtual methods, protected members, and tests with
friend access is a sign that things have started to go awry! The next person who
touches AudioRendererImpl will spend more time updating tests than writing
production code.

It'd be preferable to eliminate the virtual/protected methods and write these
tests using higher-level methods such as Render() and DecodedAudioReady() -- for
example you can set recieved_end_of_stream_ to true by passing in an EOS Buffer
to DecodedAudioReady()

Failing that -- we could also check in your HTML test as a layout test and do
without C++ unit tests

[enal1, 2012-03-05 17:24:00]

It's not urgent, and it probably can wait till code refactoring is
complete. I also don't like the resulting code, but I didn't want to go and
change everything to clean things up, I have mixer to implement -- after
all, on all-hands meeting we were told that using less resources is highest
priority :-)

I hoped we can speed up things a bit before worsening latency when mixer
would be ready (do your remember saying "what andy gath bill taketh away"?)

Thanks,
Eugene

On Fri, Mar 2, 2012 at 6:24 PM, <scherkus@chromium.org> wrote:

> I'm really wary of landing this patch. It looks like a lot of the
> trickiness
> stems from AudioRendererBase having ill-defined threading policy.
>
> How urgent is it for this to land in M19? It seems like fixing
> http://crbug.com/116645 first as well as cleaning up the locking/threading
> policy prior to landing this fix would be better (I think this is on vrk's
> plate)
>
>
> https://chromiumcodereview.**appspot.com/9347029/diff/**
>
18001/content/renderer/media/**audio_renderer_impl.cc<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl.cc>
> File content/renderer/media/audio_**renderer_impl.cc (right):
>
> https://chromiumcodereview.**appspot.com/9347029/diff/**
>
18001/content/renderer/media/**audio_renderer_impl.cc#**newcode193<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl.cc#newcode193>
> content/renderer/media/audio_**renderer_impl.cc:193:
> base::Bind(&AudioRendererImpl:**:DoSignalEndOfStream, AsWeakPtr()));
> are weak pointers enforced by cancellable callbacks?
>
> seems strange considering AudioRendererImpl is refcounted
>
> https://chromiumcodereview.**appspot.com/9347029/diff/**
>
18001/content/renderer/media/**audio_renderer_impl.cc#**newcode200<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl.cc#newcode200>
> content/renderer/media/audio_**renderer_impl.cc:200:
> DoSetOrCheckMessageLoopProxy()**;
> I would strongly prefer having pipeline set the message loop as opposed
> to hijacking it via these methods.
>
> https://chromiumcodereview.**appspot.com/9347029/diff/**
>
18001/content/renderer/media/**audio_renderer_impl.cc#**newcode288<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl.cc#newcode288>
> content/renderer/media/audio_**renderer_impl.cc:288: if (delay_ms <= 0) {
> if the delay time was calculated to be <=0 wouldn't we want to call PDT
> with a time of 0? otherwise the task will get put behind whatever other
> tasks are queued when using PostTask
>
> in which case using std::max(0, delay) would work
>
> https://chromiumcodereview.**appspot.com/9347029/diff/**
>
18001/content/renderer/media/**audio_renderer_impl_unittest.**cc<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl_unittest.cc>
> File content/renderer/media/audio_**renderer_impl_unittest.cc (right):
>
> https://chromiumcodereview.**appspot.com/9347029/diff/**
> 18001/content/renderer/media/**audio_renderer_impl_unittest.**
>
cc#newcode228<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl_unittest.cc#newcode228>
> content/renderer/media/audio_**renderer_impl_unittest.cc:228:
> TEST_F(AudioRendererImplTest, OnRenderEndOfStream) {
> These tests rely way too much on internal knowledge of the class.
>
> The fact that we're adding virtual methods, protected members, and tests
> with friend access is a sign that things have started to go awry! The
> next person who touches AudioRendererImpl will spend more time updating
> tests than writing production code.
>
> It'd be preferable to eliminate the virtual/protected methods and write
> these tests using higher-level methods such as Render() and
> DecodedAudioReady() -- for example you can set recieved_end_of_stream_
> to true by passing in an EOS Buffer to DecodedAudioReady()
>
> Failing that -- we could also check in your HTML test as a layout test
> and do without C++ unit tests
>
>
https://chromiumcodereview.**appspot.com/9347029/<https://chromiumcodereview....
>

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

On 2012/03/05 17:24:00, enal1 wrote:
> It's not urgent, and it probably can wait till code refactoring is
> complete. I also don't like the resulting code, but I didn't want to go and
> change everything to clean things up, I have mixer to implement -- after
> all, on all-hands meeting we were told that using less resources is highest
> priority :-)
> 
> I hoped we can speed up things a bit before worsening latency when mixer
> would be ready (do your remember saying "what andy gath bill taketh away"?)
> 
> Thanks,
> Eugene

Thanks for the details Eugene!

We'll put this CL on hold for now but I do think we should use your test page to
benchmark our end-to-end latency and improve said latency after some of the code
has been cleaned up.

I've filed the following bug with the details + a benchmark:
http://code.google.com/p/chromium/issues/detail?id=117314

Andrew
 
> On Fri, Mar 2, 2012 at 6:24 PM, <mailto:scherkus@chromium.org> wrote:
> 
> > I'm really wary of landing this patch. It looks like a lot of the
> > trickiness
> > stems from AudioRendererBase having ill-defined threading policy.
> >
> > How urgent is it for this to land in M19? It seems like fixing
> > http://crbug.com/116645 first as well as cleaning up the locking/threading
> > policy prior to landing this fix would be better (I think this is on vrk's
> > plate)
> >
> >
> > https://chromiumcodereview.**appspot.com/9347029/diff/**
> >
>
18001/content/renderer/media/**audio_renderer_impl.cc<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl.cc>
> > File content/renderer/media/audio_**renderer_impl.cc (right):
> >
> > https://chromiumcodereview.**appspot.com/9347029/diff/**
> >
>
18001/content/renderer/media/**audio_renderer_impl.cc#**newcode193<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl.cc#newcode193>
> > content/renderer/media/audio_**renderer_impl.cc:193:
> > base::Bind(&AudioRendererImpl:**:DoSignalEndOfStream, AsWeakPtr()));
> > are weak pointers enforced by cancellable callbacks?
> >
> > seems strange considering AudioRendererImpl is refcounted
> >
> > https://chromiumcodereview.**appspot.com/9347029/diff/**
> >
>
18001/content/renderer/media/**audio_renderer_impl.cc#**newcode200<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl.cc#newcode200>
> > content/renderer/media/audio_**renderer_impl.cc:200:
> > DoSetOrCheckMessageLoopProxy()**;
> > I would strongly prefer having pipeline set the message loop as opposed
> > to hijacking it via these methods.
> >
> > https://chromiumcodereview.**appspot.com/9347029/diff/**
> >
>
18001/content/renderer/media/**audio_renderer_impl.cc#**newcode288<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl.cc#newcode288>
> > content/renderer/media/audio_**renderer_impl.cc:288: if (delay_ms <= 0) {
> > if the delay time was calculated to be <=0 wouldn't we want to call PDT
> > with a time of 0? otherwise the task will get put behind whatever other
> > tasks are queued when using PostTask
> >
> > in which case using std::max(0, delay) would work
> >
> > https://chromiumcodereview.**appspot.com/9347029/diff/**
> >
>
18001/content/renderer/media/**audio_renderer_impl_unittest.**cc<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl_unittest.cc>
> > File content/renderer/media/audio_**renderer_impl_unittest.cc (right):
> >
> > https://chromiumcodereview.**appspot.com/9347029/diff/**
> > 18001/content/renderer/media/**audio_renderer_impl_unittest.**
> >
>
cc#newcode228<https://chromiumcodereview.appspot.com/9347029/diff/18001/content/renderer/media/audio_renderer_impl_unittest.cc#newcode228>
> > content/renderer/media/audio_**renderer_impl_unittest.cc:228:
> > TEST_F(AudioRendererImplTest, OnRenderEndOfStream) {
> > These tests rely way too much on internal knowledge of the class.
> >
> > The fact that we're adding virtual methods, protected members, and tests
> > with friend access is a sign that things have started to go awry! The
> > next person who touches AudioRendererImpl will spend more time updating
> > tests than writing production code.
> >
> > It'd be preferable to eliminate the virtual/protected methods and write
> > these tests using higher-level methods such as Render() and
> > DecodedAudioReady() -- for example you can set recieved_end_of_stream_
> > to true by passing in an EOS Buffer to DecodedAudioReady()
> >
> > Failing that -- we could also check in your HTML test as a layout test
> > and do without C++ unit tests
> >
> >
>
https://chromiumcodereview.**appspot.com/9347029/%3Chttps://chromiumcoderevie...>
> >