<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/103752/">http://git.reviewboard.kde.org/r/103752/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 21st, 2012, 11:56 a.m., <b>Ralf Engels</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;">No obvious technical issues.
However, I am wondering: Is there really a transcoding capability?
I mean, we should be able to transcode everything independent of the collection it's in.
Also, I don't like the concept of the capabilities. The only reason to have these capabilities is to keep the API binary compatible.
But in Amarok we don't really care about that for now.</pre>
</blockquote>
</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;">> However, I am wondering: Is there really a transcoding capability?
> I mean, we should be able to transcode everything independent of the collection it's in.
Yes, I originally thought this would be the right & best approach. Thanks to CollectionLocation::copyUrlsToCollection( const QMap<Meta::TrackPtr, KUrl> ... ) we could transcode tracks somewhere in CollectionLocation and just replace the KUrls with the transcoded ones. But it would have some drawbacks:
* motivation for this change was transcoding for iPod collection rewrite: transferring tracks onto iPod is usually slow (nearly as slow as transcoding). With such a 2-step process it would take twice the time. (because currently it can be done in parallel)
* you would be essentially copying the files twice (witout greater CollectionLocation redesign)
* many collection locations just copy source track metadata such as bitrate, length, filetype etc. This would need to be dealed with.
* I'd still want to have ability to save per-destinaiton-collection preferred transcoding configuration. Collection locations would then have to provide something like {get,has}SavedConfiguration and saveConfiguration. (we don't want this to be handed universally, for example UmsCollection and iPod collection want to store this preferrence on the device)
* The same applies to playableFileTypes() (altough this would maybe fit into CollectionLocation directly)
Therefore I came to the conslusion that each CollectionLocation should implement transcoding in its copyUrlsToCollection() method. This is not really hard, Transcoding::Job facilitates it. So the reason to put this into a capability was to have it self-contained and do not add additional methods to over-crowded CollectionLocation of Collection. But I will happily implement a better design should some suggest such.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On January 21st, 2012, 12:47 a.m., Matěj Laitl wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/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 and Teo Mrnjavac.</div>
<div>By Matěj Laitl.</div>
<p style="color: grey;"><i>Updated Jan. 21, 2012, 12:47 a.m.</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;">Rework transcoding: CollectionLocation asks user, not caller of prepareCopy()
This moves the "get transcoding configuration" from callers of
prapareCopy() to CollectionLocation itself, resulting in hopefully more
predictable user experience.
New capability, TranscodeCapability is created and supposed to be
provided by collections to indicate source CollectionLocation it should
display the transcoding assistant dialog. Default implementation of it
has no pure virtual methods and SqlCollection is changed to provide it,
making this change invisible to user for now.
TranscodeCapability can also tell what formats will be playable on
target collection (so that meaningless codecs can be disabled in the
dialog) and mainly it allows target collections to store their
preferred transcoding configuration so that the user is not bothered
with the dialog every time. (the NULL_CODEC option can store a
preference "just copy") No collection is currently able to do this,
but I will implement it for SqlCollection and rewrite of IpodCollection.
IpodCollection rewrite (not yet in master) is the other coll. that
provides the capability, it already implements playableFileTypes()
TranscodingAssistantDialog is also tweaked so that it disables (not
hides) unavailable transcoding options. (with info in a tooltip) Some
core transcoding classes are cleaned up.
Following bugs are still valid, but this is a first step to solving
them:
CCBUG: 280526
CCBUG: 264681
CCBUG: 291722
DIGEST: groundwork for more convenient transcoding in Amarok</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;">Works as before with SqlCollection, works with iPod collection rewrite that also disables (gays-out) unplayable transcoding options.</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>src/CMakeLists.txt <span style="color: grey">(aed51026af8b4b7e826ba0f38c1bce328f78089a)</span></li>
<li>src/browsers/CollectionTreeView.h <span style="color: grey">(b70f99d811d1f7f271ec79298c3b0140fbd0ede4)</span></li>
<li>src/browsers/CollectionTreeView.cpp <span style="color: grey">(c9069c770fd28fe16e76b1af132f3c7dff4c82d3)</span></li>
<li>src/browsers/filebrowser/FileView.h <span style="color: grey">(890b9f458e7ae504a52019b8f41e3e6d2ba218a5)</span></li>
<li>src/browsers/filebrowser/FileView.cpp <span style="color: grey">(b23abc22a91bb8ca7bc8339cc873153c7cb179eb)</span></li>
<li>src/core-impl/collections/db/sql/SqlCollection.cpp <span style="color: grey">(ace308b3831deaf51c91dd892e224060d9c03461)</span></li>
<li>src/core-impl/collections/support/CollectionLocationDelegateImpl.h <span style="color: grey">(12b975fc7d5c5d56e314f3fce4384eb559e88274)</span></li>
<li>src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp <span style="color: grey">(fb7c18f6fef5ef634ef1a40cea604f12a379cfc6)</span></li>
<li>src/core/CMakeLists.txt <span style="color: grey">(6a876e842fc2551763848ebfd3f09a7d35fcc7a6)</span></li>
<li>src/core/capabilities/Capability.h <span style="color: grey">(b38d9166797658c99a0162e6dfa7944d47b98de5)</span></li>
<li>src/core/capabilities/TranscodeCapability.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/capabilities/TranscodeCapability.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/collections/CollectionLocation.h <span style="color: grey">(375a858aae47b27b5cb9081567b3e384c05d97d8)</span></li>
<li>src/core/collections/CollectionLocation.cpp <span style="color: grey">(01d0f5b8100cd2ad7fb0d3a4fa6d6c4a61e89931)</span></li>
<li>src/core/collections/CollectionLocationDelegate.h <span style="color: grey">(c49a7445e260665e508795ae8dbee4ac9f271f71)</span></li>
<li>src/core/transcoding/TranscodingController.h <span style="color: grey">(e9b04df6478e1f18d1e8c7d6d9859e6c1c86d9f3)</span></li>
<li>src/core/transcoding/TranscodingController.cpp <span style="color: grey">(e16b6fe7d3d60621ef0a237951ac038f7846b51e)</span></li>
<li>src/core/transcoding/TranscodingDefines.h <span style="color: grey">(4ba3e2ae9de710aa7a5f446eddf565e9f5138e4c)</span></li>
<li>src/core/transcoding/TranscodingFormat.h <span style="color: grey">(5bf4e6098be4f3e08dcd45c48fdd264418660293)</span></li>
<li>src/core/transcoding/formats/TranscodingNullFormat.cpp <span style="color: grey">(98f09d1853833d2fd8b8de0603612ae337c6ef52)</span></li>
<li>src/transcoding/TranscodingAssistantDialog.h <span style="color: grey">(718c01d40fed0113087b90e425a9102b365076cb)</span></li>
<li>src/transcoding/TranscodingAssistantDialog.cpp <span style="color: grey">(b64e74b9ee75c8b597a07c008a4e161333bb0d86)</span></li>
<li>tests/core/collections/MockCollectionLocationDelegate.h <span style="color: grey">(a58ca4b344e98f6b75da19cdd6b38172ecc95f59)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103752/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>