Closed Bug 1205540 Opened 4 years ago Closed 4 years ago

skip processing of inactive audio nodes

Categories

(Core :: Web Audio, defect, P1)

43 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

For bug 1189562 we need to detect which streams have no input and so can be suspended.
These inactive streams must continue to produce output until the end of the MSG iteration but the output is known to be zero and so processing is simple.

In this bug, the streams are detected and the zero processing will be optimized.
The suspending for bug 1189562 is not covered by this bug.
MSG thread processing time from perf top -t <tid> on http://webaudiodemos.appspot.com/MIDIDrums/index.html initially is distributed mostly in number crunching:

    12.00%  liblgpllibs.so  [.] rdft_calc_c                                   
     5.45%  libxul.so       [.] WebCore::DirectConvolver::process             
     5.17%  libxul.so       [.] moz_speex_interpolate_product_single          
     4.89%  liblgpllibs.so  [.] ff_fft_permute_sse.loop                       
     4.50%  libxul.so       [.] resampler_basic_interpolate_single            
     4.42%  libxul.so       [.] mozilla::AudioBufferAddWithScale              
     4.34%  libxul.so       [.] mozilla::BufferComplexMultiply                
     4.33%  libxul.so       [.] mozilla::FFTBlock::GetInverseWithoutScaling   
     4.07%  liblgpllibs.so  [.] pass_sse.loop                                 
     3.86%  libxul.so       [.] WebCore::FFTConvolver::process                
     3.18%  libxul.so       [.] cubic_coef                                    
     2.93%  liblgpllibs.so  [.] fft16_sse                                     
     2.48%  libxul.so       [.] mozilla::DelayBuffer::ReadChannels            
     2.46%  liblgpllibs.so  [.] pass_interleave_sse.loop                      
     2.14%  libxul.so       [.] mozilla::FFTBlock::PerformFFT                 
     1.99%  libm-2.20.so    [.] __powf_finite                                 
     1.42%  liblgpllibs.so  [.] fft8_sse                                      
     1.22%  libm-2.20.so    [.] __logf_finite                                 
     1.21%  libxul.so       [.] WebCore::ReverbConvolverStage::process        
     1.09%  libxul.so       [.] mozilla::StreamBuffer::FindTrack              
     1.08%  libxul.so       [.] WebCore::DynamicsCompressorKernel::process    
     0.98%  firefox         [.] je_arena_dalloc_junk_small                    
     0.93%  libxul.so       [.] WebCore::ZeroPole::process                    
     0.92%  liblgpllibs.so  [.] ff_fft_permute_sse.loopcopy                   
     0.76%  libxul.so       [.] mozilla::AudioNodeStream::AdvanceOutputSegment

As inactive nodes sit waiting for GC things the distribution changes to be dominated by overhead:

    10.48%  libxul.so       [.] mozilla::StreamBuffer::FindTrack                
     8.38%  libxul.so       [.] mozilla::AudioNodeStream::ObtainInputBlock      
     7.52%  libxul.so       [.] mozilla::MediaStreamGraphImpl::ProduceDataForStr
     7.08%  libxul.so       [.] mozilla::AudioNodeStream::AdvanceOutputSegment  
     5.52%  libxul.so       [.] mozilla::AudioNodeStream::ProcessInput          
     5.42%  libxul.so       [.] mozilla::MediaSegmentBase<mozilla::AudioSegment,
     3.41%  libxul.so       [.] mozilla::AudioNodeStream::ComputedNumberOfChanne
     3.34%  libxul.so       [.] mozilla::MediaStreamGraphImpl::AddBlockingRelate
     3.12%  libxul.so       [.] mozilla::AudioBlock::SetBuffer                  
     2.90%  liblgpllibs.so  [.] rdft_calc_c                                     
     1.66%  libxul.so       [.] mozilla::TimeVarying<long, bool, 5u>::GetAt     
     1.64%  liblgpllibs.so  [.] ff_fft_permute_sse.loop                         
     1.50%  libxul.so       [.] WebCore::FFTConvolver::process                  
     1.44%  libxul.so       [.] mozilla::AudioBufferAddWithScale                
     1.40%  libxul.so       [.] WebCore::DirectConvolver::process               
     1.34%  libxul.so       [.] mozilla::FFTBlock::GetInverseWithoutScaling     
     1.30%  libxul.so       [.] mozilla::MediaStreamGraphImpl::UpdateStreamOrder
     1.28%  libxul.so       [.] mozilla::dom::PannerNodeEngine::ProcessBlock    
     1.26%  libxul.so       [.] mozilla::BufferComplexMultiply                  
     1.24%  libxul.so       [.] moz_speex_interpolate_product_single            
     1.13%  libxul.so       [.] resampler_basic_interpolate_single              
     1.12%  libxul.so       [.] mozilla::StreamBuffer::ForgetUpTo               
     1.05%  liblgpllibs.so  [.] pass_sse.loop                                   
     1.00%  libxul.so       [.] mozilla::MediaStream::EnsureTrack               
     0.84%  libxul.so       [.] cubic_coef                                      

