Moodle

Online image editor

Details

  • Type: New Feature New Feature
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: 1.8
  • Fix Version/s: None
  • Component/s: General
  • Labels:
    None
  • Affected Branches:
    MOODLE_18_STABLE

Description

It would be really great if Moodle had an online image editor. At the moment the only way to resize an image is by changing the height and width attributes in the HTML editor. This works a lot of the time, but if a teacher uploads a 3mb 3000px x 3000px image and then resizes it to 200px x 200px, then Moodle will still send the browser an image 3mb in size!

I recently came across an AJAX/PHP image editor (http://www.ajaxprogrammer.com/?p=9) that acts in a very clean, quick manner. Maybe something like it could be considered?

Sorry that I'm just asking for this and not implementing it myself, but I'm not really a PHP programmer!

Activity

Hide
Darcy W. Christ added a comment -

I would articulate this request in a different way. I recently posted to the forum (http://moodle.org/mod/forum/discuss.php?d=194026), but i'll try to summarize my theory approach to this.

Moodle already has code to produce alternatively sized images (icons are produced using GD, lib/gdlib.php). The library could be extended to provide a more site-wide approach to producing additional sizes for an image. This could be handled via param modifers to the various image rendering scripts (pluginfile.php and draftfile.php) or just to the lib/filestorage/stored_file.php file (ie. readfile method). This would allow for different sized images to be produced dynamically and saved.

Some issues to contend with:

  • auto-generating images scripts do not permit any sized image to be produced as it can expose a vulnerability
  • html resizing can be used to 'trigger' the image generation, with considerations to previous point
  • typically this feature is most useful within the context of the htmleditor and should likely be performed when a user requests a lighter image be produced.
  • system-wide defaults can be established so that administrators have control over the general size of images being produced and whether the default is to produce a lighter image, nevertheless the ability to opt-out is also available.
Show
Darcy W. Christ added a comment - I would articulate this request in a different way. I recently posted to the forum (http://moodle.org/mod/forum/discuss.php?d=194026), but i'll try to summarize my theory approach to this. Moodle already has code to produce alternatively sized images (icons are produced using GD, lib/gdlib.php). The library could be extended to provide a more site-wide approach to producing additional sizes for an image. This could be handled via param modifers to the various image rendering scripts (pluginfile.php and draftfile.php) or just to the lib/filestorage/stored_file.php file (ie. readfile method). This would allow for different sized images to be produced dynamically and saved. Some issues to contend with:
  • auto-generating images scripts do not permit any sized image to be produced as it can expose a vulnerability
  • html resizing can be used to 'trigger' the image generation, with considerations to previous point
  • typically this feature is most useful within the context of the htmleditor and should likely be performed when a user requests a lighter image be produced.
  • system-wide defaults can be established so that administrators have control over the general size of images being produced and whether the default is to produce a lighter image, nevertheless the ability to opt-out is also available.
Hide
Darcy W. Christ added a comment -

I've begun work on a new version of the function process_new_icon in lib/gdlib.php. This function essentially does what we want except that it produces two sizes at once and saves them to the icon file area. I figure we could make a more general function, process_new_image(), which would

a) produce images of whatever size requested
b) save it to wherever requested

process_new_icon() could become a thin wrapper around process_new_image().

Two points to consider:

1) do we need performance, such that this function can handle an array and generate more than one size, as opposed to calling it more than once and opening the original more than once?
2) should the location of these new images be something like filedir/<content_hashed_trie_path>_<size> (ie. filedir/69/d9/69d9284ead0ee950ff3ff8bb663162230bde227b_large) or is there something better? In other projects, my content_path would be a directory that includes all sizes, but that doesn't work with existing images. You can also choose to group sizes together, but since filedir is not meant to be traversed, this isn't a meaningful idea. No matter what, we'll need to be able to delete all sizes when removing an unused file.

Show
Darcy W. Christ added a comment - I've begun work on a new version of the function process_new_icon in lib/gdlib.php. This function essentially does what we want except that it produces two sizes at once and saves them to the icon file area. I figure we could make a more general function, process_new_image(), which would a) produce images of whatever size requested b) save it to wherever requested process_new_icon() could become a thin wrapper around process_new_image(). Two points to consider: 1) do we need performance, such that this function can handle an array and generate more than one size, as opposed to calling it more than once and opening the original more than once? 2) should the location of these new images be something like filedir/<content_hashed_trie_path>_<size> (ie. filedir/69/d9/69d9284ead0ee950ff3ff8bb663162230bde227b_large) or is there something better? In other projects, my content_path would be a directory that includes all sizes, but that doesn't work with existing images. You can also choose to group sizes together, but since filedir is not meant to be traversed, this isn't a meaningful idea. No matter what, we'll need to be able to delete all sizes when removing an unused file.
Hide
Darcy W. Christ added a comment -

I changed direction a little. Discovered a method in lib/filestorage/file_storage.php, which could resize images just fine. Also, i think the correct place for this auto-generation is in the method get_file, which in turn looks for a parameter named size, that matches a built-in array. The array is hardwired at the moment. Next step is to find the best place to create settings for this, so it can be modified. Once it is established, we can explore ways to tie in the size param, so that other image sizes can be used.

https://github.com/1000camels/moodle/compare/master...MDL-10950

Please comment on github or here, if you have thoughts on this work.

One other thing, in my opinion, the lib/gdlib.php file could be removed, once some of the bicubic logic is added to the convert_image() method in file_storage.php. i understand the purpose of having the bicubic logic, but i think it would be best to keep this code in one place, so that all image manipulation code would use the same base code in filestorage.

Show
Darcy W. Christ added a comment - I changed direction a little. Discovered a method in lib/filestorage/file_storage.php, which could resize images just fine. Also, i think the correct place for this auto-generation is in the method get_file, which in turn looks for a parameter named size, that matches a built-in array. The array is hardwired at the moment. Next step is to find the best place to create settings for this, so it can be modified. Once it is established, we can explore ways to tie in the size param, so that other image sizes can be used. https://github.com/1000camels/moodle/compare/master...MDL-10950 Please comment on github or here, if you have thoughts on this work. One other thing, in my opinion, the lib/gdlib.php file could be removed, once some of the bicubic logic is added to the convert_image() method in file_storage.php. i understand the purpose of having the bicubic logic, but i think it would be best to keep this code in one place, so that all image manipulation code would use the same base code in filestorage.
Hide
Darcy W. Christ added a comment -

UPDATE:

I've created a filter (https://github.com/1000camels/moodle-filter_imageresize) which handles the rendering of the different sizes. It specifically looks at the width/height attributes set via tinyMCE and tries to use a smaller image format instead of the original.

Also adding settings for the sizes: thumbnail, small, medium and large, so that the dimensions can be changed. There is no support right now for adding new sizes.

Another issue is that once an image is created, if you change the dimensions, the image does not get recreated. In general, I think more work needs to be done to handle automatic cleanup and regeneration. Otherwise, it is good to go.

I solved my other issue of dealing with draft files, which seem not to use the same code from file_storage - not sure why. Anyway, in draft, the original is used. This is exclusively for production to help aleviate the performance issue of serving large images.

I'm looking for testers and comments.

Show
Darcy W. Christ added a comment - UPDATE: I've created a filter (https://github.com/1000camels/moodle-filter_imageresize) which handles the rendering of the different sizes. It specifically looks at the width/height attributes set via tinyMCE and tries to use a smaller image format instead of the original. Also adding settings for the sizes: thumbnail, small, medium and large, so that the dimensions can be changed. There is no support right now for adding new sizes. Another issue is that once an image is created, if you change the dimensions, the image does not get recreated. In general, I think more work needs to be done to handle automatic cleanup and regeneration. Otherwise, it is good to go. I solved my other issue of dealing with draft files, which seem not to use the same code from file_storage - not sure why. Anyway, in draft, the original is used. This is exclusively for production to help aleviate the performance issue of serving large images. I'm looking for testers and comments.
Hide
Darcy W. Christ added a comment -

Demo is now available: http://www.youtube.com/watch?v=J68hSVSM_3s

The only outstanding code which needs to be developed is some process that deletes existing image sizes, if the dimensions are changed for that size.

Show
Darcy W. Christ added a comment - Demo is now available: http://www.youtube.com/watch?v=J68hSVSM_3s The only outstanding code which needs to be developed is some process that deletes existing image sizes, if the dimensions are changed for that size.
Hide
Andrew Nicols added a comment - - edited

Hi Darcy,

This looks great - it's something I keep meaning to look into. I had in mind to implement it as a filter which resizes on the fly:

