[Marble-devel] Review Request 114170: Basic implementation of movie capturing

[Dennis Nienhüser]

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


I implemented most of the things pointed out below locally and uploaded a new patch to https://nienhueser.de/marble/marble-movie-record-2.diff
Feel free to work with that.


src/lib/marble/MovieCapture.h
<http://git.reviewboard.kde.org/r/114170/#comment32106>

    private, unless you plan to use it externally later (for data-driven recording)



src/lib/marble/MovieCapture.h
<http://git.reviewboard.kde.org/r/114170/#comment32105>

    private



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32107>

    QProcess::start() runs asynchronously and is the only thing executed by the thread. Therefore the thread has no benefit here and the process can be called from MovieCapture directly. It might be beneficial to use threads when executing the mapScreenShot() method from there as well. I'm not sure if that is well-defined though.



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32119>

    A static time driven frame rate is usually a bad guess. Either the value is too high and you'll queue up image requests, or too low and you get worse results than the system would support. A better approach is to start recording the next frame a slight amount after the current one has been processed. It's very easy to implement as well: Just call something like
    QTimer::singleShot( 10, this, SLOT(recordFrame()) );
    in recordFrame(), and use a boolean flag to stop recording.
    



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32108>

    the directory name should somehow hint that it is related to Marble. e.g. "Marble-Screencasts"



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32111>

    dir.cd("movie")
    
    The patch records to ~/ currently.
    



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32109>

    Let's execute 'avconv -version' instead. It returns with code 0.



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32118>

    - Output should either be a unique filename, or -y must be passed to avconv to overwrite output files without asking.
    - Set the output frame rate since it will differ from the default one (around 25)
    - Set the output bit rate to avoid getting really bad quality
    - Use a container/codec like webm with a much better compression/quality ratio
    



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32117>

    Each argument must be one item of the string list, not all arguments separated by space as one string



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32116>

    doesn't work, "cannot create children for object created in different thread". I'd avoid using a thread as noted above



src/lib/marble/MovieCapture.cpp
<http://git.reviewboard.kde.org/r/114170/#comment32115>

    Should also inform the user about the location of the movie and clean up single images


- Dennis Nienhüser


On Nov. 30, 2013, 2:37 p.m., Illya Kovalevskyy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114170/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2013, 2:37 p.m.)
> 
> 
> Review request for Marble, Utku Aydın, Dennis Nienhüser, and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> According to Google Code-In 2013 task (http://www.google-melange.com/gci/task/view/google/gci2013/5805119807422464):
> 
> Basic template + implementation of timer-based fps marblewidget caster. Current impl saves 1 fps to home directory in BMP format (as separate files).
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-qt/QtMainWindow.h 80ae250 
>   src/apps/marble-qt/QtMainWindow.cpp a713272 
>   src/apps/marble-ui/icons/16x16/tool-animator.png PRE-CREATION 
>   src/apps/marble-ui/marble.qrc 5ff3a32 
>   src/lib/marble/CMakeLists.txt 8f2c6eb 
>   src/lib/marble/MovieCapture.h PRE-CREATION 
>   src/lib/marble/MovieCapture.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/114170/diff/
> 
> 
> Testing
> -------
> 
> UI/UX, unit-tests
> 
> 
> File Attachments
> ----------------
> 
> UI/UX
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/11/27/743aed4b-bee3-4fdc-b48c-283ae78ee400__menu-entry.png
> Frame files
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/11/27/ddd49c4e-43cb-4608-ab08-5080bc628002__formats.png
> 
> 
> Thanks,
> 
> Illya Kovalevskyy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20131201/597d8ba6/attachment-0001.html>