Patches that I'll attach here to skip ProcessBlock and ObtainInputBlock on inactive nodes change the distribution to:

    14.77%  libxul.so       [.] mozilla::StreamBuffer::FindTrack                
     9.53%  libxul.so       [.] mozilla::MediaStreamGraphImpl::ProduceDataForStr
     7.79%  libxul.so       [.] mozilla::AudioNodeStream::AdvanceOutputSegment  
     6.73%  libxul.so       [.] mozilla::AudioNodeStream::ProcessInput          
     5.93%  libxul.so       [.] mozilla::MediaSegmentBase<mozilla::AudioSegment,
     3.75%  libxul.so       [.] mozilla::AudioNodeStream::ObtainInputBlock      
     3.05%  liblgpllibs.so  [.] rdft_calc_c                                     
     2.81%  libxul.so       [.] mozilla::MediaStreamGraphImpl::AddBlockingRelate
     2.21%  libxul.so       [.] mozilla::AudioBlock::SetBuffer                  
     2.06%  libxul.so       [.] mozilla::TimeVarying<long, bool, 5u>::GetAt     
     1.62%  liblgpllibs.so  [.] ff_fft_permute_sse.loop                         
     1.51%  libxul.so       [.] mozilla::MediaStreamGraphImpl::UpdateStreamOrder
     1.49%  libxul.so       [.] mozilla::AudioBufferAddWithScale                
     1.45%  libxul.so       [.] WebCore::DirectConvolver::process               
     1.42%  libxul.so       [.] WebCore::FFTConvolver::process                  
     1.38%  libxul.so       [.] mozilla::MediaStreamGraphImpl::RecomputeBlocking
     1.25%  libxul.so       [.] mozilla::StreamBuffer::ForgetUpTo               
     1.24%  libxul.so       [.] moz_speex_interpolate_product_single            
     1.23%  libxul.so       [.] mozilla::BufferComplexMultiply                  
     1.15%  liblgpllibs.so  [.] pass_sse.loop                                   
     1.15%  libxul.so       [.] mozilla::FFTBlock::GetInverseWithoutScaling     
     1.13%  libxul.so       [.] resampler_basic_interpolate_single              
     0.88%  libxul.so       [.] mozilla::MediaStreamGraphImpl::StreamSet::iterat
     0.83%  libxul.so       [.] cubic_coef                                      
     0.80%  libxul.so       [.] mozilla::MediaStreamGraphImpl::NotifyHasCurrentD
bug 1205540 don't send more null chunks than necessary to AnalyserNode r?padenot
Attachment #8662206 - Flags: review?(padenot)
bug 1205540 provide querying whether engines need to continue processing even without input r?padenot
Attachment #8662207 - Flags: review?(padenot)
bug 1205540 make source stream available during RemoveInput r?padenot
Attachment #8662208 - Flags: review?(padenot)
bug 1205540 account for active inputs and skip processing when streams are inactive r?padenot
Attachment #8662209 - Flags: review?(padenot)
https://reviewboard.mozilla.org/r/19569/#review17513

::: dom/media/webaudio/AudioNodeStream.cpp:736
(Diff revision 1)
> +  if (((mActiveInputCount >= 0 || mEngine->IsActive()) &&

This should be strictly mActiveInputCount > 0.

With that change there are some test failures so feel free to hold off review until I sort them out.
Attachment #8662209 - Flags: review?(padenot)
Comment on attachment 8662209 [details]
MozReview Request: bug 1205540 mark BufferSource finished only when producing silent output block r?padenot

bug 1205540 mark BufferSource finished only when producing silent output block r?padenot

This allows simpler processing of the finished state to mark the node as an
inactive input of any downstream nodes.  Otherwise the input could not be
considered inactive until after downstream nodes have finished processing,
but ProcessInput() may not be called again on finished streams.

AudioBufferSourceNode now behaves the same as OscillatorNode and similarly
to nodes that release a playing ref.
Attachment #8662209 - Attachment description: MozReview Request: bug 1205540 account for active inputs and skip processing when streams are inactive r?padenot → MozReview Request: bug 1205540 mark BufferSource finished only when producing silent output block r?padenot
Attachment #8662209 - Flags: review?(padenot)
bug 1205540 account for active inputs and skip processing when streams are inactive r?padenot
Attachment #8662345 - Flags: review?(padenot)
Rank: 5
Priority: -- → P1
Attachment #8662206 - Flags: review?(padenot) → review+
Comment on attachment 8662206 [details]
MozReview Request: bug 1205540 don't send more null chunks than necessary to AnalyserNode r?padenot

https://reviewboard.mozilla.org/r/19563/#review17657
Comment on attachment 8662207 [details]
MozReview Request: bug 1205540 provide querying whether engines need to continue processing even without input r?padenot

https://reviewboard.mozilla.org/r/19565/#review17581

::: dom/media/webaudio/AudioBufferSourceNode.cpp:514
(Diff revision 1)
> +    return mBufferSampleRate != 0;
Attachment #8662207 - Flags: review?(padenot) → review+
Comment on attachment 8662208 [details]
MozReview Request: bug 1205540 make source stream available during RemoveInput r?padenot

https://reviewboard.mozilla.org/r/19567/#review17659
Attachment #8662208 - Flags: review?(padenot) → review+
Comment on attachment 8662209 [details]
MozReview Request: bug 1205540 mark BufferSource finished only when producing silent output block r?padenot

https://reviewboard.mozilla.org/r/19569/#review17661
Attachment #8662209 - Flags: review?(padenot) → review+
Attachment #8662345 - Flags: review?(padenot) → review+
Comment on attachment 8662345 [details]
MozReview Request: bug 1205540 initialize mBufferFormat when constructing silent block r?padenot

https://reviewboard.mozilla.org/r/19581/#review17663
(In reply to Paul Adenot (:padenot) from comment #11)
> Comment on attachment 8662207 [details]
> MozReview Request: bug 1205540 provide querying whether engines need to
> continue processing even without input r?padenot
> 
> https://reviewboard.mozilla.org/r/19565/#review17581
> 
> ::: dom/media/webaudio/AudioBufferSourceNode.cpp:514
> (Diff revision 1)
> > +    return mBufferSampleRate != 0;

Not sure what happened with review board, I don't have anything in particular to say about this line.
sorry had to back this out for assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14327934&repo=mozilla-inbound
Flags: needinfo?(karlt)
Attachment #8662345 - Attachment description: MozReview Request: bug 1205540 account for active inputs and skip processing when streams are inactive r?padenot → MozReview Request: bug 1205540 initialize mBufferFormat when constructing silent block r?padenot
Comment on attachment 8662345 [details]
MozReview Request: bug 1205540 initialize mBufferFormat when constructing silent block r?padenot

bug 1205540 initialize mBufferFormat when constructing silent block r?padenot

This makes an AudioBlock valid for code testing mBufferFormat without IsNull(),
without the need for explicit SetNull().

This is useful so that setting AudioNodeStream::mLastChunks each iteration is
not required for inactive nodes.
Flags: needinfo?(karlt)
Attachment #8662345 - Flags: review+ → review?(padenot)
Attachment #8663473 - Flags: review+
Comment on attachment 8663473 [details]
MozReview Request: bug 1205540 account for active inputs and skip processing when streams are inactive r=padenot

https://reviewboard.mozilla.org/r/19815/#review17819
Thanks for all the reviews, Paul.

I'm going to assume that comment 25 was aimed at the latest change
https://reviewboard.mozilla.org/r/19561/diff/2-3/
https://reviewboard.mozilla.org/r/19581/diff/2#index_header

Inserting a changeset, as I did, makes changeset evolution hard to track and
the reviewboard UI looks very similar regardless of which particular changeset
is being reviewed.

Let me know if I'm jumping to conclusions.
(In reply to Karl Tomlinson (ni?:karlt) from comment #26)
> Thanks for all the reviews, Paul.
> 
> I'm going to assume that comment 25 was aimed at the latest change
> https://reviewboard.mozilla.org/r/19561/diff/2-3/
> https://reviewboard.mozilla.org/r/19581/diff/2#index_header
> 
> Inserting a changeset, as I did, makes changeset evolution hard to track and
> the reviewboard UI looks very similar regardless of which particular
> changeset
> is being reviewed.
> 
> Let me know if I'm jumping to conclusions.

No you're right, I'm really struggling with ReviewBoard, I need to invest a bit more time in it.
Comment on attachment 8662345 [details]
MozReview Request: bug 1205540 initialize mBufferFormat when constructing silent block r?padenot

(clearing the review request)
Attachment #8662345 - Flags: review?(padenot)
You need to log in before you can comment on or make changes to this bug.