It should be possible to write a new filter which:

  • looks for images which:
    • are served by pluginfile.php; and
    • have a width and height.

The filter could then:

  • generate a unique hash for that file with the given dimensions;
  • check whether the cached file exists with the new hash;
    • if it does - serve it
    • if not resize to that size and cache the file

This is the method that RSS feeds currently use.

I've got part of this working as a proof of concept. A large chunk of file_pluginfile() in lib/file.php would beed to be abstracted to make this more useful, and you'd have to decide where to store the cached files (e.g. in the filemanager, or in the cachedir).

There would also potentially still be issues with file generation time, but this may be preferable over the amount of time required to serve a large file over a slow connection.

Andrew

Show
Andrew Nicols added a comment - - edited Hi Darcy, This looks great - it's something I keep meaning to look into. I had in mind to implement it as a filter which resizes on the fly: It should be possible to write a new filter which:
  • looks for images which:
    • are served by pluginfile.php; and
    • have a width and height.
The filter could then:
  • generate a unique hash for that file with the given dimensions;
  • check whether the cached file exists with the new hash;
    • if it does - serve it
    • if not resize to that size and cache the file
This is the method that RSS feeds currently use. I've got part of this working as a proof of concept. A large chunk of file_pluginfile() in lib/file.php would beed to be abstracted to make this more useful, and you'd have to decide where to store the cached files (e.g. in the filemanager, or in the cachedir). There would also potentially still be issues with file generation time, but this may be preferable over the amount of time required to serve a large file over a slow connection. Andrew
Hide
Darcy W. Christ added a comment -

It had not occurred to me to try to do it all within the filter. There is a side of me which wants to solve the larger issue. I was somewhat surprise to find some existing code in Moodle core which does file resizing and yet no consolidated approach to doing this for all image files. Although it may be harder to get this code into the core, I think it is the more mature approach to offer alternative image sizes within file_storage.

I don't know if you looked at my core modification (https://github.com/1000camels/moodle/compare/master...MDL-10950), but it is really rather small. I've been trying to find out which core developers have strong opinions about the file API to determine whether my approach of prepending the image size to the file name makes the most sense.

The nice thing about this approach, rather than coding it within in the filter is that this makes the code available to any code in the system. As long as you form the URL correctly (ie. add size/<image size name>/ to the path in pluginfile.php), the appropriate image will get built.

Show
Darcy W. Christ added a comment - It had not occurred to me to try to do it all within the filter. There is a side of me which wants to solve the larger issue. I was somewhat surprise to find some existing code in Moodle core which does file resizing and yet no consolidated approach to doing this for all image files. Although it may be harder to get this code into the core, I think it is the more mature approach to offer alternative image sizes within file_storage. I don't know if you looked at my core modification (https://github.com/1000camels/moodle/compare/master...MDL-10950), but it is really rather small. I've been trying to find out which core developers have strong opinions about the file API to determine whether my approach of prepending the image size to the file name makes the most sense. The nice thing about this approach, rather than coding it within in the filter is that this makes the code available to any code in the system. As long as you form the URL correctly (ie. add size/<image size name>/ to the path in pluginfile.php), the appropriate image will get built.
Hide
Andrew Nicols added a comment -

Hi Darcy,

Sorry, I meant that the filter approach would complement the handling you're doing in pluginfile.php. Reading back this wasn't very clear, but that's probably what you get when you're trying to write something as you're running out the door - apologies for that.

I've had a quick look over your code and I agree that it would be good to have this in core. Passing a height and width as additional arguments for pluginfile.php to handle resizing seems extremely advantageous. In the code you've submitted though, I don't see the TinyMCE ImageResize plugin you mention. Additionally, it would be great if it could take either height, width, or both height and width to perform the resize and scale as appropriate. Rather than having a TinyMCE plugin to do this, it could be performed by a filter and using a regular expression relatively easily.

The other part of my suggestion was that rather than generating a variety of other sizes for the image and storing them in the filemanager, and then choosing the nearest size to the one selected, why not generate the exact size requested and store it in the cache. This alleviates the issues with deleting old images as the cache can be periodically cleared, and means that you don't require an admin setting with the various sizes.

Andrew

