I've spent a few hours this afternoon tracing through the code to Media (1.x) to discover why the thumbnail display takes, for my sample content, 70 times longer to display than the list. Eventually, it boiled down to the call to `image_load` in the `file_entity_file_formatter_file_image_view` function. Removing this call (and hardcoding the `#width` and `#height` values in the `$element` returned by the formatter) result in runtimes which are approximately equivalent to the list view.

I've not tried 2.x, but I have examined the code for `file_entity` (4db2744b1d3d1f5e50a2390bb48dc45645365ed6) and find the same call in the same place, so I expect the same problem applies.

I'm not sure what the correct solution is (either ditching the dimension attributes on the rendered `Only local images are allowed.` tag and letting the image style return whatever, or caching image dimensions when the file is created) but invoking `image_load` every time it's displayed seems to me to be unsupportable.

Files: 
CommentFileSizeAuthor
#35 no_image_load_in_formatter-1447790-35.patch6.74 KBChaulky
PASSED: [[SimpleTest]]: [MySQL] 11 pass(es).
[ View ]
#35 interdiff.txt485 bytesChaulky
#33 no_image_load_in_formatter-1447790-33.patch6.74 KBChaulky
PASSED: [[SimpleTest]]: [MySQL] 11 pass(es).
[ View ]
#24 no_image_load_in_formatter-1447790-24.patch6.63 KBDevin Carlson
PASSED: [[SimpleTest]]: [MySQL] 11 pass(es).
[ View ]
#19 no_image_load_in_formatter-1447790-19.patch7.25 KBdlcerva
FAILED: [[SimpleTest]]: [MySQL] 11 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#15 no_image_load_in_formatter-1447790-15.patch9.94 KBpbuyle
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]
#13 no_image_load_in_formatter-1447790-13.patch10.21 KBpbuyle
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]
#11 no_image_load_in_formatter-1447790-11.patch10.31 KBpbuyle
FAILED: [[SimpleTest]]: [MySQL] 58 pass(es), 14 fail(s), and 25 exception(s).
[ View ]
#9 no_image_load_in_formatter-1447790-9.patch4.82 KBpbuyle
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es).
[ View ]
#1 1447790-media thumbnails callgraph.png381.26 KBthsutton

Comments

thsutton’s picture

StatusFileSize
new381.26 KB

Attached: a call graph generated by xhprof showing `imagecreatefromjpeg` completely dominating the rendering of `admin/content/media/thumbnails'.

Dave Reid’s picture

Assigned:Unassigned» Dave Reid
Priority:Normal» Major

Grr this is due to the fact that we don't have access to the image height and width attributes from file_load() due to a stupid core decision - #1448124: Image dimensions should be available from file_load() for images, and not stored in field data. You get PHP notices if you do not provide height and width for theme_image_style(). What we will have to do is add a new table {image_dimensions} via file_entity with columns fid, height, and width that can store this data and then load these values in via hook_file_load().

prinds’s picture

Is it really necessary to create a new table? I suggest the following..

1. try to get the data saved with the image field using file_usage in a db query
2. if that fails, fall back to using image_get_info
3. if that also fails, null values will be passed as width and height, which should avoid the php notices

I have tried to modify the file_entity_file_formatter_file_image_view function. (from 7.x-1.0-rc3, where I have the same problem) Here it is:

function file_entity_file_formatter_file_image_view($file, $display, $langcode) {
  $scheme = file_uri_scheme($file->uri);
  $local_wrappers = file_get_stream_wrappers(STREAM_WRAPPERS_LOCAL);
 
  if (isset($local_wrappers[$scheme]) && strpos($file->filemime, 'image/') === 0 ){

  $width = NULL;$height = NULL;
 
  //try to read the field name of this file in the db, using the file_usage table
  $field_name_result = db_query(
  "SELECT field_config_instance.field_name ".
  "FROM file_usage ".
  "JOIN node ON node.nid = file_usage.id ".
  "JOIN field_config_instance ON node.type = field_config_instance.bundle ".
  "JOIN field_config ON field_config_instance.field_id = field_config.id ".
  "WHERE file_usage.module = 'file' && file_usage.fid = :fid && field_config.type = 'image' LIMIT 1",
  array(':fid'=>$file->fid))->fetchCol(0);
 
  if(!empty($field_name_result)){
  //with the field name, we can read the field data for the file
  $field_name = $field_name_result[0];
  $field_data = db_select('field_data_'.$field_name,'fd')
                ->fields('fd',array($field_name.'_width', $field_name.'_height'))
                ->condition($field_name.'_fid',$file->fid)
                ->execute()
                ->fetchAssoc();
               
  if(!empty($field_data))
  {
      $width = $field_data[$field_name.'_width'];
            $height = $field_data[$field_name.'_height'];
  }
  }
 
  //if the field data couldn't be found in the db fall back to the image_get_info function
  if(empty($width) || empty($height))
  {
  $info = image_get_info($file->uri);
  $width = $info['width'];
  $height = $info['height'];
  }
 
    if (!empty($display['settings']['image_style'])) {
      $element = array(
        '#theme' => 'image_style',
        '#style_name' => $display['settings']['image_style'],
        '#path' => $file->uri,
        '#width' => $width,
        '#height' => $height,
      );
    }
    else {
      $element = array(
        '#theme' => 'image',
        '#path' => $file->uri,
        '#width' => $width,
        '#height' => $height,
      );
    }
    return $element;
  }
}

But as you say, I agree, it would be more relevant to save the width and height with the file data in stead of the field data.. Or it would be a good idea to add which field uses the file in the file_usage table, because, that would make it easier to get the field data.

jbrown’s picture

image_theme() defines defaults for width and height, so you only get warnings when width and height are not provided if you haven't cleared the cache or you are calling theme_image_style() directly (illegal).

Dave Reid’s picture

@prinds Your code would add two additional multi-table queries for every single image, so yes, an additional {image_dimensions} table is wise in this case.

prinds’s picture

@Dave Reid,
well, two multi-table queries is a lot faster than image_load in my setup.. :)

But I see what you mean. In the long run, an extra table is better performance (and probably also safer than complex queries)..

Dave Reid’s picture

Issue tags:+sprint, +Media Initiative
pbuyle’s picture

Assigned:Dave Reid» pbuyle

I would be happy to work on this during the Denver2012 sprint.

pbuyle’s picture

StatusFileSize
new4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 17 pass(es).
[ View ]

Here is a patch implementing the solution suggested in #2. Image dimensions are loaded from the {image_dimensions} table in file_entity_file_load as $file->image_dimensions. In file_entity_file_formatter_file_image_view, if $file->image_dimensions is not set, the dimensions are loaded using image_get_info and stored in {image_dimensions}.

pbuyle’s picture

Status:Active» Needs review
pbuyle’s picture

StatusFileSize
new10.31 KB
FAILED: [[SimpleTest]]: [MySQL] 58 pass(es), 14 fail(s), and 25 exception(s).
[ View ]

Here is an updated patch with hook_file_insert, hook_file_update abd hook_file_delete support. There is no more code to load image dimensions file_entity_file_formatter_file_image_view since it is always done on file_save and file_load. I did write tests for the patch, but I'm issue running the test cases locally.

Status:Needs review» Needs work

The last submitted patch, no_image_load_in_formatter-1447790-11.patch, failed testing.

pbuyle’s picture

Status:Needs work» Needs review
StatusFileSize
new10.21 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]

Here is an updated version of the patch in #11 with fixed tests.

Dave Reid’s picture

+++ b/file_entity.file.incundefined
@@ -109,3 +118,80 @@ function file_entity_file_operation_info() {
+    // Delete image dimensions from the {image_dimensions} table if it's already there.
+    db_query('DELETE FROM {image_dimensions} WHERE fid = :fid', array(':fid' => $file->fid));
+    // Insert the image dimensions into the {image_dimensions} table.
+    db_query('INSERT INTO {image_dimensions} (fid, width, height) VALUES (:fid, :width, :height);', array(
+      ':fid' => $file->fid,
+      ':width' => $file->image_dimensions['width'],
+      ':height' => $file->image_dimensions['height'],

We should use db_merge() here. Otherwise this is a great patch and you've put really good work into it, especially with the documentation. Once we've got that

+++ b/file_entity.installundefined
@@ -531,22 +531,22 @@ function file_entity_file_formatter_file_image_view($file, $display, $langcode)

   $scheme = file_uri_scheme($file->uri);
   $local_wrappers = file_get_stream_wrappers(STREAM_WRAPPERS_LOCAL);
-  if (isset($local_wrappers[$scheme]) && $image = image_load($file->uri)) {
+  if (isset($local_wrappers[$scheme]) && isset($file->image_dimensions)) {
     if (!empty($display['settings']['image_style'])) {

Just a note that #1142630: media_fid_illegal_value error when adding media to an entity is going to conflict with this hunk which is about to land. I can probably fix this when applying. Just a note to myself.

pbuyle’s picture

StatusFileSize
new9.94 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]

Here is an updated patch suing db_merge().

Dave Reid’s picture

Status:Needs review» Reviewed & tested by the community

Awesome. Putting on commit list.

Dave Reid’s picture

Project:File Entity (fieldable files)» Media
Version:7.x-2.x-dev» 7.x-1.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed to 7.x-2.x: http://drupalcode.org/project/file_entity.git/commit/d897533

This probably needs to be backported to Media for 7.x-1.x since it's a severe performance issue.

gmclelland’s picture

FYI.. I'm not sure, but I think this commit might be causing an issue with the @becw patch on #1426730: Automatically open the file edit modal after uploading a file. I'm not sure if her patch needs to be reworked or if I need to open a new issue for File Entity?

My comment http://drupal.org/node/1426730#comment-5817694 shows how the warning appears.

dlcerva’s picture

StatusFileSize
new7.25 KB
FAILED: [[SimpleTest]]: [MySQL] 11 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

Here is a possible patch that would backport to Media 7.x-1.x. Had little chance to fully test.

dlcerva’s picture

Status:Patch (to be ported)» Needs review

Status:Needs review» Needs work

The last submitted patch, no_image_load_in_formatter-1447790-19.patch, failed testing.

Berdir’s picture

Ouch, this is huge. Took me a while to track this down as it only happened on production. We have the files on a relatively slow NFS storage and this is absolutely killing the site, as we have tons of images displayed everywhere. I'm talking about 60s+ response times.

I have commented it out for now, this site has it's own handing for that anyway as it was built before this existed. Will have a closer look when I find the time.

mcfilms’s picture

I read through this thread and I wonder why I am having this problem. See I'm not using images. I have a folder with a few hundred MP3 files. These obviously do not have a height or width. Yet when I load a piece of media from the library, the full list does not load. And I have to scroll down the list and wait for Ajax to load it bit by bit.

It used to take over a minute to load the full list. I got it down a little. I visited admin/config/media/browser
and switched the allowed field extensions from:
jpg jpeg gif png txt doc docx xls xlsx pdf ppt pptx pps ppsx odt ods odp mp3 mov m4v mp4 mpeg avi ogg oga ogv wmv ico

to mp3

This has obvious drawbacks if I ever want to use the Media module to manage any other media besides MP3.

It now loads in about 20 seconds or so. But I would ask, why is it not possible for me to load all the songs instantly?

Devin Carlson’s picture

Assigned:pbuyle» Devin Carlson
Status:Needs work» Needs review
StatusFileSize
new6.63 KB
PASSED: [[SimpleTest]]: [MySQL] 11 pass(es).
[ View ]

Reroll of #15 for Media 7.x-1.x with the File Entity test omitted.

Tested it locally and it worked fine.

ParisLiakos’s picture

DamienMcKenna’s picture

The code looks good, though I haven't tested it yet =)

ParisLiakos’s picture

This would also require removing existing update function in file_entity..care to submit a patch there, so i commit them both in same time?

jcnventura’s picture

Status:Needs review» Reviewed & tested by the community

Just tested this. and it worked perfectly. I don't know what update function is being talked about here, but this should be in 7.x-1.x-dev ASAP. After adding a couple of large files and in a server with 128Mb PHP max mem, this prevented the media browser from working.

aaron’s picture

@rootatwc: what update function are you referring to? Is it in 7.x-1.x or 2.x? If in the first version, don't forget that file entity is included with the media module of that version.

ParisLiakos’s picture

this would be not needed
http://drupalcode.org/project/file_entity.git/blob/0a29735:/file_entity....
If i commit this one, then file_entity_update_7200 will make other updates fail, right?

Dave Reid’s picture

We should ensure any update adding the table first checks db_table_exists(), file_entity_update_7200() included.

ParisLiakos’s picture

Status:Reviewed & tested by the community» Needs work

Back to needs work then, to add this conditional.
This issue will take care of it in file_entity #1890486: Check if image_dimensions exists before creating it

Chaulky’s picture

StatusFileSize
new6.74 KB
PASSED: [[SimpleTest]]: [MySQL] 11 pass(es).
[ View ]

I added the table check to the update function. Applies cleanly to latest 7.x-1.x.

Chaulky’s picture

Status:Needs work» Needs review

setting back to needs review

Chaulky’s picture

StatusFileSize
new485 bytes
new6.74 KB
PASSED: [[SimpleTest]]: [MySQL] 11 pass(es).
[ View ]

After reading more I've noticed that in File Entity it uses !$file->filesize instead of !filesize($file->uri) for determining if the file is empty. Since filesize() doesn't prevent the warning mentioned in the code comments (http://drupal.org/node/681042) I've updated the patch to use $file->filesize.

arosboro’s picture

I haven't thoroughly tested this patch but it applied without errors, and I am not experiencing any performance issues related to the media module.

Devin Carlson’s picture

Devin Carlson’s picture

Status:Needs review» Fixed

Committed #35 to 7.x-1.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

joachim’s picture

+++ b/file_entity/file_entity.install
@@ -211,3 +243,44 @@ function file_entity_update_7002() {
+function file_entity_update_7101() {
+  if (!db_table_exists('image_dimensions')){
+    $schema['image_dimensions'] = array(
+      'description' => 'Cache images dimensions.',

How is this going to get populated for existing images?

pbuyle’s picture

@joachim: In file_entity_file_load($files), file_entity_image_dimensions($file, FALSE); is called for all loaded files. This function takes care of populating the table.