[KPhotoAlbum] new version of the videopatch

[Andreas Schleth]

Hi,

this is a better version of the patch against v5.2-23-g9e32396e:


The old patch still had a few problems with broken video files and a
segfault in one case. Now, it should produce the thumbs for most files,
even ones, where the stream is broken somewhere. Then it outputs a dummy
image / pixmap. In my case it ran at 20-25 films per minute (1-2 GB
files over NFS) and successfully completed.

The only exceptions is the case, when ffprobe does not show any
indication of video duration.


I still could not test, if the Mplayer-functionality is still there and
working properly.


Cheers, Andreas

[Andreas Schleth]

Hi,

I did some more testing: Mplayer1 was somewhere on my system, not
mplayer2 though.
Thus, I moved away the ffmpeg tools to a safe location and tried KPA
with mplayer1 (which gives a warning).
Et viola, thumbnails and videothumbnails appeared after a while for my
small test data set.
So my patch did not break too much.

Cheers, Andreas

Post by Andreas Schleth
Hi,
The old patch still had a few problems with broken video files and a
segfault in one case. Now, it should produce the thumbs for most
files, even ones, where the stream is broken somewhere. Then it
outputs a dummy image / pixmap. In my case it ran at 20-25 films per
minute (1-2 GB files over NFS) and successfully completed.
The only exceptions is the case, when ffprobe does not show any
indication of video duration.
I still could not test, if the Mplayer-functionality is still there
and working properly.
Cheers, Andreas
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum

[Johannes Zarl-Zierl]

Hi Andreas,

Post by Andreas Schleth
I did some more testing: Mplayer1 was somewhere on my system, not
mplayer2 though.

Thanks for testing!

mplayer2 is (somewhat counter-intuitively) not maintained anymore. It's normal
that you only have mplayer 1. The whole mplayer version mess was a major
incentive for me to add ffmped support in the first place.


Commenting on the patch itself:

1. "Create 20 thumbs and take one"
// Thus it might be unnecessary to produce 20 pngs and then sift through them.

That's very much possible (at least when not using mplayer). I just copied the
approach from the existing mplayer thumbnailing code when adding ffmpeg
support. At this point in time, we might think loudly about dropping mplayer
support alltogether.


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.


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().


Cheers,
Johannes

[Andreas Schleth]

Hi Johannes,

Post by Johannes Zarl-Zierl
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 ...

Post by Johannes Zarl-Zierl
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

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.

Andreas

[Johannes Zarl-Zierl]

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.

Post by Andreas Schleth

Post by Johannes Zarl-Zierl
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.

Post by Andreas Schleth

Post by Johannes Zarl-Zierl
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.

Post by Andreas Schleth
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 :|

[Robert Krawitz]

patchset, I simply had no time to look further into it :|

There are good reasons to read to the end of messages.

I'm all for anything that speeds up thumbnailing.