Review Request: Rework transcoding: CollectionLocation asks user, not caller of prepareCopy()

Sat Jan 21 13:31:55 UTC 2012

> On Jan. 21, 2012, 11:56 a.m., Ralf Engels wrote:
> > 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.

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

- Matěj

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103752/#review9975
-----------------------------------------------------------

On Jan. 21, 2012, 12:47 a.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103752/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2012, 12:47 a.m.)
> 
> 
> Review request for Amarok and Teo Mrnjavac.
> 
> 
> Description
> -------
> 
> 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
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt aed51026af8b4b7e826ba0f38c1bce328f78089a 
>   src/browsers/CollectionTreeView.h b70f99d811d1f7f271ec79298c3b0140fbd0ede4 
>   src/browsers/CollectionTreeView.cpp c9069c770fd28fe16e76b1af132f3c7dff4c82d3 
>   src/browsers/filebrowser/FileView.h 890b9f458e7ae504a52019b8f41e3e6d2ba218a5 
>   src/browsers/filebrowser/FileView.cpp b23abc22a91bb8ca7bc8339cc873153c7cb179eb 
>   src/core-impl/collections/db/sql/SqlCollection.cpp ace308b3831deaf51c91dd892e224060d9c03461 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.h 12b975fc7d5c5d56e314f3fce4384eb559e88274 
>   src/core-impl/collections/support/CollectionLocationDelegateImpl.cpp fb7c18f6fef5ef634ef1a40cea604f12a379cfc6 
>   src/core/CMakeLists.txt 6a876e842fc2551763848ebfd3f09a7d35fcc7a6 
>   src/core/capabilities/Capability.h b38d9166797658c99a0162e6dfa7944d47b98de5 
>   src/core/capabilities/TranscodeCapability.h PRE-CREATION 
>   src/core/capabilities/TranscodeCapability.cpp PRE-CREATION 
>   src/core/collections/CollectionLocation.h 375a858aae47b27b5cb9081567b3e384c05d97d8 
>   src/core/collections/CollectionLocation.cpp 01d0f5b8100cd2ad7fb0d3a4fa6d6c4a61e89931 
>   src/core/collections/CollectionLocationDelegate.h c49a7445e260665e508795ae8dbee4ac9f271f71 
>   src/core/transcoding/TranscodingController.h e9b04df6478e1f18d1e8c7d6d9859e6c1c86d9f3 
>   src/core/transcoding/TranscodingController.cpp e16b6fe7d3d60621ef0a237951ac038f7846b51e 
>   src/core/transcoding/TranscodingDefines.h 4ba3e2ae9de710aa7a5f446eddf565e9f5138e4c 
>   src/core/transcoding/TranscodingFormat.h 5bf4e6098be4f3e08dcd45c48fdd264418660293 
>   src/core/transcoding/formats/TranscodingNullFormat.cpp 98f09d1853833d2fd8b8de0603612ae337c6ef52 
>   src/transcoding/TranscodingAssistantDialog.h 718c01d40fed0113087b90e425a9102b365076cb 
>   src/transcoding/TranscodingAssistantDialog.cpp b64e74b9ee75c8b597a07c008a4e161333bb0d86 
>   tests/core/collections/MockCollectionLocationDelegate.h a58ca4b344e98f6b75da19cdd6b38172ecc95f59 
> 
> Diff: http://git.reviewboard.kde.org/r/103752/diff/diff
> 
> 
> Testing
> -------
> 
> Works as before with SqlCollection, works with iPod collection rewrite that also disables (gays-out) unplayable transcoding options.
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120121/2a5b93a4/attachment.html>