Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

Dynamic default action and quality for thumbnail information #813

Closed
futjikato opened this issue Mar 8, 2019 · 58 comments · Fixed by #1467
Closed

Dynamic default action and quality for thumbnail information #813

futjikato opened this issue Mar 8, 2019 · 58 comments · Fixed by #1467
Labels
enhancement New feature or request

Comments

@futjikato
Copy link

Feature Request

Make action and quality used in generating thumbnail information in file helper configurable or dynamic in some way.

What problem does this feature solve?

Currently the thumbnail information added to the image data is by default "crop" with quality "good".
See:
https://github.com/directus/api/blob/master/src/helpers/file.php#L210

How do you think this should be implemented?

A simple approach would be to add setting options "Default thumbnail action" same for quality. That could be used as a 4th/5th parameter in the file helper.

A more sophisticated solution would be to read thumbnail settings and create thumbnails for all configured thumbnail actions and qualities. But maybe the current default would result in too many entries ( 2 actions * 4 qualities per size ).

Would you be willing to work on this?

Don't know if this would be a good place for a first contribution but I would be willing to give it a shot.

@benhaynes
Copy link
Sponsor Member

Isn't this covered by the Thumbnailer configuration? I agree that we need a better way to enter these configurations.

https://docs.directus.io/guides/thumbnailer.html#thumbnail-actions

@benhaynes benhaynes added the enhancement New feature or request label Mar 9, 2019
@rijkvanzanten
Copy link
Member

I'm not really following regarding the "Default thumbnail action". Doesn't it allow you to get any source based on the URL structure of the file (where the thumbnail settings act as a whitelist/blacklist)?

@futjikato
Copy link
Author

futjikato commented Mar 9, 2019

I am talking about the thumbnails array that is provided in an image.data.thumbnails. That array is generated by the "get_thumbnails" function ( https://github.com/directus/api/blob/master/src/helpers/file.php#L181 ) and only fetches the thumbnail dimensions from settings and generate a thumbnail URL with action crop and quality better. Doesnt matter what you configure.

@rijkvanzanten
Copy link
Member

Ahhh gotcha

@futjikato
Copy link
Author

As I am fairly new to directus I would be interested to know if this array is considered to be a good place to start in regards to generating thumbnail URLs. Sure, I could get the filename and build the thumbnail URL by myself but it feels more error resistant and maintainable to use that thumbnail array.
Currently I just eplace the string "/good/" with "/better/" and "/crop/" with "/contains/" where needed but maybe I am missing the easy solution ;)

@rijkvanzanten
Copy link
Member

I'm not entirely sure what a good solution would be here. If that array would return all the possible options, we'd end up with tens or even hundreds of links 🤔

The path structure of the thumbnail stays the same, so maybe we can return something like

{
  "sizes": ["200x200", "250x300"],
  "qualities": ["good", "poor"],
  "fit": ["contain"]
}

and you could use that information to dynamically generate the URL?

@benhaynes
Copy link
Sponsor Member

It seems the the ideal solution is to create a custom interface for managing this array. Then we can have a proper GUI for editing the config and still save things in the current format.

@rijkvanzanten
Copy link
Member

I think you're confused @benhaynes. We're discussing the output of the API. Right now, the API returns an array of URLs to the various thumbnails, however its incomplete: It will return urls for the different sizes, but not the different qualities.

@benhaynes
Copy link
Sponsor Member

Yup! Haha... didn't pay attention to the repo.

The thumbnail whitelist should set each set of params, which it might not be doing now. That would give explicit control over what's allowed and avoid the issue of too many combinations being returned.

We use a named quality tag, but could switch to a percentage based one to keep things easier. So a whitelisted thumb value would look like:

200x200,80,contain
250x300,100,crop

Then it would be super easy to return all possible thumbs since each is whitelisted.

Am I on the right track now?

@futjikato
Copy link
Author

That sounds like a proper solution to me 👍

@benhaynes
Copy link
Sponsor Member

To keep this a CSV and since this is already for URLs, this seems cleaner:

200/200/80/contain,250/300/100/crop

We could also have quality last so it's separate from the dimensions and maybe even optional (need to have a single default, like 100).

200/200/contain/80,250/300/crop/90

@benhaynes
Copy link
Sponsor Member

To achieve better clarity/visibility, we are now tracking feature requests within the Feature Request project board.

This issue being closed does not mean it's not being considered.

@hemratna
Copy link
Contributor

Just for the suggestion.

Can we use the crop syntax something like below?
w_300,h_200,c_crop

This gives more flexibility for adding additional parameters without making any major change.

The above syntax is used by many image delivery services like Cloudinary, ImageKit

For more info
https://cloudinary.com/documentation/image_transformations
https://imagekit.io/features/image-resize-smart-crop-responsive-dpr-client-hints

@rijkvanzanten
Copy link
Member

Random thought of the day: should we use a POST request instead and rely on JSON for all the options?

@benhaynes
Copy link
Sponsor Member

benhaynes commented Jul 22, 2019

There are two things to consider here, how we save the settings for this and how we access thumbnails in the URL. Ideally both are the same.

I like the idea of your syntax recommendation @hemratna — but I also don't want to make things overly complex if we can avoid it. Things like zoom, x, y, format, etc are all good options, but would we completely change the way we store thumbnail files? Right now we have a set directory architecture based on the width, height, action, and quality.

With the way I recommended we can have a CSV or whitelisted configurations...
200/200/contain/80,250/300/crop/90

How would we store multiple whitelisted thumbnail configs with this? Use a different character?
w_300,h_200,c_crop

Here is another way of doing it:
https://docs.cloudimage.io/go/cloudimage-documentation/en/operations

And another with query strings:
https://docs.imgix.com/apis/url/size/w

@hemratna
Copy link
Contributor

Random thought of the day: should we use a POST request instead and rely on JSON for all the options?

If you are suggesting let's create a POST endpoint with /thumbnail with all the operation information in JSON then
I think <img src=""> will not do a POST request.

@hemratna
Copy link
Contributor

Store thumbnail

Store all the thumbnail under one directory thumbnails.
All the operational info will be appended in the name as below.

  1. image-{width}x{height}-{operation}-{quality}.jpg
    In this approch, we can handle the optional parameter also.

  2. image-w_300,h_200,c_crop.jpg

Store configuration

Store multiple whiltelisted thumbnail configs as below.

  1. w_300,h_200,c_contain|w_250,h_300,c_crop,q_good
  2. w-300_h-200_c-contain,w-250_h-300_c-crop_q-good

Simply we are looking for 2 different delimiters here.

Pros of this approach is
Every operation is not compulsory. For example, a user can whitelist config like below

  1. w_300|h_300,q_good|h_200

@benhaynes
Copy link
Sponsor Member

Thumbnail Storage

Storing all thumbnails in one directory might have issues when we support folders in the future. When we support folders, you could potentially have files with the same name in different folders:

/directory-a/file-1.jpg
/directory-b/file-1.jpg

Note: Our current solution doesn't work for this either.

This also changes the original file name, which might not be a huge deal, but it's nice that as of now we serve thumbnails with the same name as the original.

It also makes it more difficult to "clear" or remove all of a specific thumbnail type (you'd really need a script to do this). Whereas now you can delete a sub-directory to remove a specific thumbnail group. Not a big deal, but worth noting.

Just wondering, couldn't we make directories with the whitelisted params and then keep the original filename within it? For example: /w_300,h_200,c_crop,q_80/original-filename.jpg

Thumbnail Config

The main reason for the whitelisting is to avoid potential file upload vulnerabilities where malicious users can generate many different thumbnails on the server simply by requesting different permutations. So we'd need to really understand how the Thumbnailer works if fewer config params are set.

For example, if we whitelist w_300 — can the user choose any action, quality, etc? Or are those locked down somehow?

I think one of these below might work (@rijkvanzanten ?), but depending on which we choose we might need to update the tags interface (the one that will manage this Setting) to allow for custom delimiters (a bar in this case).

w_300,h_200,q_80|w_300,h_200,q_80
w_300+h_200+q_80,w_300+h_200+q_80

Lastly, I'm not sure about the "named" quality params — since those would need to be defined/set somewhere. I think a 1-100 quality is more foolproof, and we could make this optional with a single global quality default (eg: 75).

@rijkvanzanten
Copy link
Member

Full disclosure: haven't read everything thoroughly / studied it yet


I like the flexibility, but

w_300,h_200,q_80|w_300,h_200,q_80
w_300+h_200+q_80,w_300+h_200+q_80

reads like an alien language. Docs are going to be absolutely crucial

@benhaynes
Copy link
Sponsor Member

benhaynes commented Jul 23, 2019

I agree. That's why it took me hours to write this... I'm just not happy with how this looks raw. We'll be using the tags interface to manage this, so it will look more like:

w_300,h_200,q_80    w_300,h_200,q_80

But still, I agree that this will need very clear docs.

@shealavington
Copy link

I've only skimmed this thread, there's a lot of discussion haha.

How about considering them like CSS properties? So it "sort of" follows a standard, with the added bonus, that imo, the following feels less "alien" w:300,h:200,q:80 then you have property:value

@rijkvanzanten
Copy link
Member

It does "break" with 'normal' URL key-value pairs though eg w=300

That could also be an option though, query parameters:

/thumbnail/my-image.png?width=300&height=500&quality=100&fit=contain

@rijkvanzanten
Copy link
Member

What different flags do we have @hemratna?

Maybe /width/height/quality/<file>?

/300/150/60/my-image.webp?

@benhaynes
Copy link
Sponsor Member

First we should decide if we want "wildcards" with true or *. For example:

[
  {
    width: true,
    height: 200,
    quality: 80
  }
]

I personally think this leaves us open to the same issues where malicious users can flood the system with unwanted files... but I wanted to discuss before totally dismissing it.

If we're planning on allowing wildcards then we should have nested directories (similar to what we have now):

/w_200/h_300/q_80/original-file-name.ext
/w-200/h-300/q-80/original-file-name.ext
/w=200/h=300/q=80/original-file-name.ext

Or, if we will NOT allow wildcards, we can group them into single folders:

/w_200,h_300,q_80/original-file-name.ext
/w_200+h_300+q_80/original-file-name.ext

@benhaynes
Copy link
Sponsor Member

@rijkvanzanten — one thing we're considering here is that there may be other operations/filters added in the future (thus moving to JSON). So we should make sure the folder structure allows for that too.

https://docs.cloudimage.io/go/cloudimage-documentation/en/operations
https://docs.imgix.com/apis/url/size/w

@hemratna
Copy link
Contributor

@rijkvanzanten — one thing we're considering here is that there may be other operations/filters added in the future (thus moving to JSON). So we should make sure the folder structure allows for that too.

Additionally, I am also thinking like all fields are not required or each field is individually supported too.

There are some possibilities like below.

[
  {
    width: 300,
    height: 200,
    quality: 80
  },
  {
    height: 200,
    fit: crop,
    quality: 80
  },
  {
    width: 300
  }
]

P.S. Optional fields may be part of future enhancement. But I want to make sure we don't have to make major changes in the future.

@rijkvanzanten
Copy link
Member

Wildcards are very dangerous yet very useful. I suppose once we have authentication for thumbnails/files it's less of a problem though

@benhaynes
Copy link
Sponsor Member

Agree. I guess as long as our defaults don't include wildcards, and there's a way to

I think our only default will be the main one used in File Library:

[
  {
    width: 200,
    height: 200,
    fit: crop,
    quality: 80
  }
]

So we need to make sure that this only allows for ONE thumbnail, and not hundreds by exploiting the undefined parameters (now or in the future). For example: the crop positioning, filters, etc. Maybe we can add a filter: false or crop_px: false to ensure those can't be used?

@hemratna
Copy link
Contributor

hemratna commented Aug 1, 2019

We make all the four arguments width, height, fit, duality required.
By enforcing this we can use the current directory structure of thumbnails too.

In the future, we may support optional arguments like filter, but it does not have any effect on directory structure.

@benhaynes
Copy link
Sponsor Member

Perfect. I guess we can figure out how to handle filter in the future... either subfolders (to preserve original file name) or we append/adapt the file name.

Anything else we need to figure out for this one?

@hemratna
Copy link
Contributor

hemratna commented Aug 2, 2019

All set for now. 💪
Let's start working on this issue.

@rijkvanzanten
Copy link
Member

1 final thing to consider: is this a breaking change?

@benhaynes
Copy link
Sponsor Member

If the directory structure is the same, and potentially the URL structure is the same (depends if we want to add the key names), all were changing is how we store the settings... which we should be able to write migration scripts for.

@binal-7span
Copy link
Contributor

binal-7span commented Aug 5, 2019

@benhaynes @rijkvanzanten

If the directory structure is the same, and potentially the URL structure is the same

Okay so let me conclude this conversation here. we will use the old URL only, and will not pass any query string or will not update the directory name. Am I right?

If we are going with this flow then below are some points which we should think once before this change.

  • URL will be changed as we are removing the quality tag with digits. It will be
    200/200/contain/80 instead of 200/200/contain/good

  • We should think for those users who already using this system.

    which we should be able to write migration scripts for.

    Yes, we can use migrations but below are some other cases which we should bear in mind.

    • We will have an n*n*n combination of thumbnails if we are going to migrate their DB.
      Let me explain this with an example. Let's assume below is the data of the user's current DB.
    thumbnail_actions : {
         contain: {
            options: {
               resizeCanvas: false,
               position: center,
               resizeRelative: false,
              canvasBackground: ccc
            }
          },
          crop: {
            options: {
              position: center
           }
          }
        }
    
    thumbnail_quality_tags : {
       poor: 25,
       good: 50,
       better: 75,
       best: 100
    }
    
    thumbnail_dimensions = 50x100,50x50 
    

    So here we will have a combination of number_of_thumbnail_actions * number_of_thumbnail_quality_tags * number_of_thumbnail_dimensions as a thumbnail_whitelist

    We make all the four arguments width, height, fit, duality required.

    If we make all these fields required then what if the user not set their value earlier? I meant what if from number_of_thumbnail_actions * number_of_thumbnail_quality_tags * number_of_thumbnail_dimensions one of this value is null?

    At that time are we supposed to set null as a thumbnail_whitelist as the one combination is not there or any other logic we should implement?

    The required validation will be implemented in the global setting too. But for that, we can throw an error of "Insufficient data". So I think we should think about the old DB.

  • fit will also contain a JSON object.

    [
    {
    width: 200,
    height: 200,
    fit: crop,
    quality: 80
    }
    ]

    This would be,

    {
      width: 200,
      height: 200,
      fit: {
        crop: {
          options: {
            position: center
          }
        }
      },
      quality: 80
    }
    

So let's move to the summing-up;

  1. JSON value of thumbnail_whitelist will be

    {
      width: 200,
      height: 200,
      fit: {
        crop: {
          options: {
            position: center
          }
        }
      },
      quality: 80
    }
    
  2. URL will be
    200/200/contain/80

Thus for the development of this functionality, we should think about the logic of migration.

@benhaynes
Copy link
Sponsor Member

benhaynes commented Aug 6, 2019

Thank you Binal. A few questions:

Creating perfect migrations for this seems impossible, since we're trying to combine different values into one combined value (too many permutations).

If you had the following settings:

{
  width: 200,
  height: 200,
  fit: {
    crop: {
      options: {
        position: center
      }
    }
  },
  quality: 80
}

What if you also wanted a thumbnail with a different position? The URL would be the same, so I don't know how it would be referenced. I think we would either need to have the position be a URL parameter, or not support it.

Problem & Requirements

Taking a step back, I wonder if we should keep the whitelist configuration more simple... and allow for it to also be more permissive/flexible (if desired). Here is what we want/need:

  • A default thumbnail for Directus Internal use
  • A way to limit/block which thumbnails can be created
  • A way to support complex options for thumbnails
  • Simple and intuitive Admin Settings for configuration

Possible Solution

Four thumbnail settings. And for those legacy users migrating, we can just turn on "allow all thumbnails" so nothing breaks in their system (no migrations needed).

system_thumbnails

String that defines the internal Directus Thumbnails, eg: 200/200/crop/80. This could be used if you want to change the details (eg: size, quality, or fit) of the Directus system thumbs.

thumbnail_whitelist

This would be a simple CSV of supported thumbnail sizes, like 50/50/crop/60,400/400/contain/100,100/100/crop/*. These allow you to easily limit the generation to certain thumbnails based on width, height, fit, and quality, but don't support the advanced parameters. All four params are required in the configuration, but ideally we can support * wildcards for any of the params.

allow_all_thumbnails or allow_thumbnail_parameters (boolean)

When enabled, the whitelist is ignored and any thumbnail parameter can be requested/generated using the more advanced syntax discussed above (filters, etc?max_width=400&blur=5. These thumbnails would probably need to be stored using a different folder/naming structure.

thumbnail_defaults

JSON that describes all the default values for thumbnail generation. We don't need this one, but it might be nice if we wanted to make certain params optional.

@binal-7span
Copy link
Contributor

Hey @benhaynes ,

After checking your suggestion it seems like we are removing 3 variable from settings and adding new 4.

So dont you think so we are doing something more difficult?

@benhaynes
Copy link
Sponsor Member

The system_thumbnails and thumbnail_defaults are optional, we don't need them I just thought they would give more control to the admin. We can hardcode the system size and also use that for any defaults.

The main reason for this switch is to allow proper limiting of thumbnails, or generation of any (if needed). These new settings do that but the old ones don't.

Thoughts?

@binal-7span
Copy link
Contributor

binal-7span commented Aug 8, 2019

Yeah, agree.

So can I start the development? As all the doubts are clear now.

@benhaynes
Copy link
Sponsor Member

Great! Let's get the App a bit more stable first (since this is a new feature). Hopefully we can come back to this soon and get it implemented.

@avonian
Copy link

avonian commented Sep 16, 2019

Hi, any updates on this? I ask because I'm in an urgent need of a solution to #1080 which appears to be dependent on this (I'm surprised other users aren't complaining about #1080 since for me it seems a rather serious impairment to the overall utility of file manager)

Is there anything I or others can do to work around this, or help bring about a solution?

@benhaynes
Copy link
Sponsor Member

No updates, but anyone can either submit a PR or sponsor the work to speed up the development. Our team has been super busy with client projects, but hopefully we'll get to this in the next month or so.

@rijkvanzanten
Copy link
Member

@avonian in the meantime, you could try creating a Hook that deletes the thumbnails on the item.update.directus_files action

@benhaynes
Copy link
Sponsor Member

benhaynes commented Oct 9, 2019

Example Asset Data

directus_files.id = h2lh3nv092gj
directus_files.asset_hash = 0kvflkjc01vyl10
directus_files.filename = original-filename.jpg

Original Absolute Path

example.com/<project>/<directus_files.id>
https://directus.media/dcblkqcp/h2lh3nv092gj
                                ^^^ directus_files.id

Generated Asset Path

example.com/<project>/generated/<query>/<directus_files.id>

https://directus.media/dcblkqcp/generated/h300,w500,fcrop,q80/h2lh3nv092gj
                                          ^^^ query          ^^^ directus_files.id 

Original/Generated Asset Middleware

example.com/<project>/assets/<directus_files.asset_hash>?<query>

https://directus.cloud/dcblkqcp/assets/0kvflkjc01vyl10?w=500&h=300&f=crop&q=80
                                       ^^^ directus_files.asset_hash

Middleware would return with original directus_files.filename so downloaded files are named correctly.
Content-Disposition: attachment; filename="original-filename.jpg"

@rijkvanzanten
Copy link
Member

We can also add a name per whitelist name:

before:

https://directus.cloud/dcblkqcp/assets/0kvflkjc01vyl10?w=500&h=300&f=crop&q=80

[
  {
    width: 200,
    height: 200,
    fit: crop,
    quality: 80
  }
]

after:

https://directus.cloud/dcblkqcp/assets/0kvflkjc01vyl10?key=small

[
  {
    key: 'small',
    width: 200,
    height: 200,
    fit: crop,
    quality: 80
  }
]

This would also give us a solution for the folder of the Generated Asset Path:

before:

https://directus.media/dcblkqcp/generated/h300,w500,fcrop,q80/h2lh3nv092gj

after:

https://directus.media/dcblkqcp/generated/small/h2lh3nv092gj

@benhaynes
Copy link
Sponsor Member

benhaynes commented Oct 10, 2019

Just adding that only width, height, fit, and quality would be required (we could discuss defaults for fit/quality if we don't want them to be required)... though there would be other optional parameters that could be added. The first object within the whitelist would be the system thumbnail (so you have the ability to change the system thumb options in the same Setting), and therefore this would be the default value.

[
  {
    width: 200,
    height: 200,
    fit: crop,
    quality: 80
  }
]

And below is an example of a more complex thumbnail whitelist:

[
  {
    key: 'small',
    width: 200,
    height: 200,
    fit: crop,
    quality: 80,
    gravity: top,
    zoom: 1.2,
    radius: 4,
    angle: 90,
    effect: {
        grayscale: true,
        brightness: 60
    }
  },
  {
    key: 'large',
    width: 800,
    height: 600,
    fit: contain,
    quality: 90
  }
]

@binal-7span
Copy link
Contributor

Files can be 2 types.

  • Public (The flow which was defined in the above comments)
  • Private

For the private files; one secret will be passed as a query string. The secret string will contain the expiry time, the user token (For whom this file will belong).
The backend will decrypt it and verified the access.

Though we can consider it as a feature enhancement.

@binal-7span
Copy link
Contributor

binal-7span commented Nov 20, 2019

@benhaynes @rijkvanzanten

Let me brief this whole flow again. Please review it and let me know if anything is missing.


JSON Format

[
  {
    key: 'small',
    width: 200,
    height: 200,
    fit: crop,
    quality: 80,
    gravity: top,
    zoom: 1.2,
    radius: 4,
    angle: 90,
    effect: {
        grayscale: true,
        brightness: 60
    }
  },
  {
    key: 'large',
    width: 800,
    height: 600,
    fit: contain,
    quality: 90
  }
]

Here, key, width, height, fit and quality are required parameter.


URL structure

The directus_fiels.id will be hashed id of the directus_files table. We can use this library to hash the ID.

1. Original Image

example.com/<project>/assets/<directus_files.id>
https://directus.media/dcblkqcp/assets/h2lh3nv092gj

2. Thumbnail

example.com/<project>/generated/<directus_files.id>?key=
https://directus.media/dcblkqcp/generated/h2lh3nv092gj?key=small
  • Here,key is required as a query parameter.
  • If key is not specified then width and height are required (we can consider default value for fit and quality same as the current flow).

Though, the storage will S3 adapter - the URL will be of the server only. The image will serve from the S3 bucket but the URL will be the user's server.


DB Variables

1. thumbnail_whitelist
All the supported thumbnails will be here in a JSON format.

2. allow_all_thumbnails or allow_thumbnail_parameters (boolean)
When enabled, the whitelist is ignored and any thumbnail parameter can be requested/generated. And wildcard[*] support is there if the allow_all_thumbnails is enabled.

3. thumbnail_defaults
JSON that describes all the default values for thumbnail generation.

I didn't add system_thumbnails as system_thumbnails and thumbnail_defaults are almost same. Here is the description of these variables.


Folder Structure

1.Original Image
Original Image will be stored in uploads/originals path

2.Thumbnails

  • Thumbnails will be at uploads/thumbnails/<key>
  • If the allow_all_thumbnails is enabled then those thumbnails will be stored in the others folder.

Thus, the thumbnails folder will contain the <key> folder as well as others folder to store all other thumbs.


Question🤔

Will the other [allow_all_thumbnails] images will be stored directly in the others folder
or we should add a folder insider others with naming convention of the query param like - w_200,h_200,f_crop,q_80?

@benhaynes
Copy link
Sponsor Member

Some updates after discussing internally:

  • We will not allow wildcards, only whitelist objects or ALL (whitelist off)
  • We will only save generated assets in directories named from concatenated parameters (not the keys). This allows users to turn off the whitelist in the future without breaking things, or change the whitelist properties.
  • Thumbnail directories will always be created based on a concat of props, required first, then additional props alphabetically: w200,h200,fcrop,q80,a123,b123,c123
  • We will not have thumbnail_defaults
  • We will add a thumbnail_whitelist_system for cards and avatars
  • We will add thumbnail_whitelist_enabled for toggling on/off the external whitelist (when off, all thumbs are allowed)
  • External generated, and system generated thumbnails will all be in the same concat pram directories... no other directory

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

7 participants