[KPhotoAlbum] new version of the videopatch

Johannes Zarl-Zierl johannes at zarl-zierl.at
Fri Sep 15 22:47:30 CEST 2017


Hi Andreas,

I've noticed another small issue in your code:
You use QString::split() and work around possible empty fields. There's also 
the SplitBehavior parameter [1].

[1] https://doc.qt.io/qt-5/qstring.html#SplitBehavior-enum

Also, I've taken a closer look at the problem you're trying to solve (sorry 
for not doing so earlier, but taking a quick look at the formalities of a 
patch takes quite a lot less time than really understanding it *g*).

For the length extraction, you replace:
ffprobe -v error -select_streams v:0 -show_entries stream=duration -of 
default=noprint_wrappers=1:nokey=1 [filename]

with:
ffprobe -hide_banner [filename]
plus some text parsing.

I've got two issues with this:
1. Parsing text in the default format of ffprobe is likely to break some time 
in the future. Setting the output format explicitly should reduce the likely-
hood of that happening.

2. Maybe it's enough to use the container duration instead of the stream 
duration:
ffprobe -v error -show_entries format=duration -of 
default=noprint_wrappers=1:nokey=1 [filename]

It works for the "surf_video.flv" error clip that you provided earlier. If it 
works for all your clips, we can stick with the simple parsing we had earlier.


On Freitag, 15. September 2017 19:36:38 CEST Andreas Schleth wrote:
> > 2. Disabled code
> > Before merging, I'd like to reduce the amount of code that is commented
> > out. Comments in the manner "we did it this way, now we changed to this
> > and that for this reason" are good, though.
> 
> Done - there were a few remains of markShort ...

Thanks.

> > 3. qDebug()
> > In newer KPA code, we usually define a file-local debug macro (see e.g.
> > BackgroundTaskManager/JobInterface.cpp).
> > If the qDebug() is actually meant for production code, it should probably
> > be a qWarning().
> 
> 1 dDebug exchanged for qWarning. I'd like to keep the commented out
> qDebugs in if I encounter some more strange or broken videos

Better than commenting them out is the following:
1. Add (I've done this already in git master):
#if DEBUG_IMAGEMANAGER
#  define Debug qDebug 
#else
#  define Debug if(0) qDebug
#endif

2. Use "Debug()" instead of "qDebug()"

→ The advantage is reduced bit-rot. E.g. if somebody renames a variable using 
standard refactoring tools, active code is changed accordingly, but comments 
are not.

> Johannes, please feel free to improve on my patchy programming skills
> and change the patch as needed.
> I know a bit about algorithms and such, but unfortunately nearly nothing
> about C++.  All I did, was some imitation of existing code and guessing
> my way around.

As the saying goes:
Fix a patch for somebody and he will code for a day. Teach a man how to fix a 
patch and his patches will be plentiful for evermore... ;-)


I've again run out of KPA time. I'll try to dedicate a little bit more dev-
time on KPA in the (hopefully) near future.

Cheers,
  Johannes


P.S.: @Robert: If you are reading this: I've not forgotten your patchset, I 
simply had no time to look further into it :|


More information about the KPhotoAlbum mailing list