Skip to content

Conversation

@tobiasKaminsky
Copy link
Member

Enable video thumbnail.
Needed:
'enabledPreviewProviders' => array(
'OC\Preview\Movie'
)
in config.php

@jancborchardt we need a video thumbnail.

@jancborchardt
Copy link
Member

Nice! :) But does this mean that every single person needs to add this config option to their config? Can’t we enable that by default? cc @LukasReschke

@Spacefish
Copy link
Member

@jancborchardt you need to activate this in your server, but this patch is needed to even be able to see videothumbs in the android app +1
Enabling the Video Preview Provider by default would be nice, but we should keep security in mind. There are multiple bugs lately in ffmpeg which allowed code execution due to malformed container / bytestream headers.. Just think of the stagefright bug in android.

registerLongClickListener();
} else {
switchToListView();
// switchToGridView();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if unused, please remove

@tobiasKaminsky
Copy link
Member Author

@jancborchardt not every user, but every admin.
I have talked to @LukasReschke on IRC an he says that this would be a security problem as the previews are currently not generated in a sandbox.

int x3 = 0;
int y3 = 0;

double ym = ( ((Math.pow(x3,2) - Math.pow(x1,2) + Math.pow(y3,2) - Math.pow(y1,2)) *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of zeros in the calculations so this probably can be simplified ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt so, as this is a mainly pythagoras and he needs three coordinates.
To simplify it, I have chosen them to be mostly zero.

@jancborchardt
Copy link
Member

not every user, but every admin.

@tobiasKaminsky yeah, this is still a bit bad cause this results in different UX for different people depending on the server, and this should ideally not happen. Since it’s a security issue, is it possible to generate the previews client-side as well?

@tobiasKaminsky
Copy link
Member Author

@jancborchardt when the previews are generated in a sandbox the generation of videos can be enabled by default.
The fact that it is a security risk we should not enable preview on client. Beside the security risk, it is also very difficult as the video has to be downloaded prior to generate a preview locally.
So I think we should do it this way and provide previews for those (home) servers that enable the preview generation and for the others we should provide a good video thumbnail.

@AndyScherzinger
Copy link
Member

@jancborchardt as far as I recall we do, yes.

@tobiasKaminsky
Copy link
Member Author

@AndyScherzinger can you make video thumbnails for me?
@jancborchardt yes, play button as overlay on video thumbnails, both in list and grid view

@AndyScherzinger
Copy link
Member

@tobiasKaminsky absolutely, do you need an icon which is shown when there is no thumbnail or a "play icon" overlay which is put on top of a video thumbnail? (can do both, whatever you need 😄)

@tobiasKaminsky
Copy link
Member Author

Placeholder when there is no video based thumbnail available.

@AndyScherzinger
Copy link
Member

@AndyScherzinger
Copy link
Member

These are already in use for all videos atm since we didn't have video thumbnails yet. :)

@tobiasKaminsky
Copy link
Member Author

You are right, then we are done here

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Jun 26, 2016

@tobiasKaminsky can you rebase and resolve the conflicts? I can also give it a try but I guess it is saver when you do it since it is your PR. I'll test drive it afterwards. Code changes look good to me so far :)

@AndyScherzinger
Copy link
Member

The placeholders work fine for me, the real video thumbnails need to be checked by someone else :(

device-2016-07-02-181725

@tobiasKaminsky
Copy link
Member Author

This is already working :)
But we have to get this running on your pc ;-)

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Jul 14, 2016

Works, see screenshot. But only if the server already created the thumbnail. At first I didn't log into the web view and opened the folder which seems to trigger the thumbnail generation. After I did that the Android client was able to fetch the thumbnails. Before that you receive a {"message":"File not found."}

device-2016-07-14-223419

@AndyScherzinger
Copy link
Member

:shipit:

@AndyScherzinger
Copy link
Member

@przybylski do you have the time to do a review? I already did one :)

@przybylski
Copy link
Member

@AndyScherzinger I already did a review some time ago, if nothing changed since than 👍

@AndyScherzinger
Copy link
Member

Sorry @przybylski I missed that :(

@tobiasKaminsky can you update it to latest master for another test round before merging?

@tobiasKaminsky
Copy link
Member Author

@AndyScherzinger is updated

@AndyScherzinger
Copy link
Member

Sweet, will do a test tomorrow or beginning of next week.

@AndyScherzinger
Copy link
Member

Test successful, can imho be merged. 👍

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Jul 20, 2016

LGTM is broken again... grrr

@AndyScherzinger
Copy link
Member

👍

@AndyScherzinger AndyScherzinger merged commit 1876746 into master Jul 20, 2016
@AndyScherzinger AndyScherzinger deleted the video_thumbnail branch July 20, 2016 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants