[RFC] Video-lite by bstoeger · Pull Request #1463 · subsurface/subsurface

bstoeger · 2018-07-05T10:49:02Z

Describe the pull request:

Pull request long description:

Because video-playback in #1456 was received with reservations, here is the lite-version: Import videos, but only show a dummy icon. On clicking the icon, the video is shown in the system viewer.

I hope we agree that nowadays this is a must-have feature.

Changes made:

Import dummy video icon.
Add list of video extensions and recognize video files.
Save to thumbnail file with an appropriate flag signaling "video".

Import a video icon from the KDE breeze theme, which is licensed under the LGPL. This icon will be used as a dummy-placeholder for video files. Source: https://github.com/KDE/breeze-icons Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

neolit123 · 2018-07-05T13:10:58Z

@bstoeger

BTW, we can only include icons and media that comes with a certain license or permissive origin that won't cause legal troubles. @dirkhh can elaborate.

one clean options is that we draw it our selfs.

bstoeger · 2018-07-05T13:18:50Z

BTW, we can only include icons and media that comes with a certain license or permissive origin that won't cause legal troubles. @dirkhh can elaborate.

Sure, that's why I took an icon of the LPGLed Breeze KDE theme. We already use them for mobile.

one clean options is that we draw it our selfs.

Definitely not an option if I am supposed to draw it.

dirkhh · 2018-07-05T15:19:19Z

Definitely a good start. I haven't stared at the code long enough, but conceptionally I like this much better. For the record, I'm not saying NO to video playback, I am voicing concerns. I am still open to being convinced that this is manageable from a packaging perspective.

neolit123 · 2018-07-05T16:06:21Z

+// codecs? Do we have to query them by hand using QMediaPlayer::hasSupport()?
+const QStringList videoExtensionsList = {
+	".avi", ".mp4", ".mpeg", ".mpg", ".wmv"
+};


the list of extensions looks fine to me, as is.

re: "dodging the bullet"
the point is that we shouldn't care about codecs.

obsolete member to list the MIMEs:
http://doc.qt.io/qt-5/qmediaplayer-obsolete.html
support is only an estimate:
http://doc.qt.io/qt-5/qmultimedia.html#SupportEstimate-enum

neolit123 · 2018-07-05T16:07:07Z

-	return res;
+
+	// Thumbnails of videos currently not supported - replace by dummy
+	// TODO: Correctly extract thumbnails


should we really keep this TODO?

Maybe tone it down to
// TODO: Possibly extract thumbnail
?

neolit123 · 2018-07-05T16:09:45Z

+	if (!fi.exists() && !fi.isFile())
+		return false;
+	metadata md;
+	return get_metadata(qPrintable(filename), &md) == MEDIATYPE_VIDEO;


not sure if this would work for all containers out there.
might as well just assume by extension and throw the file to the external player.

If you look at https://github.com/Subsurface-divelog/subsurface/blob/master/core/metadata.cpp#L208 we try to parse the MP4 container - so this seems like a reasonable thing to do for us? This at least doesn't blindly accept any extension.
(Of course I worry about the security implications here... reading that code it should be fairly easy to fool us and to at least crash Subsurface if not worse)

@neolit123 is in principle right - it will parse MP4s, but not AVI. So if that fails, we can still go after extension.

But: even if we fail to identify a video as such, it will simply show a broken icon (we could refine to a format unknown icon). Clicking on it will still open the system video player.

Of course I worry about the security implications here... reading that code it should be fairly easy to fool us and to at least crash Subsurface if not worse)

I tried to make it at most a denial-of-service, which you can do in various ways anyway. If you think this is unsafe, I'll give it a second look in the next few days.

No, I didn't see anything that glaringly unsafe. This was more an idle comment...

dirkhh · 2018-07-05T19:40:55Z

 {
 	QImage thumb;
-	bool stillLoading = false;
+	mediatype_t type = MEDIATYPE_UNKNOWN;


we set type here to UNKNOWN and a few lines later we test if it is != STILL LOADING. What am I missing?

never mind. std::tie confused me

As I said in a previous PR:
We can replace std::pair<QImage, mediatype_t> by

struct SomeSensibleClassName { QImage thumbnail; mediatype_t type; };

Then the tie will become:

SomeSensibleClassName some_sensible_variable_name = fetchImage(localFilename, filename, tryDownload);

So far I was simply too lazy.

Yeah, that would have more matched my experience.
I keep thinking this is just a sign that I'm not really a C++ developer. But realistically, the struct way of doing this does allow you to call a function like this from C as well (assuming it's linked with C bindings)

But realistically, the struct way of doing this does allow you to call a function like this from C as well (assuming it's linked with C bindings)

Except that C will not be happy about the QImage... :(

If you prefer the struct-version, that's fine by me. The hardest part will be coming up with a sensible name. Perhaps simply struct Thumbnail?

So here's the struct version.

dirkhh · 2018-07-05T19:45:07Z

It is so embarrassing how much this is already pushing my comfort zone reading code.
I agree with @neolit123 's comment about the TODO - other than this I couldn't find anything wrong, but given that this is clearly not easy to read for me, I'm also not super comfortable with my ability to successfully review this. So I didn't actually approve the PR :-/

When generating thumbnails, test for video files. If it is, use a dummy-thumbnail. Write only the type (video), but no image to the thumbnail cache, for forward-compatibility. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

sfuchs79 · 2018-07-08T21:01:48Z

@bstoeger
Great that we have the video support in master now! I'll try to continue testing this during the next days.

May I make a few suggestion (again) for fine tuning of the UI:

Now that we support pictures and video let's rename every occurrence of "pictures" e.g. the tab name, the context menu string,... to s.th. else. I once already proposed "media" but I'm open to any other idea.
For the file open dialog for the file filter I think it could be nice to have minimum three options: All pictures plus video files, picture files only, video files only.

I can submit a PR for this in a few days or maybe you like to do it but let's first discuss how we want to change it.

BTW: I also would not give up video playback in the profile immediately. I want to try once to recompile my Qt/MXE with the "-wmf-backend" option to see if this leads to s.th.

bstoeger · 2018-07-09T11:53:20Z

I can submit a PR for this in a few days or maybe you like to do it but let's first discuss how we want to change it.

As you wish. I think the discussion would be ideally conducted at the mailing list, not in a closed PR. My personal opinion: "Media" is fine.

BTW: I also would not give up video playback in the profile immediately. I want to try once to recompile my Qt/MXE with the "-wmf-backend" option to see if this leads to s.th.

I opened a new PR with the video-playback. Perhaps this helps you with these experiments.

Dive pictures: Import video icon

6b2bb0f

Import a video icon from the KDE breeze theme, which is licensed under the LGPL. This icon will be used as a dummy-placeholder for video files. Source: https://github.com/KDE/breeze-icons Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

neolit123 added enhancements-we-intend-to-add dive-video labels Jul 5, 2018

neolit123 reviewed Jul 5, 2018

View reviewed changes

dirkhh reviewed Jul 5, 2018

View reviewed changes

Dive pictures: Recognize video files

910b0d9

When generating thumbnails, test for video files. If it is, use a dummy-thumbnail. Write only the type (video), but no image to the thumbnail cache, for forward-compatibility. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

bstoeger force-pushed the video5 branch from 66cc920 to 910b0d9 Compare July 5, 2018 22:02

dirkhh merged commit b28dba6 into subsurface:master Jul 8, 2018

This was referenced Jul 8, 2018

Fixes / improvements to video-lite PR [#1463] #1478

Closed

Dive pictures: improve recognition of video files. #1479

Merged

bstoeger deleted the video5 branch April 7, 2020 15:20

Uh oh!

Conversation

bstoeger commented Jul 5, 2018

Describe the pull request:

Pull request long description:

Changes made:

Uh oh!

neolit123 commented Jul 5, 2018

Uh oh!

bstoeger commented Jul 5, 2018

Uh oh!

dirkhh commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bstoeger Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirkhh commented Jul 5, 2018

Uh oh!

sfuchs79 commented Jul 8, 2018

Uh oh!

bstoeger commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bstoeger Jul 5, 2018 •

edited

Loading