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

Thumbnail generation: does not obey aspect ratio / max values for x and y #16250

Closed
tobiasKaminsky opened this issue May 11, 2015 · 28 comments
Closed

Comments

@tobiasKaminsky
Copy link
Contributor

tobiasKaminsky commented May 11, 2015

Hello,

i am currently developing for our android app the posibility to only download reduced images instead of the full sized images.
Therefore I use the following url address to download the image file:
http://localhost/owncloud/index.php/apps/files/api/v1/thumbnail/244/326/bild.jpg

The problem is that a landscape oriented image will get cropped as the x-parameter is smaller than y. But on the client I do not know whether it is landscape or portrait.

So the best for me would be that I specify a max x and max y with the url and get an image that fits these requirements:
on portrait this would be the same
on landscape the thumbnail generation system must obey the x size and scale the image to the requirements.

I have looked into it, but unfortunately I could not solve it.

Can someone help me?
Of course I can try any patch.

Thank you

@oparoz
Copy link
Contributor

oparoz commented May 11, 2015

Could you give some detailed examples of what you mean?
Like dimension of the original file and the expected dimensions of the preview.

Also, bear in mind that there is this problem: #14712

@tobiasKaminsky
Copy link
Contributor Author

I will try. :)
Original size 1024x1280 (portrait)
desired size: 256x230
this will work for a portrait image.

But if the original size is: 1024x1280 (landscape)
and you specify x=256 and y=320 (as this is e.g. your display size) you will get an image with exact these size. But this will crop the image: The image fills the complete display but the left and the right part is cropped.

Regarding #14712 I do not think that problem applies here as the desired size is always smaller than the original size.

Thank you for your very quick reply!

@oparoz
Copy link
Contributor

oparoz commented May 11, 2015

So, in your first example (which is landscape), you end up with an image which is exactly 320x256 and where there has been no cropping since the ratio is exactly the same.

And in the second example, you end up with a 320x256 "extract of the image, but you would like to get a 205x256 image.

Correct?

I think the problem is that the public interface of preview does not have the setKeepAspect() method.
There is a ticket here, asking for more methods:
#12772

One thing which may slow things down is that this would require changing the API to introduce the aspect ratio flag.

@tobiasKaminsky
Copy link
Contributor Author

Oh. My fault.
The first one is portrait. I have changed this.
Sorry for the mess :)

To clarify: The thumbnail api seems not to the x and y values as maxX and maxY. Instead it crops (if neccessary in landscape) the image.
But in landscape it should keep the ratio and resize the image to maxX.

@tobiasKaminsky
Copy link
Contributor Author

Any updates on this?
I have currently created a PR for /android and it can only be merged when this problem is fixed.

Maybe this should be not a thumbnail route but a resizedImage route as these are not really thumbnails...? Then we could avoid the problem of changing the api...

@oparoz
Copy link
Contributor

oparoz commented May 23, 2015

Could you try #16382 and see if it changes things for you? You may need to clear your thumbnail cache or test it with new pictures

@tobiasKaminsky
Copy link
Contributor Author

Unfortunately it does not help.
You can try it yourself:
I have used a landscape image: 600x450 and opened this url:
http://localhost/owncloud/index.php/apps/files/api/v1/thumbnail/300/300/Landscape_2.jpg

I would expect an image with 300 width and (450/2)=225 height and nothing being cropped.
Instead I get a 300x300 image which has been resized to fit completely in height, there is nothing being cropped. But on the left and right (width) it is cropped.

Original
landscape_2

cropped
cropped

desired
resized_landscape_2

I do not know how you want to handle thumbnails:
Should they be always a square and allow croppnig? Then this request should be a new route?
Or should they be like in dolphin (kde file browser) and show always the complete image but resized?

Thank you very much for your help!

@oparoz
Copy link
Contributor

oparoz commented May 24, 2015

I'm sorry, I should have re-read what I wrote earlier.
This is blocked by #12772, so the only alternative is to use another app for your previews. Seems like that thumbnail endpoint was only designed, for thumbnails.

Your problem is that it's going to be difficult to ask people to install a separate app, unless this is an additional feature in the app and so people will do what needs to be done to get it working.

@tobiasKaminsky
Copy link
Contributor Author

This should be in /core as it is now with the thumbnail route.
Then the android app only has to check if the server is recent enough to have this new route.