Show
Andrew Nicols added a comment - Hi Darcy, Sorry, I meant that the filter approach would complement the handling you're doing in pluginfile.php. Reading back this wasn't very clear, but that's probably what you get when you're trying to write something as you're running out the door - apologies for that. I've had a quick look over your code and I agree that it would be good to have this in core. Passing a height and width as additional arguments for pluginfile.php to handle resizing seems extremely advantageous. In the code you've submitted though, I don't see the TinyMCE ImageResize plugin you mention. Additionally, it would be great if it could take either height, width, or both height and width to perform the resize and scale as appropriate. Rather than having a TinyMCE plugin to do this, it could be performed by a filter and using a regular expression relatively easily. The other part of my suggestion was that rather than generating a variety of other sizes for the image and storing them in the filemanager, and then choosing the nearest size to the one selected, why not generate the exact size requested and store it in the cache. This alleviates the issues with deleting old images as the cache can be periodically cleared, and means that you don't require an admin setting with the various sizes. Andrew
Hide
Darcy W. Christ added a comment -

on the 10th, I mentioned and included the Moodle filter I built, as I bailed on the tinyMCE approach, since then I would have to change code to affect the draftfile.php as well.

"I've created a filter (https://github.com/1000camels/moodle-filter_imageresize) which handles the rendering of the different sizes. It specifically looks at the width/height attributes set via tinyMCE and tries to use a smaller image format instead of the original."

Show
Darcy W. Christ added a comment - on the 10th, I mentioned and included the Moodle filter I built, as I bailed on the tinyMCE approach, since then I would have to change code to affect the draftfile.php as well. "I've created a filter (https://github.com/1000camels/moodle-filter_imageresize) which handles the rendering of the different sizes. It specifically looks at the width/height attributes set via tinyMCE and tries to use a smaller image format instead of the original."
Hide
Darcy W. Christ added a comment -

I think my next stage of development is to make the types of auto-generated images a configurable list. Instead of the default, Thumbnail, Small, Medium and Large, in which you can just set the size, I think it could also be possible configure as many sizes as you want. Again, the filter just tries to match the best size.

Also, keep in mind, this core change ultimately supports an important way in which any module can access different sized images. It would be very useful.

I'm also considering changing the way I construct the filename. At present, I add the size label to the front of the filename (ie. IMG_0234.jpg -> Large-IMG_0234.jpg). I think maybe the size should go to the end (ie. IMG_0234.jpg-large). This just makes it easier to compare versions of the same file and looking at the db - purely for development purposes.

Show
Darcy W. Christ added a comment - I think my next stage of development is to make the types of auto-generated images a configurable list. Instead of the default, Thumbnail, Small, Medium and Large, in which you can just set the size, I think it could also be possible configure as many sizes as you want. Again, the filter just tries to match the best size. Also, keep in mind, this core change ultimately supports an important way in which any module can access different sized images. It would be very useful. I'm also considering changing the way I construct the filename. At present, I add the size label to the front of the filename (ie. IMG_0234.jpg -> Large-IMG_0234.jpg). I think maybe the size should go to the end (ie. IMG_0234.jpg-large). This just makes it easier to compare versions of the same file and looking at the db - purely for development purposes.
Hide
Darcy W. Christ added a comment -

Andrew, I'm still trying to breathe some life into this issue - not sure what I am doing wrong. You are the only one who has commented...

I didn't properly answer you points above. It is dangerous to open your system for arbitrary image sizes (ie. width and height specified as parameters). This creates a potential DDOS situation. A famous php image library called phpThumbs had this vulnerability several years ago. The solution was to set fixed sizes. This is why I've implemented it to use a set of fixed sizes. There is nothing preventing us from creating several sizes. Also if you look to how responsive design is being accompished these days, you'll see that generally people are settling on 5 potential sizes (eg. http://storiesbyvignette.com/blog/embracing_the_full_canvas_with_responsive_web_design) for layout. Any scaling that is done to images in between those sizes is accomplished with html or css scaling - both of which could be accomplished with the filter.

Also, are you under the impression that all sizes are created at once? This is not true. Only image sizes requested are created!

Finally, as I mentioned in the post above, I'd like to extend this core code change to allow multiple sizes to be defined. Not sure how to create the Admin setting UI for that, but I'll figure it out. My only remaining issue is image regeneration. If the sizes change, we need a mechanism for removing the old sizes.

