WordPress.org
Get WordPress

Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#28907 closed defect (bug) (fixed)

media_handle_upload calls wp_read_image_metadata on video files, causing errors and timeouts.

Reported by: slangley's profile slangley Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.8
Component: Media Keywords: has-patch
Focuses: performance Cc:

Description

When uploading media files, if the mime type is anything other than audio/* then wp_read_image_metadata will be called on the file, even if the file is not an image.

This in turn calls getimagesize which will read the entire file trying to determine if it's (a) an image, and (b) it's dimensions. If we know the uploaded media has a mime type video/* (and probably others) then doing this is pointless. If it turns out your uploaded media is stored remotely, (e.g. on your CDN) then this results in failed uploads and timeouts as it takes a while to completely read large video files.

The most straightforward fix is to just check if the mime type is for a video and skip the call to wp_read_image_metadata, something like

Code highlighting: 

  } else if ( preg_match( '#^video#', $type ) === false ) {
    if ( $image_meta = @wp_read_image_metadata( $file ) ) {
      if ( trim( $image_meta['title'] ) && ! is_numeric( sanitize_title( $image_meta['title'] ) ) )
        $title = $image_meta['title'];
      if ( trim( $image_meta['caption'] ) )
        $content = $image_meta['caption'];
    }
}

Attachments (1)

media.php.patch​ (967 bytes) - added by jrf 11 years ago.
Patch as suggested by submitter

Download all attachments as: .zip

Change History (8)

#1 @SergeyBiryukov
11 years ago

  • Keywords needs-patch added

@jrf
11 years ago

Patch as suggested by submitter

#2 @jrf
11 years ago

  • Keywords has-patch added; needs-patch removed

I've added a patch implementing the suggestion by slangley.
Better would be IMHO to create a separate case for video which would look for the relevant information, though as there are lots of different places that information could be, that's beyond the scope of this bug.

#3 @aidanlane
11 years ago

Thanks slangley or for filing this bug report and to SergeyBiryukov and jrf handling this, as I was the one that had the original issue (​http://wordpress.org/support/topic/large-uploads-die-with-503-service-unavailable).

However, I wonder if just checking if 'not video' is enough? We use WordPress to host ZIP archives also (for interactives), which seems to be 'natively' supported as an 'archive' (e.g. with wp-includes/images/media/archive.png). Should ZIP archives also be be downloaded and have it's image dimensions evaluated?
Is it too dangerous just to check if the $type is some form of image?

Thanks again for all your help!

#4 @tellyworth
11 years ago

  • Version changed from trunk to 3.8

#5 @wonderboymusic
11 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 29737:

In media_handle_upload(), don't call wp_read_image_metadata() on things that aren't images (like videos). We never caught this error, because we are suppressing it by calling @wp_read_image_metadata().

Props jrf for the initial patch.
Fixes #28907.

#6 @wonderboymusic
11 years ago

  • Milestone changed from Awaiting Review to 4.1

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. ​View the logs.
10 years ago

Note: See TracTickets for help on using tickets.