Max execution time exceeded in Pageimage.php for specific pages

[snck]

Hi there,

I am having a strange problem. I have a bunch of sites that have similar content (text, images, map markers), but I am not able to edit two specific pages in the backend. I always get the following error (or 503 service unavailable):

Maximum execution time of 30 seconds exceeded (Zeile 520 in [...]/wire/core/Pageimage.php)

Line 520 in Pageimage.php:

$imagick->readImage($filename);

All of the images used on all of the pages are SVG drawings and I have no clue why there are no problems with the majority of pages but just two of them. Maybe one of you guys has experienced a similar problem with ImageMagick and SVGs?

I have debug mode enabled, but I only get these errors and nothing more specific. Is there any smart way to find the source of the error?

In the assets folder for all of the pages there are only SVGs so I expect no conversion to be done. If no conversion is happening, why would the script run into the max execution time? 

I appreciate your thoughts on this.

Cheers,
Flo

[kongondo]

@snck

Did you get this resolved?

[snck]

On 4/24/2020 at 10:24 AM, kongondo said:

@snck

Did you get this resolved?

I further investigated the issue by reviewing all of the SVG files used on the pages that have thrown errors and comparing them to files from pages that were editable without problems. I realised that the guys who generated the files (architecture students) used different software in their process (Adobe Illustrator, Vectorworks etc.) which led to really complex files. I ran some of them through an online SVG minification tool which helped to resolve the issue in some cases and instructed the students to develop a kind of maximum complexity guideline for their files.

To make the backend usable again and offer them a chance to exchange their "bad" files, it raised the max_execution_time temporarily in the .htaccess:

php_value max_execution_time 120

 

[kongondo]

@snck,

Thanks for reporting back.

12 minutes ago, snck said:

used different software in their process (Adobe Illustrator, Vectorworks etc.) which led to really complex files.

I don't know much about SVGs, excuse my ignorance please. Was the issue the complexity of the files or the size of the files? I am guessing though that complex files would probably also be large in size. 

Thanks.

[snck]

18 minutes ago, kongondo said:

@snck,

Thanks for reporting back.

I don't know much about SVGs, excuse my ignorance please. Was the issue the complexity of the files or the size of the files? I am guessing though that complex files would probably also be large in size. 

Thanks.

Size alone should not be a problem (considering that I have used a lot of much bigger JPEG images with IMagick). In some cases the SVGs consisted of many 1000 paths and a lot of class definitions for their styling. I do not know the how efficient the rasterizing in IMagick works, but I guess it was the bootleneck in this specific case.

[adrian]

In reality, that getImageInfoSVG() method should probably try to get the dimensions from the viewbox attribute in the SVG (if actual width and height attributes are not available), before it resorts to using imagick to get them. That would remove the need to rasterize just to get the dimensions.

[Mikie]

I have personally found errors related to imagick and pageimage.php to be misleading as the failure can happen at the server / imagick module level and can be due to issues with multi-threading. Are you in a multi-threaded (ie two or more cpu cores) environment and are the timeouts happening much faster than 30 seconds?

[snck]

On 4/29/2020 at 7:52 PM, adrian said:

In reality, that getImageInfoSVG() method should probably try to get the dimensions from the viewbox attribute in the SVG (if actual width and height attributes are not available), before it resorts to using imagick to get them. That would remove the need to rasterize just to get the dimensions.

@adrian Again had problems with this and got back to your idea. Thank you so much!

This quick fix might be not the most beautiful/efficient way, but worked fine for me (and might also for someone else):

/**
  * Gets the image info/size of an SVG
  *
  * Returned width and height values may be integers OR percentage strings.
  *
  * #pw-internal
  *
  * @param string $filename Optional filename to check
  * @return array of width and height
  *
  */
protected function getImageInfoSVG($filename = '') {
  $width = 0;
  $height = 0;
  if(!$filename) $filename = $this->filename;
  $xml = @file_get_contents($filename);
  
  if($xml) {
    $a = @simplexml_load_string($xml)->attributes();
    if((int) $a->width > 0) $width = (int) $a->width;
    if((int) $a->height > 0) $height = (int) $a->height;

    // start viewbox fix
    if(!$width || !$height){
      if($a->viewBox != ""){
        // get rid of commas: replace them with spaces
        $a->viewBox = str_replace(",", " ", $a->viewBox);
        // get rid of (now possible) double spaces: replace them with a single space
        $a->viewBox = str_replace("  ", " ", $a->viewBox);
        // get single values
        $viewbox = explode(" ", $a->viewBox);
        
        if(count($viewbox) === 4){
          // we need 4 values
          $min_x = (float) $viewbox[0];
          $min_y = (float) $viewbox[1];
          $max_x = (float) $viewbox[2];
          $max_y = (float) $viewbox[3];

		  $width = $max_x - $min_x;
          $height = $max_y - $min_y;
        }
      }
    }
    // end viewbox fix
  }
  
  if((!$width || !$height) && (extension_loaded('imagick') || class_exists('\IMagick'))) {
    try {
      $imagick = new \Imagick();
      $imagick->readImage($filename);
      $width = $imagick->getImageWidth();
      $height = $imagick->getImageHeight();
    } catch(\Exception $e) {
      // fallback to 100%
    }
  }
  
  if($width < 1) $width = '100%';
  if($height < 1) $height = '100%';
  
  return array(
    'width' => $width, 
    'height' => $height
  ); 
}

 

[adrian]

Hi @snck - nice work. I really do think this should be incorporated into the core, although I am a bit confused by the calculation method you have for getting the width / height. I feel like the width and height should be take directly from [2] and [3]. Even if the min_x or min_y aren't set to 0, surely that doesn't change the width of the canvas area that the illustration is on which is what we want the width / height of.

Do you agree, or am I missing something?

We need to get Ryan's attention on this - maybe a Github issue?

 

 

[snck]

2 hours ago, adrian said:

Hi @snck - nice work. I really do think this should be incorporated into the core, although I am a bit confused by the calculation method you have for getting the width / height. I feel like the width and height should be take directly from [2] and [3]. Even if the min_x or min_y aren't set to 0, surely that doesn't change the width of the canvas area that the illustration is on which is what we want the width / height of.

Do you agree, or am I missing something?

We need to get Ryan's attention on this - maybe a Github issue?

 

 

Hey @adrian, you are right. I was thinking of the viewBox as a set of coordinates in x/y dimension that are defining the corners of the viewbox relative to the origin of the coordinate system (this is why i called them max_x and max_y), which is obviously wrong. It should look like this:

/**
  * Gets the image info/size of an SVG
  *
  * Returned width and height values may be integers OR percentage strings.
  *
  * #pw-internal
  *
  * @param string $filename Optional filename to check
  * @return array of width and height
  *
  */
protected function getImageInfoSVG($filename = '') {
  $width = 0;
  $height = 0;
  if(!$filename) $filename = $this->filename;
  $xml = @file_get_contents($filename);
  
  if($xml) {
    $a = @simplexml_load_string($xml)->attributes();
    if((int) $a->width > 0) $width = (int) $a->width;
    if((int) $a->height > 0) $height = (int) $a->height;

    // start viewbox fix
    if(!$width || !$height){
      if($a->viewBox != ""){
        // get rid of commas: replace them with spaces
        $a->viewBox = str_replace(",", " ", $a->viewBox);
        // get rid of (now possible) double spaces: replace them with a single space
        $a->viewBox = str_replace("  ", " ", $a->viewBox);
        // get single values
        $viewbox = explode(" ", $a->viewBox);
        
        if(count($viewbox) === 4){
          // we need 4 values, even though we are just using 2
		  $width = (float) $viewbox[2];
          $height = (float) $viewbox[3];
        }
      }
    }
    // end viewbox fix
  }
  
  if((!$width || !$height) && (extension_loaded('imagick') || class_exists('\IMagick'))) {
    try {
      $imagick = new \Imagick();
      $imagick->readImage($filename);
      $width = $imagick->getImageWidth();
      $height = $imagick->getImageHeight();
    } catch(\Exception $e) {
      // fallback to 100%
    }
  }
  
  if($width < 1) $width = '100%';
  if($height < 1) $height = '100%';
  
  return array(
    'width' => $width, 
    'height' => $height
  ); 
}

@ryan, maybe something like this could be a useful addition to the core? 

[bernhard]

I'm not sure if I understand everything 100% but as I was mentioned here (seems to be removed now) I read the thread and it reminds me a bit of a problem we had recently when using SVGs on one of our websites. SVGs didn't show up on the frontend and also the PW backend had problems (it showed them in the small preview but not when viewed via the lightbox). The problem was that the SVGs did not have dimensions specified, so I'm wondering if that could be related?

Could maybe PW check SVGs on upload and add with and height automatically if they are not present? Would that also fix your issue?

Please ignore if that is not related or nonsense ? 

[adrian]

Just now, bernhard said:

Please ignore if that is not related or nonsense

It's not nonsense, but SVGs don't need to have width / height attributes. It's perfectly acceptable for these to be missing, but it can result in some display issues depending on how they are output. I don't think PW should automatically add these (at least without it being optional), but on some sites I do this via a hook because I do want to ensure those attributes are set. If they're not set, I get them from the viewbox and add them - similarly to the way @snck is showing above.

 

[bernhard]

21 minutes ago, adrian said:

but on some sites I do this via a hook because I do want to ensure those attributes are set.

Want to share that? ? 

[horst]

1 hour ago, bernhard said:

Want to share that?

The hook to work with images on upload, before added to any image field, is this:
https://processwire.com/talk/topic/13759-tagging-multiple-images/#comment-123851

(But its just an example for the hook, not for changing the svg issue.)