So my idea is to modify apps/files/controller/apicontroller.php:70
public function getThumbnail($x, $y, $file) {
to
public function getThumbnail($x, $y, $file, $keepAspectRatio = false) {

Then it would not change the current API as it is an optional parameter.
Unfortunately I am not this good at /core programming to add the functionality to previewManager.

@tobiasKaminsky
Copy link
Contributor Author

Any updates on this?
As the related PR for the android app will be merged in nearer future, this is essential.

Thank you!

@oparoz
Copy link
Contributor

oparoz commented Oct 1, 2015

The fix is in this unmerged PR: #18675
The alternative is https://github.com/owncloud/gallery/wiki/RESTful-API#apipreviewfileidwidthheight

@tobiasKaminsky
Copy link
Contributor Author

The PR suggests to wait to OC9? This is very long, or?
The alternative is to require every user to have the gallery app installed or is this automatically installed? If not, this will not be a solution.

What about my suggestion to extend the current API with an optional parameter?
Do you think this could be accepted?

@oparoz
Copy link
Contributor

oparoz commented Oct 2, 2015

Gallery is automatically installed from 8.2, but it's true that an admin could decide to disable it, in which case you could fall back to the broken behaviour.

What about my suggestion to extend the current API with an optional parameter?
Do you think this could be accepted?

Too late as we're past feature freeze.

@tobiasKaminsky
Copy link
Contributor Author

Then Gallery will be the way to go, I guess.
I very little experience with the restful api you have posted.
I tried this to get the file ids:
curl http://tobi:test@localhost/owncloud/index.php/apps/gallery/api/files/list?location=/hidden
but this results in
{"files":[],"albuminfo":{"path":"hidden","fileid":110,"permissions":31,"etag":"560f9409e034c"},"locationhaschanged":false}
with no file informations.
How can I get the needed file ids? (This is only for testing via shell, as later I have these ids via the android app I suppose).

@tobiasKaminsky
Copy link
Contributor Author

Update: I got it partially working with 8.2.
But with 8.1 from app store I cannot access the api. Is it not there?

@oparoz
Copy link
Contributor

oparoz commented Oct 4, 2015

I guess you've figured out how to get the IDs :)

Regarding the app store version, there are two problems:

  1. It's not included
  2. The URI is different galleryplus vs gallery

@tobiasKaminsky
Copy link
Contributor Author

Regarding the IDs: There is a strange behaviour with the android app. The getFileId() function delivers an id value that does not match the value in mysql -> oc_filecache -> fileid for the same file.
Therefore I cannot use your API currently.
This is something I have to check with the android app devs.

But https://tobi:[email protected]/owncloud/index.php/apps/galleryplus/api/preview/23379/400/300 is working and respects the ratio and max values as I need it.
Many thanks for this great app and api 👍

So basically this can be closed (although it would be better to have a fool proof way as the admin can still disable the app and then brake the system).

@oparoz
Copy link
Contributor

oparoz commented Oct 4, 2015

The getFileId() function delivers an id value that does not match the value in mysql -> oc_filecache -> fileid for the same file.

:S That's strange, maybe there are 2 Ids, the local one, which may not have been uploaded yet and the remote one.

is working and respects the ratio and max values as I need it.

Nice. The only caveat is that this endpoint does not let you set the aspect ratio. So it respects it as this is what is needed for a "preview", but you can't ask for one without it. The API can evolve, of course, since the thumbnails endpoint is streaming back thumbnails and some clients can't capture that stream.

So basically this can be closed (although it would be better to have a fool proof way as the admin can still disable the app and then brake the system).

I'd say, keep it open until the WebDAV preview endpoint has implemented all the needed features. Hopefully in 9.0.

@tobiasKaminsky
Copy link
Contributor Author

Yes. There seems to be an internal id.

I tried to add to your app an additional route which accepts the file path, but all I get is a
{"reqId":"VhIg-38AAAEAAHwfFDoAAAAP","remoteAddr":"127.0.0.1","app":"no app in context","message":"CSRF check failed","level":0,"time":"2015-10-05T07:04:31+00:00","method":"GET","url":"/owncloud/index.php/apps/gallery/api/previewByPath/1.jpg/1080/1776"}

Would be great, if you can help me out as I have no experience with owncloud and php.

@oparoz
Copy link
Contributor

oparoz commented Oct 5, 2015

I initially misread your post...

If you add a route, you need to add the proper method to the controller, including the special tags.

But I'm not sure this is the best way forward, as it would require making permanent changes to Gallery...

I'm guessing the problem you're having is that the Android app is using paths as IDs?

@oparoz
Copy link
Contributor

oparoz commented Oct 5, 2015

My main problem with changes right now is that if they're too big, they're not going to be in 8.2 and that's going to be a problem for you.

I could probably squeeze in an extra parameter like path="/dir/file"

@tobiasKaminsky
Copy link
Contributor Author

I was thinking about this route:
/gallery/api/previewByPath/{path}/{width}/{height}
So nearly the same like the existing one, but with a file path instead of the file id.
As this would be an additional route it will not affect any existing code.

On android I have

  • remote path
  • file id (which is not the same as the server file id)
  • remote id, which is a long string, so not a real file id

Therefore I thought it would be the easiest to use the remote path.
But if you think it is too much work, I will try to fetch the real server file id.

@oparoz
Copy link
Contributor

oparoz commented Oct 5, 2015

As this would be an additional route it will not affect any existing code.

Unfortunately, it will, so does adding a new path parameter. It means adding a new test as well.
It can work if it only affects the API and doesn't require any changes outside of the controllers.

Those are the planned changes for 9.0:
owncloud/gallery#408

@tobiasKaminsky
Copy link
Contributor Author

With the help of IRC I have found out that the android app already gets the file id.
Therefore I just can use your existing route.
Great :)

So, we are done here for now.
I keep this issue open that we can maybe later change to the WebDav preview endpoint as you said.

Thanks again for your help!

@oparoz
Copy link
Contributor

oparoz commented Oct 7, 2015

Excellent news!

@tobiasKaminsky
Copy link
Contributor Author

@oparoz any news on the webDav preview endpoint?

@oparoz
Copy link
Contributor

oparoz commented Apr 9, 2016

@tobiasKaminsky - Nope. @PVince81 Is it planned for 9.1?

@davivel
Copy link

davivel commented Jul 5, 2016

If I read correctly the related issues, this is dependent of feature:preview. Daring to label...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants