[libavg-devel] Audio Support Patch

[Ulrich von Zadow]

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.

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

[...]

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

[...]

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

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

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()?

[...]
> 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.

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

Cheers,

  Uli

-- 

Ulrich von Zadow | +49-172-7872715
Jabber: cocacoder at jabber.berlin.ccc.de
Skype: uzadow