<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/100021/">http://git.reviewboard.kde.org/r/100021/</a>
</td>
</tr>
</table>
<br />
<p>On October 4th, 2010, 1:38 p.m., <b>Daniel Dewald</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is far from being complete. It something of a beginning. I just basically removes the ffmpeg stuff but puts nothing into place to replace it. A slot for the audioDataReady signal is created but not implemented anywhere let alone being connected somehow to some audioDataOutput. This has to go a long way before it could work. Also remember that the audioDataOutput was originally invented for visualizing stuff.. so the data wont come in at once.. you'd have to build some kind of buffer (as I did) before this could work. otherwise musicbrainz will either hang or starve.</pre>
</blockquote>
<p>On October 4th, 2010, 4:50 p.m., <b>Kevin Kofler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> I just basically removes the ffmpeg stuff but puts nothing into place to replace it.
Uh yes, it puts a Phonon::AudioDataOutput in place.
> A slot for the audioDataReady signal is created but not implemented anywhere
line 160:
void MusicDNSAudioDecoder::handleFrame(const QMap<Phonon::AudioDataOutput::Channel, QVector<qint16> > &data)
> let alone being connected somehow to some audioDataOutput.
lines 130-131:
connect(&dataout, SIGNAL(dataReady(const QMap<Phonon::AudioDataOutput::Channel, QVector<qint16> >&)),
this, SLOT(handleFrame(const QMap<Phonon::AudioDataOutput::Channel, QVector<qint16> >&)));
> you'd have to build some kind of buffer (as I did) before this could work.
The DecodedAudioData class in the original FFmpeg-based code is already such a buffer. FFmpeg doesn't deliver all the data at once either, the code used a loop. Due to how Phonon::AudioDataOutput works, I replaced the loop with a QThread event loop.</pre>
</blockquote>
<p>On October 4th, 2010, 5:36 p.m., <b>Daniel Dewald</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> line 160:
> void MusicDNSAudioDecoder::handleFrame(const QMap<Phonon::AudioDataOutput::Channel, QVector<qint16> > &data)
Sorry overlooked that because of strange coloring (still dont get this reviewboard stuff). Looking through that stuff now things look better ;o).
> lines 130-131:
> connect(&dataout, SIGNAL(dataReady(const QMap<Phonon::AudioDataOutput::Channel, QVector<qint16> >&)),
> this, SLOT(handleFrame(const QMap<Phonon::AudioDataOutput::Channel, QVector<qint16> >&)));
I also overlooked that because of the color ;o).
> The DecodedAudioData class in the original FFmpeg-based code is already such a buffer. FFmpeg doesn't deliver all the data at once either, the code used a loop. Due to how Phonon::AudioDataOutput works, I replaced the >loop with a QThread event loop.
I've no clue about ffmpeg of how its internal stuff works. May work or may not.. I'd say it won't because I know phonon. If it does I'd be surprised. Some other comments:
- Dont try to get the samplerate via dataout.sampleRate(). I know the docu says it should work but most times and with most backends it wont. I had / have the same problem.
- With most (if not all) backends you wont get ANY data at all (for example VLC or gstreamer). With other it will crash (xine). Your theoretical approach might work (haven't checked all of it) but as for my experience it wont because of phonon and the backends.
- As Leo mentioned before phonon wont give you the data any faster as if you'd play the song in real.. so this will be FUCKING slow ;o).
So my conclusion:
Good approach (I dont know if it could / would work ) and maybe even a smarter solution then what I implemented. But as for now this will have to sit and wait for the phonon stuff to work out I guess. I didn't try any of this though. This is just my brain speaking.</pre>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I can't really see this working well. Also, we depend on FFmpeg anyway (for Teo's Transcoding). And FFmpeg is proven and well maintained.
IMHO we should reject this one, sorry. Thanks for the effort, though.
</pre>
<br />
<p>- Mark</p>
<br />
<p>On October 3rd, 2010, 11:07 p.m., Kevin Kofler wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Kevin Kofler.</div>
<p style="color: grey;"><i>Updated 2010-10-03 23:07:03</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch removes FFmpeg dependencies from the new MusicBrainz fingerprinting by using Phonon::AudioDataOutput instead of FFmpeg directly.
WARNING: This version of the patch is completely untested and may not even build.
In addition, Phonon::AudioDataOutput may not be ready for production use yet (crashes, decoding speed issues).
You have been warned.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">None yet.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>CMakeLists.txt <span style="color: grey">(baacef5)</span></li>
<li>src/musicbrainz/MusicDNSAudioDecoder.h <span style="color: grey">(97976f6)</span></li>
<li>src/musicbrainz/MusicDNSAudioDecoder.cpp <span style="color: grey">(eef459a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100021/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>