Review Request: GSoC: Transcoding

Maximilian Kossick maximilian.kossick at googlemail.com
Mon Oct 18 20:30:05 CEST 2010 

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



src/browsers/CollectionTreeView.cpp
<http://git.reviewboard.kde.org/r/100068/#comment86>

    This should really be moved into CollectionLocation, as a first step before CollectionLocation starts calling its sub-classes to do the actual work.
    
    Oh, and what about automatic transcoding, e.g. when copying oggs to an ipod?



src/browsers/filebrowser/FileView.cpp
<http://git.reviewboard.kde.org/r/100068/#comment88>

    This looks like duplicated code, the same as in CollectionTreeView.



src/browsers/filebrowser/FileView.cpp
<http://git.reviewboard.kde.org/r/100068/#comment89>

    This method blocks, which forces Qt to start another event loop. This should be avoided if possible



src/core/transcoding/TranscodingController.cpp
<http://git.reviewboard.kde.org/r/100068/#comment90>

    Static instance methods are evil. That way it becomes really hard to mock the Controller. Please use an interface and a registry, i.e. Amarok::Components to get the necessary indirection to avoid the dependency hell created by static factory methods.



src/core/transcoding/TranscodingController.cpp
<http://git.reviewboard.kde.org/r/100068/#comment91>

    How about a foreach loop instead?



src/transcoding/TranscodingJob.cpp
<http://git.reviewboard.kde.org/r/100068/#comment74>

    How is this going to work for non-local source tracks?


- Maximilian


On 2010-10-17 20:51:11, Teo Mrnjavac wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100068/
> -----------------------------------------------------------
> 
> (Updated 2010-10-17 20:51:11)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> -------
> 
> The attached diff is the work done during this summer to implement transcoding, it should apply to master as of right now. There are some known issues with how FFmpeg behaves with metadata, and I'm not sure I made all the right choices with how I modified CollectionLocation-related stuff.
> 
> 
> Diffs
> -----
> 
>   src/App.cpp 445fdc3 
>   src/CMakeLists.txt f8559c4 
>   src/browsers/CollectionTreeView.h 63c99b5 
>   src/browsers/CollectionTreeView.cpp ed34beb 
>   src/browsers/filebrowser/FileView.h 11547fe 
>   src/browsers/filebrowser/FileView.cpp 9cb7c27 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h f65cd4c 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.cpp a858d44 
>   src/core-impl/collections/sqlcollection/CMakeLists.txt f530d67 
>   src/core-impl/collections/sqlcollection/SqlCollectionFactory.cpp a1966ac 
>   src/core-impl/collections/sqlcollection/SqlCollectionLocation.h ec3a6d8 
>   src/core-impl/collections/sqlcollection/SqlCollectionLocation.cpp 4b023fc 
>   src/core/CMakeLists.txt 5863ca1 
>   src/core/collections/CollectionLocation.h 567f6d3 
>   src/core/collections/CollectionLocation.cpp dbf3b37 
>   src/core/transcoding/TranscodingConfiguration.h PRE-CREATION 
>   src/core/transcoding/TranscodingConfiguration.cpp PRE-CREATION 
>   src/core/transcoding/TranscodingController.h PRE-CREATION 
>   src/core/transcoding/TranscodingController.cpp PRE-CREATION 
>   src/core/transcoding/TranscodingDefines.h PRE-CREATION 
>   src/core/transcoding/TranscodingFormat.h PRE-CREATION 
>   src/core/transcoding/TranscodingProperty.h PRE-CREATION 
>   src/core/transcoding/TranscodingProperty.cpp PRE-CREATION 
>   src/core/transcoding/formats/TranscodingAacFormat.h PRE-CREATION 
>   src/core/transcoding/formats/TranscodingAacFormat.cpp PRE-CREATION 
>   src/core/transcoding/formats/TranscodingAlacFormat.h PRE-CREATION 
>   src/core/transcoding/formats/TranscodingAlacFormat.cpp PRE-CREATION 
>   src/core/transcoding/formats/TranscodingFlacFormat.h PRE-CREATION 
>   src/core/transcoding/formats/TranscodingFlacFormat.cpp PRE-CREATION 
>   src/core/transcoding/formats/TranscodingMp3Format.h PRE-CREATION 
>   src/core/transcoding/formats/TranscodingMp3Format.cpp PRE-CREATION 
>   src/core/transcoding/formats/TranscodingNullFormat.h PRE-CREATION 
>   src/core/transcoding/formats/TranscodingNullFormat.cpp PRE-CREATION 
>   src/core/transcoding/formats/TranscodingVorbisFormat.h PRE-CREATION 
>   src/core/transcoding/formats/TranscodingVorbisFormat.cpp PRE-CREATION 
>   src/core/transcoding/formats/TranscodingWmaFormat.h PRE-CREATION 
>   src/core/transcoding/formats/TranscodingWmaFormat.cpp PRE-CREATION 
>   src/dialogs/OrganizeCollectionDialog.h 2c0adf7 
>   src/dialogs/OrganizeCollectionDialog.cpp 2607e99 
>   src/dialogs/TrackOrganizer.h 9d62467 
>   src/dialogs/TrackOrganizer.cpp 135d520 
>   src/transcoding/CMakeLists.txt PRE-CREATION 
>   src/transcoding/TranscodingAssistantDialog.h PRE-CREATION 
>   src/transcoding/TranscodingAssistantDialog.cpp PRE-CREATION 
>   src/transcoding/TranscodingAssistantDialog.ui PRE-CREATION 
>   src/transcoding/TranscodingJob.h PRE-CREATION 
>   src/transcoding/TranscodingJob.cpp PRE-CREATION 
>   src/transcoding/TranscodingOptionsStackedWidget.h PRE-CREATION 
>   src/transcoding/TranscodingOptionsStackedWidget.cpp PRE-CREATION 
>   src/transcoding/TranscodingPropertyComboBoxWidget.h PRE-CREATION 
>   src/transcoding/TranscodingPropertyComboBoxWidget.cpp PRE-CREATION 
>   src/transcoding/TranscodingPropertySliderWidget.h PRE-CREATION 
>   src/transcoding/TranscodingPropertySliderWidget.cpp PRE-CREATION 
>   src/transcoding/TranscodingPropertySpinBoxWidget.h PRE-CREATION 
>   src/transcoding/TranscodingPropertySpinBoxWidget.cpp PRE-CREATION 
>   src/transcoding/TranscodingPropertyWidget.h PRE-CREATION 
>   src/transcoding/TranscodingPropertyWidget.cpp PRE-CREATION 
>   tests/core-impl/collections/sqlcollection/TestSqlCollectionLocation.cpp 671759f 
> 
> Diff: http://git.reviewboard.kde.org/r/100068/diff
> 
> 
> Testing
> -------
> 
> The patch is the result of git diff for the range of several dozen transcoding-related commits prior to HEAD, rebased on current master, and should build. Transcoding from file browser to collection and inside sqlcollection should definitely work. FFmpeg is required, and Amarok should detect the encoders that are supported by your build of FFmpeg at runtime and populate the dialog accordingly.
> 
> 
> Thanks,
> 
> Teo
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101018/26d5ec79/attachment-0001.htm

More information about the Amarok-devel mailing list