Review Request: Add ffmpegthumbnailer for video previews

[Christoph Feck]

> On 2010-04-27 17:27:56, Christoph Feck wrote:
> > There is probably something I miss, but could you explain
> > 
> > * how your cmake files handle the case when FFmpeg is not found
> > * why the code saves the image data to PNG, then loads it from PNG
> > * why there is code to handle JPEG format and command line arguments when those are not needed?
> > 
> > Thanks.

I now understand ... what you wrote is just a wrapper around another program. Is it possible to use it as an external dependency? If yes, it might be better to add a dependency, instead of copying it. kdemultimedia maintainers would have to decide that, you should post a message to kde-multimedia and refer to this review.

If is is not possible (and you must copy it to KDE), then you could massively strip it, for example, main.cpp is not needed, and the gray scale expansion is handled by Qt, too.

What I meant with "saving then loading PNG" is in ffmpegthumbnailer.cpp, line 47-49. There it writes a PNG stream from the raw image data, then reads it back. It would be better to use QImage data directly.

Regarding the cmake files, you would need something like

if (HAS_FFMPEG)
   macro_optional_add_subdirectory(ffmpegthumbs)
endif (HAS FFMPEG)

This makes it optional on systems where FFMPEG is not available.


- Christoph


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


On 2010-04-27 18:13:57, Andreas Scherf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3818/
> -----------------------------------------------------------
> 
> (Updated 2010-04-27 18:13:57)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This change adds a ffmpegbackend for rendering video thumbnails 
> FFMPEG is required for this. 
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdemultimedia/CMakeLists.txt 1119516 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/CMakeLists.txt PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer.cpp PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/.directory PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/AUTHORS PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/ChangeLog PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/README PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/filmstripfilter.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/filmstripfilter.cpp PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/grayscalefilter.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/histogram.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/ifilter.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/imagetypes.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/imagewriter.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/imagewriterfactory.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/main.cpp PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/moviedecoder.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/moviedecoder.cpp PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/pngwriter.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/pngwriter.cpp PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/stringoperations.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/stringoperations.cpp PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/videoframe.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/videothumbnailer.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/videothumbnailer.cpp PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/videothumbnailerc.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/videothumbnailerc.cpp PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbs.desktop PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/3818/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Dolphin with ffmpegthumbs
>   http://reviewboard.kde.org/r/3818/s/375/
> 
> 
> Thanks,
> 
> Andreas
> 
>