Show
Darcy W. Christ added a comment - Andrew, I'm still trying to breathe some life into this issue - not sure what I am doing wrong. You are the only one who has commented... I didn't properly answer you points above. It is dangerous to open your system for arbitrary image sizes (ie. width and height specified as parameters). This creates a potential DDOS situation. A famous php image library called phpThumbs had this vulnerability several years ago. The solution was to set fixed sizes. This is why I've implemented it to use a set of fixed sizes. There is nothing preventing us from creating several sizes. Also if you look to how responsive design is being accompished these days, you'll see that generally people are settling on 5 potential sizes (eg. http://storiesbyvignette.com/blog/embracing_the_full_canvas_with_responsive_web_design) for layout. Any scaling that is done to images in between those sizes is accomplished with html or css scaling - both of which could be accomplished with the filter. Also, are you under the impression that all sizes are created at once? This is not true. Only image sizes requested are created! Finally, as I mentioned in the post above, I'd like to extend this core code change to allow multiple sizes to be defined. Not sure how to create the Admin setting UI for that, but I'll figure it out. My only remaining issue is image regeneration. If the sizes change, we need a mechanism for removing the old sizes.
Hide
Martin Dougiamas added a comment -

I'm just breezing past on the way to somewhere but wanted to point out that we are looking at implementing thumbnail support for images (in filepicker, filemanager etc) and that it's probably related. Adding marina as a watcher just in case.

Show
Martin Dougiamas added a comment - I'm just breezing past on the way to somewhere but wanted to point out that we are looking at implementing thumbnail support for images (in filepicker, filemanager etc) and that it's probably related. Adding marina as a watcher just in case.
Hide
Darcy W. Christ added a comment -

Thanks Martin. I would love to help out with this. I think if you look at my proposal, you'll see that its more about building in a system to create whatever sizes are required, which means it wouldn't be limited to the filepicker. Also, my technique allows us to use the existing sizing mechanisms (ie. the one in tinyMCE), but still get actual file resizing. Incorporating a feature like this, but not adding additional layers of UI is important.

Anyway, please contact me - I'm happy to help however I can. If you follow my issue here and my comments in the forums, you'll see I've run through a lot of the issues.

Show
Darcy W. Christ added a comment - Thanks Martin. I would love to help out with this. I think if you look at my proposal, you'll see that its more about building in a system to create whatever sizes are required, which means it wouldn't be limited to the filepicker. Also, my technique allows us to use the existing sizing mechanisms (ie. the one in tinyMCE), but still get actual file resizing. Incorporating a feature like this, but not adding additional layers of UI is important. Anyway, please contact me - I'm happy to help however I can. If you follow my issue here and my comments in the forums, you'll see I've run through a lot of the issues.
Hide
Darcy W. Christ added a comment -

Marina, is there any update on what Martin mentioned above? The ability to have smaller sized images is so important to user experience. Moodle will suffer if it cannot handle this kind of simple feature.

If I could connect with some of the core developers or get some feedback, I think they would find how simple this proposed fix is. This code is small and only needs a little more work, but without the input of developers who know the core and the plans for the future, I cannot push this code any further.

I appreciate the help.

Show
Darcy W. Christ added a comment - Marina, is there any update on what Martin mentioned above? The ability to have smaller sized images is so important to user experience. Moodle will suffer if it cannot handle this kind of simple feature. If I could connect with some of the core developers or get some feedback, I think they would find how simple this proposed fix is. This code is small and only needs a little more work, but without the input of developers who know the core and the plans for the future, I cannot push this code any further. I appreciate the help.
Hide
Marina Glancy added a comment -

Hi Darcy,

David Mudrak was working on MDL-32471 to support resizing of images when calling to pluginfile.php
It will be integrated in new filepicker/filemanager in 2.3 so users will see image previews (thumbnails) instead of filetype icon.

Besides, in new repositories API for 2.3 it became possible for repositories to provide their own interface. This way it must become possible to create new repositories such as image editors.

Marina

Show
Marina Glancy added a comment - Hi Darcy, David Mudrak was working on MDL-32471 to support resizing of images when calling to pluginfile.php It will be integrated in new filepicker/filemanager in 2.3 so users will see image previews (thumbnails) instead of filetype icon. Besides, in new repositories API for 2.3 it became possible for repositories to provide their own interface. This way it must become possible to create new repositories such as image editors. Marina

Dates

  • Created:
    Updated: