[libavg-devel] Audio Support Patch
Nick Hebner
hebnern at gmail.com
Mon Dec 17 23:57:51 CET 2007
On Dec 16, 2007 3:22 PM, Ulrich von Zadow <uzadow at libavg.de> wrote:
> Lots of comments inline...
>
> (Very technical & sometimes critical comments. Don't take this to mean I
> don't value your work. On the contrary!)
>
> Nick Hebner wrote:
> > Index: /home/nick/workspace/libavg/configure.in
> > ===================================================================
> > --- /home/nick/workspace/libavg/configure.in (revision 2460)
> > +++ /home/nick/workspace/libavg/configure.in (working copy)
> > Index: /home/nick/workspace/libavg/src/avgconfig.h.in
> [...]
> > ===================================================================
> > --- /home/nick/workspace/libavg/src/avgconfig.h.in (revision 2460)
> > +++ /home/nick/workspace/libavg/src/avgconfig.h.in (working copy)
> [...]
> Just a nitpick, but these aren't real changes, right? Then they
> shouldn't be in the patch.
>
Yeah, this patch isn't as clean as it could be.
>
> > Index: /home/nick/workspace/libavg/src/player/AudioEngine.cpp
> > ===================================================================
> > --- /home/nick/workspace/libavg/src/player/AudioEngine.cpp (revision
> 0)
> > +++ /home/nick/workspace/libavg/src/player/AudioEngine.cpp (revision
> 0)
> > @@ -0,0 +1,59 @@
> > +//
> > +// libavg - Media Playback Engine.
> > +// Copyright (C) 2007 Nick Hebner
>
> To avoid legal issues, I have to ask you to assign the copyright to me:
>
> // Copyright (C) 2007 Ulrich von Zadow
>
> If the copyrights are owned by lots of different people, even a
> technical license change (such as updating to LGPL 3.0) becomes a major
> hassle because I have to ask everyone who contributed. Of course, I can
> never change the license of the current version, so there is really
> nothing to fear. (I'm also pretty committed to open source, but you are
> under no obligation to believe me ;-).)
>
> You can add a line at the bottom of the copyright blurb if you want,
> though:
>
> // Original author of this file is Nick Hebner (hebern at gmail.com).
>
Hmm, ok. I am honestly not all that interested in the legal mumbo jumbo
involved in licensing, I would just like to have my work attributed to me.
If this is the easiest way to do that, then that is absolutely fine with me.
>
> [...]
>
> > Index: /home/nick/workspace/libavg/src/player/avgdtd.cpp
> > ===================================================================
> > --- /home/nick/workspace/libavg/src/player/avgdtd.cpp (revision 2460)
> > +++ /home/nick/workspace/libavg/src/player/avgdtd.cpp (working copy)
> > @@ -56,6 +56,8 @@
> > " onkeydown CDATA #IMPLIED\n"
> > " onkeyup CDATA #IMPLIED\n"
> > " enablecrop CDATA #IMPLIED\n"
> > +" samplerate CDATA #IMPLIED\n"
> > +" channels CDATA #IMPLIED\n"
>
> I'm not sure, but shouldn't samplerate and channels be in avgrc instead?
>
Can you explain how avgrc is used? A quick look at it seems to indicate that
these parameters should go there too, but I would like to know how these
integrated. Looks to me like this just defines variables that remain fairly
static.
>
> [...]
>
> > +// TODO: do we want these to be dynamically changable?
> > +//void Player::setAudioFrequency(int freq)
> > +//{
> > +// m_AP.m_SampleRate = freq;
> > +//}
> > +//
> > +//void Player::setAudioChannels(int channels)
> > +//{
> > +// m_AP.m_Channels = channels;
> > +//}
>
> Nice to have so tests can run with different frequency/channel setups
>
Sure, but making them dynamic would require a restart of the audio system.
This would mean that we would need to add any locking to make that happen,
and support dynamic resampling context changes in the video decoder. That
may or may not be that big of a deal, but I cannot envisage these being used
for any reason besides testing. As mentioned earlier since these are pretty
static, they belong in avgrc.
>
> > void Player::initConfig() {
> > // Get data from config files.
> > ConfigMgr* pMgr = ConfigMgr::get();
> > @@ -726,9 +743,34 @@
> > m_pDisplayEngine->init(m_DP);
> > }
> >
> > +void Player::initAudio()
> > +{
> > + m_pAudioEngine = new SDLAudioEngine();
> > + m_pAudioEngine->init(m_AP);
> > +
> > + registerAudioSource(m_pRootNode);
> > +
> > + m_pAudioEngine->play();
> > +}
> > +
> > +void Player::registerAudioSource(NodePtr pNode)
> > +{
> > + AudioSourcePtr pAudioSource =
> boost::dynamic_pointer_cast<AudioSource>(pNode);
> > + if (pAudioSource) {
> > + m_pAudioEngine->addSource(pAudioSource);
> > + }
> > +
> > + DivNodePtr pDivNode = boost::dynamic_pointer_cast<DivNode>(pNode);
> > + if (pDivNode) {
> > + for (int i=0; i<pDivNode->getNumChildren(); i++) {
> > + registerAudioSource(pDivNode->getChild(i));
> > + }
> > + }
> > +}
>
> Can't m_pAudioEngine->addSource be called from inside the Node? In
> Video::setDisplayEngine() perhaps? Would probably be a lot less code and
> get rid of the dynamic casts.
>
Would we want to add a Node::setAudioEngine() that is overridden by video
and audio nodes? Seems to make more sense semantically then adding this to
setDisplayEngine(). Alternatively, we could make setDisplayEngine a more
general setRenderingEngines(DisplayEngine, AudioEngine) or something like
that.
>
> Which reminds me: This stuff needs to be tested with nodes that are
> dynamically added and removed from the scene, too...
>
> [...]
> > +//void Player::unregisterNode(NodePtr pNode)
> > +//{
> > +// removeNodeID(pNode);
> > +//
> > +// AudioSourcePtr pAudioSource =
> boost::dynamic_pointer_cast<AudioSource>(pNode);
> > +// if (pAudioSource) {
> > +// m_pAudioEngine->removeSource(pAudioSource);
> > +// }
> > +//
> > +// DivNodePtr pDivNode =
> boost::dynamic_pointer_cast<DivNode>(pNode);
> > +// if (pDivNode) {
> > +// for (int i=0; i<pDivNode->getNumChildren(); i++) {
> > +// unregisterNode(pDivNode->getChild(i));
> > +// }
> > +// }
> > +//}
>
> Possibly implement this in Video::disconnect()?
>
That makes sense.
>
> [...]
> > Index: /home/nick/workspace/libavg/src/player/SDLAudioEngine.h
> > ===================================================================
> > --- /home/nick/workspace/libavg/src/player/SDLAudioEngine.h (revision
> 0)
> > +++ /home/nick/workspace/libavg/src/player/SDLAudioEngine.h (revision
> 0)
> [...]
> > +#define SDL_AUDIO_BUFFER_SIZE 4096
>
> In the long run, we probably want this to be configurable, since
> different setups will have different minimal buffer sizes and hence
> latencies.
>
> [...]
> > Index: /home/nick/workspace/libavg/src/video/AsyncVideoDecoder.cpp
> > ===================================================================
> > --- /home/nick/workspace/libavg/src/video/AsyncVideoDecoder.cpp
> (revision 2460)
> > +++ /home/nick/workspace/libavg/src/video/AsyncVideoDecoder.cpp
> (working copy)
> > @@ -175,6 +175,11 @@
> > return m_bEOF;
> > }
> >
> > +void AsyncVideoDecoder::fillAudioFrame(unsigned char* audioBuffer,
> int audioBufferSize, int channels, int rate)
> > +{
> > + m_pSyncDecoder->fillAudioFrame(audioBuffer, audioBufferSize,
> channels, rate);
> > +}
> > +
>
> Will this work? The rest of m_pSyncDecoder is running in a different
> thread and I don't see any locks, so this will probably cause
> intermittent crashes.
>
It probably will not. I have only tested with the synchronous decoder yet,
so it is very likely that the async one does not work.
>
> The cleanest way I know to make this thread-safe is to add a queue
> between decoder thread and main thread. Have a look at how the video
> frames get passed from the decoder to the main thread to see what I
> mean...
>
I'll take a look.
>
> Cheers,
>
> Uli
>
> --
>
> Ulrich von Zadow | +49-172-7872715
> Jabber: cocacoder at jabber.berlin.ccc.de
> Skype: uzadow
>
> _______________________________________________
> libavg-devel mailing list
> libavg-devel at datenhain.de
> https://mail.datenhain.de/mailman/listinfo/libavg-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.datenhain.de/pipermail/libavg-devel/attachments/20071217/abf634a0/attachment.html
More information about the libavg-devel
mailing list