Simplify AudioSplicer logic which slots buffers before or after a splice point.

Simplify AudioSplicer logic which slots buffers before or after a splice point.

Since the first post splice buffer after the config change has a splice_timestamp()
of kNoTimestamp() we can definitively say when we have the first post splice
buffer instead of having to relying on a problematic timestamp match.

The new code makes it so that clients must always call SetSpliceTimestamp() with
kNoTimestamp() once the first post splice buffer is received.

Adds tests to the AudioRendererImpl to verify behavior of the AudioBufferConverter
which impacts the splicer.

BUG=334493
TEST=existing tests pass.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264766

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264944

[DaleCurtis, 2014-04-17 01:13:35]

acolwell: Even though we discussed this, if you're overloaded I can pass this to
scherkus@ or wolenetz@. Thanks!

[acolwell GONE FROM CHROMIUM, 2014-04-17 17:30:09]

On 2014/04/17 01:13:35, DaleCurtis wrote:
> acolwell: Even though we discussed this, if you're overloaded I can pass this
to
> scherkus@ or wolenetz@. Thanks!

reassigning to wolenetz@ for review

[wolenetz, 2014-04-17 18:24:03]

My first round of comments is mostly just clarification questions:

https://codereview.chromium.org/240123004/diff/1/media/base/audio_splicer_uni...
File media/base/audio_splicer_unittest.cc (left):

