Review Request: Add ffmpegthumbnailer for video previews

Mon May 10 00:25:32 BST 2010

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

Okay, I was able to build it after some struggling with FindFFmpeg vs FindFFMPEG. If you can address the issues below, I have no further objections.

/trunk/KDE/kdemultimedia/CMakeLists.txt
<http://reviewboard.kde.org/r/3818/#comment5238>

    Two options:

    1) add FindFFMPEG.cmake to kdemultimedia/cmake/modules, and find FFMPEG instead of FFmpeg

    2) ask for merging the changes from FindFFMPEG.cmake into KDE's FindFFmpeg.cmake

    Otherwise swscale is not found and ffmpegthumbs is not build

/trunk/KDE/kdemultimedia/CMakeLists.txt
<http://reviewboard.kde.org/r/3818/#comment5239>

    Fourth argument is the projects URL

/trunk/KDE/kdemultimedia/CMakeLists.txt
<http://reviewboard.kde.org/r/3818/#comment5240>

    Use if (FFMPEG_FOUND AND SWSCALE_FOUND)

/trunk/KDE/kdemultimedia/ffmpegthumbs/CMakeLists.txt
<http://reviewboard.kde.org/r/3818/#comment5243>

    From what I can tell, this directory is only added when FFMPEG_FOUND is true, so you do not need a separate FFMPEG_API variable/check, right?

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer.h
<http://reviewboard.kde.org/r/3818/#comment5244>

    Trailing whitespace (use the diff viewer on reviewboard to see all issues)

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer.cpp
<http://reviewboard.kde.org/r/3818/#comment5249>

    Yes, picking a smart frame is slower, but that feature was added to avoid "black" frames if you hit a film cut at exactly that position, so it might sense to reenable that. Not sure.

    Some smarter idea for the future would be to decode one frame, decide if it is good enough, and if not, seek to e.g. 30% and try again.

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/.directory
<http://reviewboard.kde.org/r/3818/#comment5241>

    Remove this file

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/filmstripfilter.h
<http://reviewboard.kde.org/r/3818/#comment5245>

    Classes with virtual functions should have a virtual destructor

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/moviedecoder.h
<http://reviewboard.kde.org/r/3818/#comment5251>

    I am not sure if it is wise to just clear the definition of INT64_C (note that this is appended to integer constants to make them 64 bit int).

    On my 32 bit system it worked fine, though.

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/moviedecoder.h
<http://reviewboard.kde.org/r/3818/#comment5246>

    Maybe #ifdef ? I get warnings about undefined _API symbols

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/moviedecoder.cpp
<http://reviewboard.kde.org/r/3818/#comment5250>

    toAscii -> toLocal8Bit or you get issues with file names containing non-ASCII characters.

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/moviedecoder.cpp
<http://reviewboard.kde.org/r/3818/#comment5247>

    Did you use Tabs here? The indentation of the source is 4 spaces, I think. You can use Kate to visualize Tabs, or set Tab size to 8 spaces to see how bad it looks :)

/trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbs.desktop
<http://reviewboard.kde.org/r/3818/#comment5242>

    Remove obsolete Encoding line

/trunk/KDE/kdemultimedia/ffmpegthumbs/tests/ffmpegthumbtest.cpp
<http://reviewboard.kde.org/r/3818/#comment5248>

    The test is useless if you hardcode a filename that I do not have. If you really want a (manual) test, please add some command line parsing.

    (I think a general KDE thumbnail command line tool might be better, maybe it even exists already?)

- Christoph

On 2010-05-09 21:53:22, Andreas Scherf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3818/
> -----------------------------------------------------------
> 
> (Updated 2010-05-09 21:53:22)
> 
> 
> 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 1123743 
>   /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/histogram.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/ifilter.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/imagewriter.h PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/ffmpegthumbnailer/imagewriter.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/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/ffmpegthumbs.desktop PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/tests/CMakeLists.txt PRE-CREATION 
>   /trunk/KDE/kdemultimedia/ffmpegthumbs/tests/ffmpegthumbtest.cpp 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
> 
>