Mattermost-server: [MM-5838] Show a generic thumbnail when max image dimensions are exceeded

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide, specifically for machine setup and for developer workflow.

---

Notes: Jira ticket

When uploading an image larger than the maximum image dimensions specified here, the upload is rejected because we cannot load the whole thing into memory to generate the smaller images used for thumbnails and previews. Instead, we should allow the file to be uploaded, but the web and mobile apps should only show the generic thumbnail for images (found here) rather than generating a custom thumbnail of the uploaded image.

To accomplish this, we should modify the server to return to clients whether or not an image has a custom thumbnail/preview. This should be done by adding an ImageURLs field to the FileInfo object returned to the server, and inside that ImageURLs field, we should include URLs for the thumbnail and preview images should those exist. If a preview or thumbnail doesn't exist for an image, they should be left out.

The URLs sent to the client will be the same as the ones the web app currently constructs itself to get the files (like /api/v4/files/<file ID>/thumbnail). They shouldn't be stored in the database since they can be generated based off whether or not the PreviewPath and ThumbnailPath fields are set on the object. They should also be relative URLs because that will allow us to support cases like on our community server where the same server is accessible through both community.mattermost.com and community-daily.mattermost.com.

The web and mobile apps should then be updated to use these new fields (instead of constructing the URL manually like they do now) and to display the generic thumbnail (found above) when those paths aren't provided.

Care should be taken to ensure that the same issue is fixed for images imported via the Slack Import feature.

[s4kh]

Instead, it should accept the upload if it is less than the maximum upload file size. Shouldn't this be changed to greater than. I am kind of confused first it says the upload is rejected then it says if less than it should accept it. If the size is less won't the system accept the file anyway?

[jasonblais]

@S4KH It's correct -- if the dimensions are larger (with **file size being smaller), it should still be accepted, but with a generic thumbnail for images.

If the file size is larger, the behaviour shouldn't change, and the user should receive a message along the lines of File above 50MB cannot be uploaded

[s4kh]

 // Check dimensions before loading the whole thing into memory later on
 if info.Width*info.Height > MaxImageSize {
     err := model.NewLocAppError("uploadFile", "api.file.upload_file.large_image.app_error", map[string]interface{}{"Filename": filename}, "")
     err.StatusCode = http.StatusBadRequest
     return nil, err
 }

Does above code also checks image size validation in terms of file size? And we don't even have to generate the thumbnails, right? How about previewImage?

[ry-wang]

I can try taking a look at this!

[jasonblais]

@R-Wang97 Anything we can help with for this ticket?

[johncoleman83]

The above link to the "generic thumbnail for images" returns 404 response. Is there an updated link for that?

[jasonblais]

@johncoleman83 Fixed the link, it's https://github.com/mattermost/mattermost-webapp/blob/master/images/icons/image.png

[ry-wang]

@jasonblais Sorry I haven't been able to work on this ticket, if someone else has started working on it they can take it :)

[johncoleman83]

I am studying to see how to make this update. To confirm (@S4KH), I believe that the file size > MaxFileSize (default: 50mb) check occurs here in the API. For the image dimensions size check, theoretically, I'm not sure how to respond when image dimensions are greater than MaxImageSize. Some ideas are:

• Add 1 _"generic thumbnail for images"_ to the file storage, and then for cases where the dimensions are greater than MaxImageSize, that path to generic thumbnail image is stored to info.ThumbnailPath.
• A new generic image added and new path set for every image with dimensions greater than MaxImageSize.
• info.ThumbnailPath is set to nil or some flag, and the client side webapp responds accordingly with an image link from the frontend.

[lindy65]

Hi @johncoleman83,

Thanks for your feedback,

Our devs prefer your first option as long as the 1 image file is stored in a path that's not under the "date" folders like the real uploads are.

If that's not possible, then your second option should work best.

Let us know if you have any additional question :)

[johncoleman83]

I just made a WIP - PR because I have not yet added tests. I wanted to open up the current solution that I am working on and approach for consultation from the maintainers/ developers. One comment is that the generic thumbnail is never added to the app store, and I was not sure if is a change that should be added.

[lindy65]

Hi @johncoleman83,

Thank you! I'll ask our devs to chime in here or on the PR :)

[saturninoabril]

Thanks @johncoleman83!
I've made comments on your WIP PR.

[johncoleman83]

@saturninoabril or @lindy65... did you see my responses to you? I have a few questions about your comments.

[lindy65]

Hi @johncoleman83,

I see saturninoabril has commented on one of your PRs and dev review has been requested on the other so you should hear back soon!

[saturninoabril]

@johncoleman83 Sorry for the delay. I've responded to your comments to server PR. Also. let's wait for @hmhealey's review for his better approach in handling thumbnail path.
Thanks for your contribution!

[jasonblais]

Hey @johncoleman83

Looks like there was a change in the proposal made here https://github.com/mattermost/mattermost-server/pull/7820#issuecomment-346085395

Wondering if there was anything we could help with? Please let us know if any.

[theblueskies]

@jasonblais Is this ticket still open/valid?

[johncoleman83]

Sorry I stoped working on this because there were mixed opinions from Mattermost Devs and it seemed like there was not yet a consensus about theoretically how this would be achieved (I wanted to wait until there was clear instructions). In my spare time, I'm working on another integration for PagerDuty / Mattermost now, that I hope to have completed in next month, and so I will not be looking at this until the other is completed.

[jasonblais]

Hey @theblueskies! The conclusion of the discussion was posted here: https://github.com/mattermost/mattermost-server/pull/7820#issuecomment-346085395. If you're interested working on this improvement, let us know! Would love to help.

And thanks @johncoleman83 for working on the PagerDuty integration, I'm really excited about it :)

[bullsseye]

Hi everyone. Should not this issue be tagged under react native as it more resolves around the client side handling of no thumbnail found.

[jasonblais]

Thanks @bullsseye! Agree, have added ReactJS label as well.

[bezumkin]

Maybe it will help, but I just tried to change this code and it works as I want:

/*if t.fileinfo.Width*t.fileinfo.Height > MaxImageSize {
    return t.newAppError("api.file.upload_file.large_image_detailed.app_error",
        "", http.StatusBadRequest)
}*/
t.fileinfo.HasPreviewImage = t.fileinfo.Width*t.fileinfo.Height <= MaxImageSize

I uploaded 2 images, almost the same size in megabytes but different dimension.

As you can see, both uploaded without error but bigger one (5197 × 7795) shows in full size, and second (2599 × 3898) is loaded as preview.

I work in creative agency and we send big images to each other very often. I really hope we could get rid of hardcoded MaxImageSize option somehow.

[M-ZubairAhmed]

I have had discussion with @jasonblais regarding this issue, this issue needs to be updated to the new flow, awaiting for further discussions after holidays

[M-ZubairAhmed]

I am all done with pending tickets from myside, 2 tickets are blocked. So i would like to take this one up, with discussion from @jasonblais

[jasonblais]

Thanks! I've assigned to you and looks like Harrison will update the description. If you haven't heard back in a few days, let me know :)

Linking to the conversation in community.mattermost.com: https://community.mattermost.com/core/pl/ogm9zd9qr3drzjckemamzbgyea

[M-ZubairAhmed]

Thanks @jasonblais for following it up

[hmhealey]

FYI, I've updated this ticket with a new method for going about this. Please ignore my previous comments on how to do this since I think the new solution for this is much better.