Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable video thumbnail #1035

Closed
wants to merge 1,376 commits into from
Closed

enable video thumbnail #1035

wants to merge 1,376 commits into from

Conversation

tobiasKaminsky
Copy link
Contributor

Requires ffmpeg and ffmpegthumbnailer on server.

@tobiasKaminsky
Copy link
Contributor Author

Update: on server side ffmpgthumbnailer is not needed, but:
'enabledPreviewProviders' => array(
'OC\Preview\Movie'
)
in config.php

@tobiasKaminsky
Copy link
Contributor Author

PR for #897


String url = ((File) mFile).getAbsolutePath();
FileNameMap fileNameMap = URLConnection.getFileNameMap();
String mMimeType = fileNameMap.getContentTypeFor("file://" + url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use FileStorageUtils.getMimeTypeFromExtension(...) instead. That way we avoid introducing java.net.URLConnection in the imports. Let's prevent the temptation of using other network libraries.

@davivel
Copy link
Contributor

davivel commented Aug 10, 2015

The 'play' PNG seems very small for high resolution screens.

Bitmap playButton = BitmapFactory.decodeResource(MainApp.getAppContext().getResources(),
R.drawable.view_play);

Bitmap resizedPlayButton = Bitmap.createScaledBitmap(playButton,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thumbnails are saved always with the same resolution in a given device, right? In that case, the scaled load of the 'play' image and the calculation for its coordinates should be done only once, and kept in static fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since I added resolution dependent play icons for all sizes the scaling shouldn't be needed anymore and the icon should be sharp on all device sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that I need the exact coordinates of the three points forming the triangle to compute the exact visual center of the play button (which is not the center of the image).
So therefore I had modified the original image and cropped all outer space of the play button.
If we do this on all play buttons, I guess it is working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndyScherzinger can I crop the images?
Scaling (or at least resizing) is still necessary as the visual center of the play button is not the center of the image.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tobiasKaminsky sure, go ahead. The pngs are the standard ones provided by googles material design icon repository

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thumbnails are saved always with the same resolution in a given device, right? In that case, the scaled load of the 'play' image and the calculation for its coordinates should be done only once, and kept in static fields.

As video thumbnails can occur in file and grid view, the computation must be in this function, as it depends on the thumbnail size.

@davivel
Copy link
Contributor

davivel commented Aug 10, 2015

Reviewed; please, see my comments.

@AndyScherzinger
Copy link
Contributor

@davivel and @tobiasKaminsky I just pushed a play icon for all the resolutions in standard 24dp sizes. Hope that is fine with you @tobiasKaminsky me simply committing this to your branch.
Please check if you are happy with the new icon size on higher resolution devices.
Also synched with master

@tobiasKaminsky
Copy link
Contributor Author

@davivel fixed and added comments to your review

@jesmrec
Copy link
Collaborator

jesmrec commented Aug 19, 2016

Dear friends @owncloud/android-developers, this branch needs a rebase with master. Does everybody volunteer? :)

LukasReschke and others added 4 commits August 19, 2016 13:32
To prevent tapjacking like attacks we should explicitly enable this attribute. Fore more details see https://blog.lookout.com/look-10-007-tapjacking/

Note that this does not affect all Android devices as for example Samsung started with Ice Cream to mitigate this issue on OS level.

To test this I can recommend the https://github.com/mwrlabs/tapjacking-poc application.
@davivel
Copy link
Contributor

davivel commented Aug 22, 2016

I'll do it, @jesmrec .

@davivel
Copy link
Contributor

davivel commented Aug 22, 2016

Rebased; replacing with a new PR to master

@davivel davivel closed this Aug 22, 2016
@davivel davivel mentioned this pull request Aug 22, 2016
@davivel davivel removed this from the backlog milestone Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.