Thanks Greg. Made the chances you suggested.
fxd
On Fri, 2009-05-01 at 07:27 -0700, Greg Wright wrote:
> + HX_DELETE(m_pStreamBufferList);
>
> Are we guaranteed that m_pStreamBufferList is empty here?
It should be since we write out or move to the aux list every item in
the list in PlayAudio().
>
>
> + if (m_pSilenceBuffer->GetSize() < ulSizeInBytes)
> + {
> + m_pSilenceBuffer->SetSize(ulSizeInBytes);
> +
> + if (m_pSilenceBuffer->GetSize() != ulSizeInBytes)
> + {
> + //cannot resize because refcount > 1, create a new buffer
> + HX_RELEASE(m_pSilenceBuffer);
> + HX_ASSERT(m_pCommonClassFactory);
> + IUnknown* pUnk = NULL;
> + if (HXR_OK ==
> m_pCommonClassFactory->CreateInstance(CLSID_IHXBuffer, (void**)&pUnk))
> + {
> + pUnk->QueryInterface(IID_IHXBuffer,
> (void**)&m_pSilenceBuffer);
> + HX_RELEASE(pUnk);
> + }
> + else
> + {
> + return HXR_OUTOFMEMORY;
> + }
> + }
> + memset(m_pSilenceBuffer->GetBuffer(), 0, ulSizeInBytes);
> + }
>
>
> When you create a new buffer above, because of refcount problems,
> you still call memset(..., ulSizeInBytes) without first setting
> the size on the new buffer. Should probably NULL check around the
> memset too.
Good catch. Here is the diff against the original CR to fix this.
Index: hxaudstr_new.cpp
===================================================================
RCS file: /cvsroot/client/audiosvc/hxaudstr_new.cpp,v
retrieving revision 1.44.2.3
diff -u -w -r1.44.2.3 hxaudstr_new.cpp
--- hxaudstr_new.cpp 1 May 2009 13:25:33 -0000 1.44.2.3
+++ hxaudstr_new.cpp 1 May 2009 13:35:00 -0000
@@ -2146,12 +2146,14 @@
{
pUnk->QueryInterface(IID_IHXBuffer,
(void**)&m_pSilenceBuffer);
HX_RELEASE(pUnk);
+ m_pSilenceBuffer->SetSize(ulSizeInBytes);
}
else
{
return HXR_OUTOFMEMORY;
}
}
+ HX_ASSERT(m_pSilenceBuffer->GetBuffer() &&
m_pSilenceBuffer->GetSize() == ulSizeInBytes);
memset(m_pSilenceBuffer->GetBuffer(), 0, ulSizeInBytes);
}
>
>
> + if (RemoveOldPackets(llStartTimeInSamples) != HXR_OK) return FALSE;
>
> Can you break that up please? In Helix coding standards we prefer single
> 'if' lines to look like:
>
> if (RemoveOldPackets(llStartTimeInSamples) != HXR_OK)
> {
> return FALSE;
> }
>
Done.
>
>
> + HXBOOL bPacketsAfterRange = FALSE;
> + HXBOOL bPacketsAfterRange = FALSE;
>
> unused variables???? Can't tell in the second one.
First one is. I removed it, as well as bDidMix.
>
> I would also suggest that HELIX_CONFIG_MIN_ROLLBACK_BUFFER is not
> as needed as it once was. The whole rollback buffer approach was
> put into place to support audio devices, back in the late 90's,
> that did not support hardware pause, the HELIX_CONFIG_MIN_ROLLBACK_BUFFER
> was added to help the HEAP use a little bit on mobile devices that
> did not have hardware pause. The full rollback buffer supports no loss
> of audio data at the cost of additional memory use. The MIN rollback
> buffer uses extra memory and looses data. So, probably better to just
> not use the full one or not use it at all, which is probably the case
> as more and more audio devices have hardware pause as default.
Thanks. I'll have a look what is the best solution here for devices like
Android.
>
> --greg.
>
>
> Sheldon fu wrote:
> > Synopsis:
> >
> > This change gets rid of unnecessary memcpy and/or equivalent when mixing
> > is not really needed.
> >
> >
> > Overview:
> >
> > Currently even when there is only a single player/stream, we still run
> > PCM data through the DSP mix engine. The mix engine logic will do two
> > memory copying operations even when in/out format matches (same sample
> > rate, channels and bytesPerSample) -- one in ConvertIntoBuffer when it
> > asks for the data from stream object and one to mix the data into the
> > output buffer session object provides. These two memcpys are pure
> > overhead in the typical single player/stream playback case.
> >
> > The change adds a buffer list parameter to the stream object
> > MixIntoBuffer and in turn mixengine MixIntoBuffer method and a flag to
> > control it. When both session and mixengine agrees to do the 'bypass'
> > optimization, the stream object will return a list of buffers containing
> > exactly one block of audio data if put together. Stream object
> > constructs this list from its internal data buffers with
> > CHXBufferFragment to avoid create any new memory buffer. mix engine
> > MixIntoBuffer still runs through its logic but it will not touch any
> > actual data (no ConvertIntoBuffer call and no mixing into output
> > buffer). Stream object sends the buffer in the 'direct list' to the
> > audio device without change.
> >
> > Session object agress to do 'bypass' when it knows there is only a
> > single player/stream and when the new HELIX_FEATURE_OPTIMIZED_MIXING is
> > defined.
> >
> > MixEngine agrees to do 'bypass' when it knows that there is nothing it
> > really needs to do -- same in/out format and when some of the DSP
> > features are not defined. The logic here can be improved in the future
> > so that we can handle these features dynamically -- e.g. when cross fade
> > is on, we can still do bypass until to the point that cross fade kicks
> > in. This is not coded yet since this work is mainly for optimization on
> > Android and Android build doesn't have these DSP features defined
> > currently.
> >
> > Also this feature conflicts with HELIX_CONFIG_MIN_ROLLBACK_BUFFER that
> > the unix audio device uses. When HELIX_CONFIG_MIN_ROLLBACK_BUFFER is on,
> > unix audio device dynamically adjust its rollback buffer size to the
> > incoming buffer size every time _Write is called. That apparently only
> > can work when _Write is always called with a constant buffer size and if
> > not, it will break. Not sure why we have such logic there. Also the
> > rollback handling in unix audio device makes copies of incoming data
> > too.
> >
> > Even with the change, the 'artificial' block-based handling of audio
> > data is still not efficient. Though now there is no memcpy anymore, the
> > control logic is quite complicated. OS Audio API can handle any size
> > write, up to the maximum buffer limit and for the simplest case, we
> > should just write the decoded PCM frame data to OS audio device in a
> > 1-to-1 mapping way.
> >
> > On Android, the change results in about 1-2% CPU usage saving when
> > playing a MP3 clip and there is no noticeable change on my host Linux
> > box (the machine is too fast already I think).
> >
> > This CR also contains the change in the previous CR for mixengine
> > cvt16/cvt32 optimization.
> >
> > Head and Atlas310.
> >
> > fxd
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > Audio-dev mailing list
> > Audio-dev@helixcommunity.org
> > http://lists.helixcommunity.org/mailman/listinfo/audio-dev
>
_______________________________________________
Audio-dev mailing list
Audio-dev@helixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/audio-dev
