Greg, > Was the case where the buffer was full taken care of(so we sleep at > least a little)? >
Read further down the thread. Looks like it's not an issue for ALSA, so he put in a microsleep if the buffer was full for non-ALSA implementations. Eric ============================================= Eric Hyche ([EMAIL PROTECTED]) Technical Lead RealNetworks, Inc. > -----Original Message----- > From: Gregory Wright [mailto:[EMAIL PROTECTED] > Sent: Thursday, April 17, 2008 11:38 PM > To: [EMAIL PROTECTED] > Cc: 'Rusty Lynch'; 'audio-dev'; 'midplayer-private-dev' > Subject: Re: Take 3 => CR: Remove polling inside UNIX audio thread > > On Apr 17, 2008, at 8:04 PM, Eric Hyche wrote: > > > > > > Further comments: > > > > @@ -201,6 +204,14 @@ > > > > UINT16 CAudioOutUNIX::_Imp_GetVolume() > > { > > +#if defined(_THREADED_AUDIO) && defined(_UNIX_THREADS_SUPPORTED) > > + //We want to sleep as a function of device buffer > > size. > > + //If we have a small m_ulDeviceBufferSize > we can only > > + //afford to sleep just a little while. > > + HX_ASSERT( m_ulDeviceBufferSize != 0 ); > > + m_ulSleepTime = (((float)m_ulDeviceBufferSize/ > > (float)m_uSampFrameSize)/ > > + (float)m_unSampleRate) * 1000 / > > (float)m_unNumChannels; > > +#endif > > if (!m_bMixerPresent) > > _OpenMixer(); > > > > @@ -271,14 +282,6 @@ > > } > > else > > { > > -#if defined(_THREADED_AUDIO) && defined(_UNIX_THREADS_SUPPORTED) > > - //We want to sleep as a function of device buffer > > size. > > - //If we have a small m_ulDeviceBufferSize > we can only > > - //afford to sleep just a little while. > > - HX_ASSERT( m_ulDeviceBufferSize != 0 ); > > - m_ulSleepTime = (((float)m_ulDeviceBufferSize/ > > (float)m_uSampFrameSize)/ > > - (float)m_unSampleRate) * 1000 / > > (float)m_unNumChannels; > > -#endif > > if (!m_bMixerPresent) > > _OpenMixer(); > > > > > > Was there some reason you decided to move this > > from _Imp_Open() to _Imp_GetVolume()? > > I don't think it belongs in any volume call, it should remain in Open. > > Was the case where the buffer was full taken care of(so we sleep at > least a little)? > > --greg. > > > > > > > > Rest looks good to me. Greg: do you have any > > further comments? > > > > Eric > > > > ============================================= > > Eric Hyche ([EMAIL PROTECTED]) > > Technical Lead > > RealNetworks, Inc. > > > >> -----Original Message----- > >> From: Rusty Lynch [mailto:[EMAIL PROTECTED] > >> Sent: Thursday, April 17, 2008 7:40 PM > >> To: audio-dev > >> Cc: midplayer-private-dev; Eric Hyche; Greg Wright > >> Subject: Take 3 => CR: Remove polling inside UNIX audio thread > >> > >> Ahh! Just took a another look at my patch and realized that I was > >> adding an extra unneeded micro sleep when exiting. Not a > big deal in > >> the grand scheme of things, but not very clean. > >> > >> This version of the patch now only calls micro sleep if the > >> bReadyToExit > >> is false and there is data to write. > >> > >> --rusty > >> > >> On Thu, 2008-04-17 at 16:22 -0700, Rusty Lynch wrote: > >>> On Thu, 2008-04-17 at 17:03 -0400, Eric Hyche wrote: > >>>> Rusty, > >>>> > >>>> Here are my comments. Greg may want to comment further. > >>>> > >>>> @@ -142,6 +141,7 @@ > >>>> CreateInstanceCCF(CLSID_IHXMutex, > >> (void**)&m_mtxWriteListPlayStateLock, m_pContext); > >>>> CreateInstanceCCF(CLSID_IHXMutex, > >> (void**)&m_mtxDeviceStateLock, m_pContext); > >>>> CreateInstanceCCF(CLSID_IHXThread, > >> (void**)&m_audioThread, m_pContext); > >>>> + HXEvent::MakeEvent(m_pAvailableDataEvent, "Available > >> Audio Data", FALSE); > >>>> > >>>> You should create an IHXEvent just like the above > >>>> code creates IHXThread or IHXMutex: > >>>> > >>>> + CreateInstanceCCF(CLSID_IHXEvent, (void**) > >> &m_pAvailableDataEvent, m_pContext); > >>>> > >>>> and of course, m_pAvailableDataEvent should be an IHXEvent rather > >>>> than an HXEvent: > >>>> > >>>> + IHXEvent* m_pAvailableDataEvent; > >>>> > >>>> and you should release it rather than delete it: > >>>> > >>>> + HX_RELEASE( m_pAvailableDataEvent ); > >>>> > >>> > >>> Ok, I started using IHXEvent, and since I wanted a infinite > >> wait I am > >>> calling: > >>> > >>> m_pAvailableDataEvent->Wait(ALLFS); > >>> > >>> ... which I am just guessing (from looking at other code) > >> is cool, but > >>> ALLFS isn't translating to anything sensible in my brain, > >> so let me know > >>> if that is not something I behavior I can count on. > >>> > >>>> > >>>> What would happen if the audio device was full and we > >>>> still had data to write? It seems like we would just > >>>> spin in a tight loop until the audio device had space > >>>> for us to write. Not really a failure case, but > >>>> seems like we should wait in that case as well. > >>>> > >>>> Eric > >>>> > >>> > >>> On an ALSA system _WriteBytes will eventually come down to an > >>> snd_pcm_writei(), and since we are setting the audio device > >> to blocking > >>> in _OpenAudio(), then snd_pcm_writei() will block until the > >> device is > >>> able to consume all the bits. > >>> > >>> > >> http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m.ht > >> ml#gf13067c0ebde29118ca05af76e5b17a9 > >>> > >>> Can I count on other UNIX systems having a blocking write > >> behavior? I > >>> don't know. I think esd (esound) blocks on write, but I'm > >> having a hard > >>> time finding any API documentation to verify that. > >>> > >>> hmm... looks like Solaris is opening the device node using > >> O_NONBLOCK. > >>> > >>> Ok, here is a version of the patch that will do a > >> microsleep if there is > >>> data to write, but only if HELIX_FEATURE_ALSA is defined. > >>> > >>> The block of code in question now looks more like... > >>> > >>> if(bReadyToExit == FALSE && > >>> (that->m_pWriteList->GetCount() == 0 || > >>> that->m_wState == RA_AOS_OPEN_PAUSED)) > >>> { > >>> that->m_pAvailableDataEvent->Wait(ALLFS); > >>> } > >>> else > >>> { > >>> #if !defined(HELIX_FEATURE_ALSA) > >>> microsleep(that->m_ulSleepTime/4); > >>> #endif > >>> } > >>> > >>> > >>> > >>> _______________________________________________ > >>> Midplayer-private-dev mailing list > >>> [EMAIL PROTECTED] > >>> > >> > http://lists.helixcommunity.org/mailman/listinfo/midplayer-private- > >> dev > >> > > > _______________________________________________ Audio-dev mailing list [email protected] http://lists.helixcommunity.org/mailman/listinfo/audio-dev
