<br><br><div class="gmail_quote">On Dec 16, 2007 3:22 PM, Ulrich von Zadow <<a href="mailto:uzadow@libavg.de">uzadow@libavg.de</a>> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Lots of comments inline...<br><br>(Very technical & sometimes critical comments. Don't take this to mean I<br>don't value your work. On the contrary!)<br><br>Nick Hebner wrote:<br>> Index: /home/nick/workspace/libavg/configure.in
<br>> ===================================================================<br>> --- /home/nick/workspace/libavg/configure.in (revision 2460)<br>> +++ /home/nick/workspace/libavg/configure.in (working copy)<br>> Index: /home/nick/workspace/libavg/src/avgconfig.h.in
<br>[...]<br>> ===================================================================<br>> --- /home/nick/workspace/libavg/src/avgconfig.h.in (revision 2460)<br>> +++ /home/nick/workspace/libavg/src/avgconfig.h.in (working copy)
<br>[...]<br>Just a nitpick, but these aren't real changes, right? Then they<br>shouldn't be in the patch.<br></blockquote><div><br>Yeah, this patch isn't as clean as it could be.<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>> Index: /home/nick/workspace/libavg/src/player/AudioEngine.cpp<br>> ===================================================================<br>> --- /home/nick/workspace/libavg/src/player/AudioEngine.cpp (revision 0)
<br>> +++ /home/nick/workspace/libavg/src/player/AudioEngine.cpp (revision 0)<br>> @@ -0,0 +1,59 @@<br>> +//<br>> +// libavg - Media Playback Engine.<br>> +// Copyright (C) 2007 Nick Hebner<br><br>To avoid legal issues, I have to ask you to assign the copyright to me:
<br><br>// Copyright (C) 2007 Ulrich von Zadow<br><br>If the copyrights are owned by lots of different people, even a<br>technical license change (such as updating to LGPL 3.0) becomes a major<br>hassle because I have to ask everyone who contributed. Of course, I can
<br>never change the license of the current version, so there is really<br>nothing to fear. (I'm also pretty committed to open source, but you are<br>under no obligation to believe me ;-).)<br><br>You can add a line at the bottom of the copyright blurb if you want, though:
<br><br>// Original author of this file is Nick Hebner (<a href="mailto:hebern@gmail.com">hebern@gmail.com</a>).<br></blockquote><div><br>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.
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>[...]<br><br>> Index: /home/nick/workspace/libavg/src/player/avgdtd.cpp<br>
> ===================================================================<br>> --- /home/nick/workspace/libavg/src/player/avgdtd.cpp (revision 2460)<br>> +++ /home/nick/workspace/libavg/src/player/avgdtd.cpp (working copy)
<br>> @@ -56,6 +56,8 @@<br>> " onkeydown CDATA #IMPLIED\n"<br>> " onkeyup CDATA #IMPLIED\n"<br>> " enablecrop CDATA #IMPLIED\n"<br>> +" samplerate CDATA #IMPLIED\n"
<br>> +" channels CDATA #IMPLIED\n"<br><br>I'm not sure, but shouldn't samplerate and channels be in avgrc instead?<br></blockquote><div><br>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.
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>[...]<br><br>> +// TODO: do we want these to be dynamically changable?<br>> +//void Player::setAudioFrequency(int freq)
<br>> +//{<br>> +// m_AP.m_SampleRate = freq;<br>> +//}<br>> +//<br>> +//void Player::setAudioChannels(int channels)<br>> +//{<br>> +// m_AP.m_Channels = channels;<br>> +//}<br><br>Nice to have so tests can run with different frequency/channel setups
<br></blockquote><div><br>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.
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>> void Player::initConfig() {<br>> // Get data from config files.<br>
> ConfigMgr* pMgr = ConfigMgr::get();<br>> @@ -726,9 +743,34 @@<br>> m_pDisplayEngine->init(m_DP);<br>> }<br>><br>> +void Player::initAudio()<br>> +{<br>> + m_pAudioEngine = new SDLAudioEngine();
<br>> + m_pAudioEngine->init(m_AP);<br>> +<br>> + registerAudioSource(m_pRootNode);<br>> +<br>> + m_pAudioEngine->play();<br>> +}<br>> +<br>> +void Player::registerAudioSource(NodePtr pNode)
<br>> +{<br>> + AudioSourcePtr pAudioSource =<br>boost::dynamic_pointer_cast<AudioSource>(pNode);<br>> + if (pAudioSource) {<br>> + m_pAudioEngine->addSource(pAudioSource);<br>> + }
<br>> +<br>> + DivNodePtr pDivNode = boost::dynamic_pointer_cast<DivNode>(pNode);<br>> + if (pDivNode) {<br>> + for (int i=0; i<pDivNode->getNumChildren(); i++) {<br>> + registerAudioSource(pDivNode->getChild(i));
<br>> + }<br>> + }<br>> +}<br><br>Can't m_pAudioEngine->addSource be called from inside the Node? In<br>Video::setDisplayEngine() perhaps? Would probably be a lot less code and<br>get rid of the dynamic casts.
<br></blockquote><div><br>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.
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>Which reminds me: This stuff needs to be tested with nodes that are<br>dynamically added and removed from the scene, too...
<br><br>[...]<br>> +//void Player::unregisterNode(NodePtr pNode)<br>> +//{<br>> +// removeNodeID(pNode);<br>> +//<br>> +// AudioSourcePtr pAudioSource =<br>boost::dynamic_pointer_cast<AudioSource>(pNode);
<br>> +// if (pAudioSource) {<br>> +// m_pAudioEngine->removeSource(pAudioSource);<br>> +// }<br>> +//<br>> +// DivNodePtr pDivNode = boost::dynamic_pointer_cast<DivNode>(pNode);<br>
> +// if (pDivNode) {<br>> +// for (int i=0; i<pDivNode->getNumChildren(); i++) {<br>> +// unregisterNode(pDivNode->getChild(i));<br>> +// }<br>> +// }<br>> +//}<br>
<br>Possibly implement this in Video::disconnect()?<br></blockquote><div><br>That makes sense. <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>[...]<br>> Index: /home/nick/workspace/libavg/src/player/SDLAudioEngine.h<br>> ===================================================================<br>> --- /home/nick/workspace/libavg/src/player/SDLAudioEngine.h (revision 0)
<br>> +++ /home/nick/workspace/libavg/src/player/SDLAudioEngine.h (revision 0)<br>[...]<br>> +#define SDL_AUDIO_BUFFER_SIZE 4096<br><br>In the long run, we probably want this to be configurable, since<br>different setups will have different minimal buffer sizes and hence
<br>latencies.<br></blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>[...]<br>> Index: /home/nick/workspace/libavg/src/video/AsyncVideoDecoder.cpp
<br>> ===================================================================<br>> --- /home/nick/workspace/libavg/src/video/AsyncVideoDecoder.cpp<br>(revision 2460)<br>> +++ /home/nick/workspace/libavg/src/video/AsyncVideoDecoder.cpp
<br>(working copy)<br>> @@ -175,6 +175,11 @@<br>> return m_bEOF;<br>> }<br>><br>> +void AsyncVideoDecoder::fillAudioFrame(unsigned char* audioBuffer,<br>int audioBufferSize, int channels, int rate)<br>
> +{<br>> + m_pSyncDecoder->fillAudioFrame(audioBuffer, audioBufferSize,<br>channels, rate);<br>> +}<br>> +<br><br>Will this work? The rest of m_pSyncDecoder is running in a different<br>thread and I don't see any locks, so this will probably cause
<br>intermittent crashes.<br></blockquote><div><br>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. <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>The cleanest way I know to make this thread-safe is to add a queue<br>between decoder thread and main thread. Have a look at how the video<br>frames get passed from the decoder to the main thread to see what I mean...
<br><div><div></div><div class="Wj3C7c"></div></div></blockquote><div><br>I'll take a look.<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div class="Wj3C7c"><br>Cheers,<br><br> Uli<br><br>--<br><br>Ulrich von Zadow | +49-172-7872715<br>Jabber: <a href="mailto:cocacoder@jabber.berlin.ccc.de">cocacoder@jabber.berlin.ccc.de</a><br>Skype: uzadow<br><br>_______________________________________________
<br>libavg-devel mailing list<br><a href="mailto:libavg-devel@datenhain.de">libavg-devel@datenhain.de</a><br><a href="https://mail.datenhain.de/mailman/listinfo/libavg-devel" target="_blank">https://mail.datenhain.de/mailman/listinfo/libavg-devel
</a><br></div></div></blockquote></div><br>