+    HX_DELETE(m_pStreamBufferList);

Are we guaranteed that m_pStreamBufferList is empty here?


+    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.


+    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;
    }



+    HXBOOL       bPacketsAfterRange = FALSE;
+    HXBOOL       bPacketsAfterRange = FALSE;

unused variables???? Can't tell in the second one.


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.

--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

Reply via email to