Renderer busy looping after video (with sound) or audio ends

Renderer busy looping after video (with sound) or audio ends

BUG=18434
TEST=AudioRendererBaseTest.CompleteLifeTime

This patch fixes the busy looping and leaks by ignore the
end-of-stream packet.

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

[Alpha Left Google, 2009-08-06 00:47:09]

See more details about the cause of this bug:
http://code.google.com/p/chromium/issues/detail?id=18434

I'm so sad to see the memory leak on Windows is so small..(~10KB/s). What it
means is that we have a descent amount of latency in message passing so it's not
leaking as much.

[Alpha Left Google, 2009-08-06 00:49:13]

+fbarchard +kylep

[scherkus (not reviewing), 2009-08-06 03:51:52]

LGTM

we'll need to fix that demuxer so that it generates "ended" packets only when
needed

http://codereview.chromium.org/165023/diff/1/3
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/165023/diff/1/3#newcode180
Line 180: buffer->SetDataSize(1024);
could we use a constant for 1024?  kDataSize or similar

[fbarchard, 2009-08-06 05:28:13]

LGTM

http://codereview.chromium.org/165023/diff/1/2
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/165023/diff/1/2#newcode124
Line 124: if (!buffer_in->IsEndOfStream()) {
you check eof twice.  is that a problem?  what if this one is false and then the
next one is true.
nit ' should be avoided

[kylep, 2009-08-06 06:27:23]

This might actually be more easily fixed in
AudioRendererAlgorithmBase::EnqueueBuffer

On Wed, Aug 5, 2009 at 10:28 PM, <fbarchard@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.org/165023/diff/1/2
> File media/filters/audio_renderer_base.cc (right):
>
> http://codereview.chromium.org/165023/diff/1/2#newcode124
> Line 124: if (!buffer_in->IsEndOfStream()) {
> you check eof twice.  is that a problem?  what if this one is false and
> then the next one is true.
> nit ' should be avoided
>
>
> http://codereview.chromium.org/165023
>

[Alpha Left Google, 2009-08-06 07:14:56]

On 2009/08/06 05:28:13, fbarchard wrote:
> LGTM
> 
> http://codereview.chromium.org/165023/diff/1/2
> File media/filters/audio_renderer_base.cc (right):
> 
> http://codereview.chromium.org/165023/diff/1/2#newcode124
> Line 124: if (!buffer_in->IsEndOfStream()) {
> you check eof twice.  is that a problem?  what if this one is false and then
the
> next one is true.
> nit ' should be avoided

The two checks on end-of-stream serve different purposes and both are needed.
And there won't be a case that end-of-stream property would change.

[Alpha Left Google, 2009-08-06 07:17:39]

On 2009/08/06 06:27:23, kylep wrote:
> This might actually be more easily fixed in
> AudioRendererAlgorithmBase::EnqueueBuffer

I was thinking about removing the check of IsEndOfStream in
AudioRendererAlgorithmBase::EnqueueBuffer instead. It seems to me
AudioRendererAlgorithmBase is responsible for audio resampling, it shouldn't
know about special case like end-of-stream buffer or anything like that.
end-of-stream buffer shouldn't feed into it anyway.

> 
>
 On Wed, Aug 5, 2009 at 10:28 PM, <fbarchard@chromium.org> wrote:
> 
> > LGTM
> >
> >
> > http://codereview.chromium.org/165023/diff/1/2
> > File media/filters/audio_renderer_base.cc (right):
> >
> > http://codereview.chromium.org/165023/diff/1/2#newcode124
> > Line 124: if (!buffer_in->IsEndOfStream()) {
> > you check eof twice.  is that a problem?  what if this one is false and
> > then the next one is true.
> > nit ' should be avoided
> >
> >
> > http://codereview.chromium.org/165023
> >
>

[Alpha Left Google, 2009-08-06 07:20:08]

On 2009/08/06 03:51:52, scherkus wrote:
> LGTM
> 
> we'll need to fix that demuxer so that it generates "ended" packets only when
> needed
> 

Instead of fixing the demuxer. The demuxer actually has the right behavior: if
you read at end of stream, you get end-of-stream packet. It should be reader's
responsibility to stop reading, if we throw errors instead that may just cause
some unnecessary panic of the pipeline.

> http://codereview.chromium.org/165023/diff/1/3
> File media/filters/audio_renderer_base_unittest.cc (right):
> 
> http://codereview.chromium.org/165023/diff/1/3#newcode180
> Line 180: buffer->SetDataSize(1024);
> could we use a constant for 1024?  kDataSize or similar

[awong, 2009-08-06 18:37:46]

LGTM after addressing other comments.

Also, please pick a nicer name before checking for the test.

http://codereview.chromium.org/165023/diff/1/2
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/165023/diff/1/2#newcode124
Line 124: if (!buffer_in->IsEndOfStream()) {
On 2009/08/06 05:28:13, fbarchard wrote:
> you check eof twice.  is that a problem?  what if this one is false and then
the
> next one is true.
> nit ' should be avoided
> 

Agree in principle, but in this case, the buffer is only used by this thread and
this function is actually a query of a flag.  Really, it should be all
lowercase. :-/

http://codereview.chromium.org/165023/diff/1/3
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/165023/diff/1/3#newcode147
Line 147: TEST_F(AudioRendererBaseTest, CompleteLifetime) {
I don't undersatnd this testname.  Waht does "CompleteLifetime" mean?

[Alpha Left Google, 2009-08-06 19:02:27]

http://codereview.chromium.org/165023/diff/1/2
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/165023/diff/1/2#newcode124
Line 124: if (!buffer_in->IsEndOfStream()) {
On 2009/08/06 18:37:46, awong wrote:
> On 2009/08/06 05:28:13, fbarchard wrote:
> > you check eof twice.  is that a problem?  what if this one is false and then
> the
> > next one is true.
> > nit ' should be avoided
> > 
> 
> Agree in principle, but in this case, the buffer is only used by this thread
and
> this function is actually a query of a flag.  Really, it should be all
> lowercase. :-/

Right, once a buffer is created, only read is possible on it.

http://codereview.chromium.org/165023/diff/1/3
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/165023/diff/1/3#newcode147
Line 147: TEST_F(AudioRendererBaseTest, CompleteLifetime) {
On 2009/08/06 18:37:46, awong wrote:
> I don't undersatnd this testname.  Waht does "CompleteLifetime" mean?

What about CompleteReadCycle? I suck at naming things.. :(

[awong, 2009-08-06 19:07:23]

http://codereview.chromium.org/165023/diff/1/3
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/165023/diff/1/3#newcode147
Line 147: TEST_F(AudioRendererBaseTest, CompleteLifetime) {
On 2009/08/06 19:02:28, Alpha wrote:
> On 2009/08/06 18:37:46, awong wrote:
> > I don't undersatnd this testname.  Waht does "CompleteLifetime" mean?
> 
> What about CompleteReadCycle? I suck at naming things.. :(

Yeah, naming is hard.  I guess one way to do it is to put it in a sentence like
"When this passes, we know that X works".

You're executing one read cycle right?  So, maybe call it "OneReadCycle"?

[Alpha Left Google, 2009-08-06 19:11:34]

On 2009/08/06 19:07:23, awong wrote:
> http://codereview.chromium.org/165023/diff/1/3
> File media/filters/audio_renderer_base_unittest.cc (right):
> 
> http://codereview.chromium.org/165023/diff/1/3#newcode147
> Line 147: TEST_F(AudioRendererBaseTest, CompleteLifetime) {
> On 2009/08/06 19:02:28, Alpha wrote:
> > On 2009/08/06 18:37:46, awong wrote:
> > > I don't undersatnd this testname.  Waht does "CompleteLifetime" mean?
> > 
> > What about CompleteReadCycle? I suck at naming things.. :(
> 
> Yeah, naming is hard.  I guess one way to do it is to put it in a sentence
like
> "When this passes, we know that X works".
> 
> You're executing one read cycle right?  So, maybe call it "OneReadCycle"?

Not really one read cycle. This actually simulates a complete cycle of
pre-rolling, fetching from buffer and then receiving an end-of-stream packet. So
I feel like CompleteReadCycle is better?

[awong, 2009-08-06 19:22:59]

On Thu, Aug 6, 2009 at 12:11 PM, <hclam@chromium.org> wrote:

> On 2009/08/06 19:07:23, awong wrote:
>
>> http://codereview.chromium.org/165023/diff/1/3
>> File media/filters/audio_renderer_base_unittest.cc (right):
>>
>
>  http://codereview.chromium.org/165023/diff/1/3#newcode147
>> Line 147: TEST_F(AudioRendererBaseTest, CompleteLifetime) {
>> On 2009/08/06 19:02:28, Alpha wrote:
>> > On 2009/08/06 18:37:46, awong wrote:
>> > > I don't undersatnd this testname.  Waht does "CompleteLifetime"
>>
> mean?
>
>> >
>> > What about CompleteReadCycle? I suck at naming things.. :(
>>
>
>  Yeah, naming is hard.  I guess one way to do it is to put it in a
>>
> sentence like
>
>> "When this passes, we know that X works".
>>
>
>  You're executing one read cycle right?  So, maybe call it
>>
> "OneReadCycle"?
>
> Not really one read cycle. This actually simulates a complete cycle of
> pre-rolling, fetching from buffer and then receiving an end-of-stream
> packet. So I feel like CompleteReadCycle is better?


OneCompleteReadCycle might be nicer.  CompleteReadCycle's problem is that it
parses two was.  It either means "a full compelte cycle" or "the event of
the cycle finishing."