https://codereview.chromium.org/240123004/diff/1/media/base/audio_splicer_uni...
media/base/audio_splicer_unittest.cc:719: TEST_F(AudioSplicerTest,
SpliceIncorrectlySlotted) {
I'm confused I think. Why is this test no longer applicable?

https://codereview.chromium.org/240123004/diff/1/media/filters/decoder_stream.cc
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/240123004/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.cc:166: decoder_selector_->Abort();
Probably unrelated to this CL, but should we also transition to STATE_STOPPED in
this code path somewhere? Or reset active_splice_?

https://codereview.chromium.org/240123004/diff/1/media/filters/decoder_stream.h
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/240123004/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.h:94: // the first buffer after which has a
splice_timestamp() of kNoTimestamp().
Clarification of my understanding: Is the intent here that SpliceObserverCB
(AudioRendererImpl::OnNewSpliceBuffer currently) is notified of each in sequence
of the splice timestamps for all pre-splice buffers followed by the
post-splice-buffer? Is the order of splice notification vs read_cb_ undocumented
here? (If so, nit: please add that documentation. My understanding is there
could be some decoder delay, but read_cb_ should be executed for the decoded
active splice buffer at some point in time *after* the associated
spliceobservercb was called. Is this correct? Could there be more than one
buffer's decoded contents passed to read_cb_ in the interim, and would the
renderer_impl take such into account?)

[DaleCurtis, 2014-04-17 20:03:18]

https://codereview.chromium.org/240123004/diff/1/media/base/audio_splicer_uni...
File media/base/audio_splicer_unittest.cc (left):

https://codereview.chromium.org/240123004/diff/1/media/base/audio_splicer_uni...
media/base/audio_splicer_unittest.cc:719: TEST_F(AudioSplicerTest,
SpliceIncorrectlySlotted) {
On 2014/04/17 18:24:03, wolenetz wrote:
> I'm confused I think. Why is this test no longer applicable?

It was necessary because we relied on the checking timestamp == splice_timestamp
to determine the splice point.  Now we rely on an explicit signal, so it's not
possible to miscategorize a buffer.

https://codereview.chromium.org/240123004/diff/1/media/filters/decoder_stream.cc
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/240123004/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.cc:166: decoder_selector_->Abort();
On 2014/04/17 18:24:03, wolenetz wrote:
> Probably unrelated to this CL, but should we also transition to STATE_STOPPED
in
> this code path somewhere? Or reset active_splice_?

Stop() is a tear down event.  STATE_STOPPED will be set in OnDecoderSelector()
after the DecoderSelector returns a NULL decoder and the code checks for a
stop_cb_.

https://codereview.chromium.org/240123004/diff/1/media/filters/decoder_stream.h
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/240123004/diff/1/media/filters/decoder_stream...
media/filters/decoder_stream.h:94: // the first buffer after which has a
splice_timestamp() of kNoTimestamp().
On 2014/04/17 18:24:03, wolenetz wrote:
> Clarification of my understanding: Is the intent here that SpliceObserverCB
> (AudioRendererImpl::OnNewSpliceBuffer currently) is notified of each in
sequence
> of the splice timestamps for all pre-splice buffers followed by the
> post-splice-buffer? Is the order of splice notification vs read_cb_
undocumented
> here? (If so, nit: please add that documentation. My understanding is there
> could be some decoder delay, but read_cb_ should be executed for the decoded
> active splice buffer at some point in time *after* the associated
> spliceobservercb was called. Is this correct? Could there be more than one
> buffer's decoded contents passed to read_cb_ in the interim, and would the
> renderer_impl take such into account?)

I'll fix the comment in StreamParserBuffer() to document this better.  The
documentation here is a statement of what will happen, not how splices are made.

Since the first post splice buffer happens after a config change, we don't need
to worry about any additional buffers in the queue when marking the end of pre
splice buffers.

[wolenetz, 2014-04-17 22:29:30]

looking pretty good. Thanks for clarifying. A couple more comments:

https://codereview.chromium.org/240123004/diff/20001/media/base/audio_splicer.h
File media/base/audio_splicer.h (right):

https://codereview.chromium.org/240123004/diff/20001/media/base/audio_splicer...
media/base/audio_splicer.h:56: // of the overlap range will be discarded.
This "discard" logic: where is it? I see that l.287 of audio_splicer.cc feeds
these to the output. By 'buffers', are decoded frames meant, instead (and the
discard happens in ExtractCrossfadeFromPreSplice()?)

https://codereview.chromium.org/240123004/diff/20001/media/base/audio_splicer...
File media/base/audio_splicer_unittest.cc (right):

https://codereview.chromium.org/240123004/diff/20001/media/base/audio_splicer...
media/base/audio_splicer_unittest.cc:575: EXPECT_TRUE(splicer_.HasNextBuffer());
nit: Why EXPECT, not ASSERT (or vice-versa elsewhere in these tests)?

[DaleCurtis, 2014-04-17 23:52:33]

https://codereview.chromium.org/240123004/diff/20001/media/base/audio_splicer.h
File media/base/audio_splicer.h (right):

https://codereview.chromium.org/240123004/diff/20001/media/base/audio_splicer...
media/base/audio_splicer.h:56: // of the overlap range will be discarded.
On 2014/04/17 22:29:30, wolenetz wrote:
> This "discard" logic: where is it? I see that l.287 of audio_splicer.cc feeds
> these to the output. By 'buffers', are decoded frames meant, instead (and the
> discard happens in ExtractCrossfadeFromPreSplice()?)

The discard of unused pre splice buffers happens on l.475 of audio_splicer.cc

https://codereview.chromium.org/240123004/diff/20001/media/base/audio_splicer...
File media/base/audio_splicer_unittest.cc (right):

https://codereview.chromium.org/240123004/diff/20001/media/base/audio_splicer...
media/base/audio_splicer_unittest.cc:575: EXPECT_TRUE(splicer_.HasNextBuffer());
On 2014/04/17 22:29:30, wolenetz wrote:
> nit: Why EXPECT, not ASSERT (or vice-versa elsewhere in these tests)?

Done.  I also removed cases where the logic was duplicated within the
VerifyXXX() function.  The ASSERT() is only necessary if the test is directly
extracting a buffer instead of using a VerifyXXX() function.

[wolenetz, 2014-04-18 00:53:40]

lgtm

[DaleCurtis, 2014-04-18 01:01:29]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-04-18 01:02:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/240123004/40001

[commit-bot: I haz the power, 2014-04-18 10:33:07]

Change committed as 264766

[DaleCurtis, 2014-04-18 19:41:41]

wolenetz: PTAL at patch set #4.  With the codec_delay() and duration changes a
new test failure was exposed:

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

The test failed because the AudioBufferConverter was holding onto buffers after
the config change, resulting splice buffers being missed.  The solution is to
drain the converter during config changes.

[wolenetz, 2014-04-18 21:01:15]

Patch set 4 is looking pretty good, though I would like to see a unit test
specifically targeting the config change arriving while renderer's buffer
converter is not yet drained. Is there such a test?

[DaleCurtis, 2014-04-18 23:55:39]

PTAL.  Tests added which found a bug in the AudioBufferConverter!  This patch
now depends on https://codereview.chromium.org/243573005/

[wolenetz, 2014-04-19 00:56:18]

Thanks for adding tests - it looks like they've already been useful :)

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:86: kOutputSamplesPerSecond,
Do any of the tests here make sense to run two different ways: 1: with
AudioBufferConverter, and 2: without AudioBufferConverter? If so,
parameterize+TEST_P+instantiation for these tests?

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:428: return
renderer_->buffer_converter_->input_frames_left_for_testing();
I'm curious why the now-prereq CL introduces a public, but test-specific, method
rather than possibly using a private-scoped friend class [this test class, plus
possibly some others as needed] mechanism. What is the general style here. I'm
drawing from perhaps uncommon style as seen in
media/base/android/media_source_player.h's private friending of
MediaSourcePlayerTest.

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:949:
EXPECT_EQ(converter_input_frames_left(), 0);
nit: put expectation first, not second.

[DaleCurtis, 2014-04-19 00:59:33]

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:86: kOutputSamplesPerSecond,
On 2014/04/19 00:56:18, wolenetz wrote:
> Do any of the tests here make sense to run two different ways: 1: with
> AudioBufferConverter, and 2: without AudioBufferConverter? If so,
> parameterize+TEST_P+instantiation for these tests?

I thought about this, but AudioBufferConverter should be a super set of
functionality, so I don't think there's much value in parameterizing.

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:428: return
renderer_->buffer_converter_->input_frames_left_for_testing();
On 2014/04/19 00:56:18, wolenetz wrote:
> I'm curious why the now-prereq CL introduces a public, but test-specific,
method
> rather than possibly using a private-scoped friend class [this test class,
plus
> possibly some others as needed] mechanism. What is the general style here. I'm
> drawing from perhaps uncommon style as seen in
> media/base/android/media_source_player.h's private friending of
> MediaSourcePlayerTest.

Either one is fine per existing style.  The for_testing() methods are enforced
via a PRESUBMIT check.  I prefer public for_testing() method's when multiple
classes might need access.

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:949:
EXPECT_EQ(converter_input_frames_left(), 0);
On 2014/04/19 00:56:18, wolenetz wrote:
> nit: put expectation first, not second.

Done.

[wolenetz, 2014-04-19 01:00:53]

sorry, one further nit:

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:635: const int
kInitialFramesBuffered = 1114;
nit: here and for kNextFramesBuffered, below: seems these values could be
computed statically from kDataSize and the rates, making their possible
maintenance in future simpler. wdyt?

[wolenetz, 2014-04-19 01:01:57]

lgtm % my last nit, above.

[DaleCurtis, 2014-04-19 01:03:24]

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:635: const int
kInitialFramesBuffered = 1114;
On 2014/04/19 01:00:53, wolenetz wrote:
> nit: here and for kNextFramesBuffered, below: seems these values could be
> computed statically from kDataSize and the rates, making their possible
> maintenance in future simpler. wdyt?

The first one is trivial to compute, but the second depends on the internal
implementation of the AudioBufferConverter, so I'd rather not write overly
complex logic here to compute it.

[wolenetz, 2014-04-19 01:04:49]

On 2014/04/19 01:03:24, DaleCurtis wrote:
>
https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
> File media/filters/audio_renderer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/240123004/diff/80001/media/filters/audio_rend...
> media/filters/audio_renderer_impl_unittest.cc:635: const int
> kInitialFramesBuffered = 1114;
> On 2014/04/19 01:00:53, wolenetz wrote:
> > nit: here and for kNextFramesBuffered, below: seems these values could be
> > computed statically from kDataSize and the rates, making their possible
> > maintenance in future simpler. wdyt?
> 
> The first one is trivial to compute, but the second depends on the internal
> implementation of the AudioBufferConverter, so I'd rather not write overly
> complex logic here to compute it.

sgtm. thanks. you have my l-g-t-m :)

[DaleCurtis, 2014-04-19 04:02:06]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-04-19 04:02:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/240123004/100001

[DaleCurtis, 2014-04-19 04:08:55]

The CQ bit was unchecked by dalecurtis@chromium.org

[DaleCurtis, 2014-04-19 04:55:07]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-04-19 04:55:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/240123004/100001

[commit-bot: I haz the power, 2014-04-20 22:04:06]

Change committed as 264944