Make WordPress Core

Opened 7 years ago

Last modified 4 years ago

#42560 reopened defect (bug)

mp4 files do not play in Safari

Reported by: cg923's profile cg923 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.0
Component: Media Keywords: has-patch
Focuses: multisite Cc:

Description (last modified by xkon)

When a user uploads an mp4 file, it will not play in Safari.

We have determined that this is not an issue with our Apache server (seems to happen to some folks that they have disabled range request headers) as the same file uploaded elsewhere on our server but externally from the wordpress site loads fine.

It seems that perhaps the video is attempting to load in byte ranges but that WordPress is returning a 200 response instead of a 206, but that is just a hunch.

Attachments (1)

ms-files.diff (1.9 KB) - added by cg923 7 years ago.
Diff file for ms-files.php

Download all attachments as: .zip

Change History (21)

#1 @blobfolio
7 years ago

Thanks for the report, @cg923!

How are you embedding video content? The reason I ask is that WordPress does not normally serve static content; that would be entirely on Apache/Safari.

#2 follow-up: @cg923
7 years ago

It's an HTML 5 video element with the source being an mp4 file in the media directory.

In other words, when creating a post I am clicking "add media," uploading a file from my computer, and inserting it into the post.

External youtube videos are working fine.

#3 in reply to: ↑ 2 ; follow-up: @blobfolio
7 years ago

Replying to cg923:

It's an HTML 5 video element with the source being an mp4 file in the media directory.
External youtube videos are working fine.

Youtube is a little different; when you embed a Youtube video, the content (and all relevant headers) come from Youtube's servers.

A way to test how your server handles video — without WordPress — would be to make a barebones HTML file, like test.html, that looks like:

<html>
<head><title>Video Test</title></head>
<body>
    <!-- Copy the <video> code from your blog post here. -->
    <video .... >
</body>
</html>

If you upload that file to the server and visit it in Safari (e.g. http://yourdomain.com/test.html), WordPress should not be playing any role whatsoever.

#4 in reply to: ↑ 3 ; follow-up: @cg923
7 years ago

Right, the youtube piece was besides the point, sorry.

When we use the built in wordpress file uploader and upload an mp4 file it will not play in Safari. The resource is loaded, it seems, as the site will return 200 for the video file, but it won't play, probably because wordpress is offering the file in one chunk, whereas Safari is trying to load it in bytes.

I'm a little new to all this so I apologize if I don't explain myself clearly.

Replying to blobfolio:

Replying to cg923:

It's an HTML 5 video element with the source being an mp4 file in the media directory.
External youtube videos are working fine.

Youtube is a little different; when you embed a Youtube video, the content (and all relevant headers) come from Youtube's servers.

A way to test how your server handles video — without WordPress — would be to make a barebones HTML file, like test.html, that looks like:

<html>
<head><title>Video Test</title></head>
<body>
    <!-- Copy the <video> code from your blog post here. -->
    <video .... >
</body>
</html>

If you upload that file to the server and visit it in Safari (e.g. http://yourdomain.com/test.html), WordPress should not be playing any role whatsoever.

Last edited 7 years ago by cg923 (previous) (diff)

#5 in reply to: ↑ 4 @blobfolio
7 years ago

  • Resolution set to invalid
  • Status changed from new to closed

Replying to cg923:

I'm a little new to all this so I apologize if I don't explain myself clearly.

No worries! Everybody has to start somewhere. :)

When we use the built in wordpress file uploader and upload an mp4 file it will not play in Safari. The resource is loaded, it seems, as the site will return 200 for the video file, but it won't play, probably because wordpress is offering the file in one chunk, whereas Safari is trying to load it in bytes.

When a web browser asks a server for an asset (an image, video, web page, etc.), the first thing Apache does is check to see whether that asset exists. (A file that literally exists on your server is what I meant earlier by "static content".)

If the file exists, Apache just passes it along. Job done, champagne!

If it doesn't, this is when WordPress gets involved. Apache will route the request through WordPress and PHP, and WordPress will put together some sort of response to send back to the browser. (Usually this will be some sort of HTML document.)

This is all rather technical, but the main takeaway is that — unless you have deviated from the default Apache/WordPress configuration — WordPress is not serving anything but text.

When your blog address is entered into Safari, Safari asks your server for that document, then parses the HTML it gets back for any additional URLs (images, video, etc.), then sends additional requests for those.

When you uploaded the MP4 video through WordPress, WordPress simply saved a copy of it on the server. Whenever any browser asks for that, Apache will find it and serve it directly.


There are plenty of other reasons your MP4 might not be streaming the way you expect:

  • Apache might not be configured correctly to handle that kind of request.
  • The video might not be encoded with web-streaming in mind.
  • Safari's release cycle has not yet evolved to cope with the fast pace of web development; if you are even a couple versions out-of-date (the latest release also requires the latest MacOS), MP4 support might be incomplete or buggy.

I'm going to go ahead and close this ticket as this issue is not related to WordPress, but thank you again for reporting, and I hope you discover the nature of the issue.

#6 @cg923
7 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Hi there. I actually investigated the issue further and found that by supporting byte-range requests, the issue is solved and Safari users can view mp4 files that have been uploaded through Wordpress.

I have attached a diff file that I believe addresses the issue. Please let me know if you have any questions!

Thanks again,
Corey

@cg923
7 years ago

Diff file for ms-files.php

#7 @wonderboymusic
7 years ago

@dd32 any thoughts here?

#8 follow-ups: @blobfolio
7 years ago

@cg923 Apologies, I completely forgot about the legacy file-serving feature for older network sites!

For sites still using that feature, your patch is on the right track. Range requests are a little tricky, though, so there are some other factors that need to be accounted for to prevent file-piping from failing in new ways:

#9 in reply to: ↑ 8 ; follow-up: @cg923
7 years ago

@blobfolio No worries at all!

I will definitely try to clean this up and account for these different issues. I'll probably have to read up on different range request unit types too to make sure I'm covering all the bases. Might take me a while, but I'll give it a shot!

Thank you!

Replying to blobfolio:

@cg923 Apologies, I completely forgot about the legacy file-serving feature for older network sites!

For sites still using that feature, your patch is on the right track. Range requests are a little tricky, though, so there are some other factors that need to be accounted for to prevent file-piping from failing in new ways:

#10 in reply to: ↑ 9 @blobfolio
7 years ago

Replying to cg923:

I will definitely try to clean this up and account for these different issues.

Thank you for sticking with the issue and proving me wrong! :)

Aside from the intended streaming functionality, this should also reduce resource usage quite a bit for sites with media embeds. A full readfile() doesn't pull any punches. Haha.

#11 in reply to: ↑ 8 ; follow-up: @dd32
7 years ago

  • Focuses multisite added
  • Version changed from 4.8.3 to 3.0

Replying to blobfolio:

I completely forgot about the legacy file-serving feature for older network sites!

For reference, we disabled it by default in WordPress 3.5 / #19235 (5 years ago), since then ms-files.php has had very little work done on it, quite a few tickets are still hanging around which are realistically never going to be fixed - this included IMHO.

Instead of adding support for the Range header, I'm curious if instead we can simply respond to the request with a Do not support response instead to force Safari into some other mode. Upon looking into it, that's what we currently do by returning a 200 OK response (according to https://tools.ietf.org/html/rfc7233#section-4.4) but it appears that possibly unfortunately Safari is buggy and doesn't support a fallback, maybe there's another response we can make to trigger Safari in that scenario?

Personally I'm inclined to say wontfix based on this being a new enhancement to a feature we deprecated quite some time ago, and it only being triggered by a browser failing to operate correctly. The the impact this has upon legacy networks isn't exactly small though, and moving a legacy network to not use ms-files.php may not be straight forward either (I honestly do not know if simply changing the option is enough) which is why adding support here may not be the worst idea.


An additional testing requirement: What versions of Safari trigger this issue? latest version? or just old versions? Is there a webkit bug # known for it?

#12 follow-up: @dd32
7 years ago

Another thing - How are the videos embedded? Using MediaElement.js? Using a <video> tag (Which it seems Safari may not support Range: requests for) or something else?

#13 in reply to: ↑ 11 @cg923
7 years ago

I totally understand the wontfix. It makes sense if this is a deprecated feature. We were able to fix it well enough to support our client's needs, anyway, so from our perspective everything is honky dory.

I don't actually know what version of Safari the client who reported the issue was using, but can confirm that the issue exists in Safari 11.0.1. I haven't looked into webkit, but I will take a peek now.

Thanks @dd32 !

Replying to dd32:

Replying to blobfolio:

I completely forgot about the legacy file-serving feature for older network sites!

For reference, we disabled it by default in WordPress 3.5 / #19235 (5 years ago), since then ms-files.php has had very little work done on it, quite a few tickets are still hanging around which are realistically never going to be fixed - this included IMHO.

Instead of adding support for the Range header, I'm curious if instead we can simply respond to the request with a Do not support response instead to force Safari into some other mode. Upon looking into it, that's what we currently do by returning a 200 OK response (according to https://tools.ietf.org/html/rfc7233#section-4.4) but it appears that possibly unfortunately Safari is buggy and doesn't support a fallback, maybe there's another response we can make to trigger Safari in that scenario?

Personally I'm inclined to say wontfix based on this being a new enhancement to a feature we deprecated quite some time ago, and it only being triggered by a browser failing to operate correctly. The the impact this has upon legacy networks isn't exactly small though, and moving a legacy network to not use ms-files.php may not be straight forward either (I honestly do not know if simply changing the option is enough) which is why adding support here may not be the worst idea.


An additional testing requirement: What versions of Safari trigger this issue? latest version? or just old versions? Is there a webkit bug # known for it?

#14 in reply to: ↑ 12 @cg923
7 years ago

The client, when editing a post, opens the media file picker, uploads the video and inserts it into the post. I haven't taken a look at the core code that handles this function.

Replying to dd32:

Another thing - How are the videos embedded? Using MediaElement.js? Using a <video> tag (Which it seems Safari may not support Range: requests for) or something else?

#15 @cg923
7 years ago

A quick glance at the webkit bug list yields no relevant results, but that doesn't mean there isn't one.

#16 @SergeyBiryukov
6 years ago

#45212 was marked as a duplicate.

#17 @xkon
6 years ago

  • Description modified (diff)

Just wanted to throw an update here.

The issue can be easily seen if you try to load the file via the 'normal' paths for example in the case I bumped into today was:

somemulti.site/subsite/files/2019/01/video.mp4 - this would return an Failed to load resource: Plug-in handled load Selected Element error.

But getting the direct file from somemulti.site/wp-content/blogs.dir/2/files/2019/01/video.mp4 was ok.

After applying the patch everything it seems to work fine using latest Safari.

#18 @dd32
4 years ago

Just noting that I've deployed a variant of ms-files.diff as a sunrise.php filter on WordPress.org, which I'm offering below for others:

// Add support for Range requests via ms-files.php
// See https://meta.trac.wordpress.org/ticket/5388
// See https://core.trac.wordpress.org/ticket/42560
if (
	defined( 'SHORTINIT' ) && SHORTINIT &&
	( ! defined( 'WPMU_ACCEL_REDIRECT' ) || ! WPMU_ACCEL_REDIRECT ) &&
	( ! defined( 'WPMU_SENDFILE' ) || ! WPMU_SENDFILE ) &&
	'ms-files.php' === basename( $_SERVER['SCRIPT_FILENAME'] )
) {
	// Note support for byte-range requests.
	header( 'Accept-Ranges: bytes' );

	// If this is a range request, once Multisite is loaded, override ms-files.php.
	isset( $_SERVER['HTTP_RANGE'] ) && add_action( 'ms_loaded', function() {
		$file = rtrim( BLOGUPLOADDIR, '/' ) . '/' . str_replace( '..', '', $_GET['file'] );
		$size = is_file( $file ) ? filesize( $file ) : 0;

		// Bail if it doesn't exist, or is empty.
		if ( ! $size ) {
			return;
		}

		if ( ! preg_match('/^bytes=(?P<start>\d+)-(?P<end>\d*)$/i', $_SERVER['HTTP_RANGE'], $m ) ) {
			return;
		}
		$start = (int) $m['start'];
		$end   = (int) $m['end'] ? $m['end'] + 1 : $size;

		// Validate the file is small/big enough.
		if ( $start > $size || $end > $size || $end < $start || $start === $end ) {
			status_header( 416 );
			header( 'Content-Range: bytes */' . $size );
			die( '416 - Request Range Not Satisfiable.' );
		}

		status_header( 206 );
		header( sprintf( 'Content-Range: bytes %d-%d/%d', $start, $end - 1, $size ) );
		header( 'Content-Length: ' . ( $end - $start ) );

		$mime = wp_check_filetype( $file );
		if ( $mime['type'] ) {
			header( 'Content-Type: ' . $mime['type'] );
		} else {
			header( 'Content-Type: image/' . substr( $file, strrpos( $file, '.' ) + 1 ) );
		}

		$last_modified = gmdate( 'D, d M Y H:i:s', filemtime( $file ) );
		$etag          = '"' . md5( $last_modified ) . '"';
		header( "Last-Modified: $last_modified GMT" );
		header( 'ETag: ' . $etag );
		header( 'Expires: ' . gmdate( 'D, d M Y H:i:s', time() + 100000000 ) . ' GMT' );

		// Support for conditional GET - use stripslashes() to avoid formatting.php dependency.
		$client_etag = isset( $_SERVER['HTTP_IF_NONE_MATCH'] ) ? stripslashes( $_SERVER['HTTP_IF_NONE_MATCH'] ) : false;

		if ( ! isset( $_SERVER['HTTP_IF_MODIFIED_SINCE'] ) ) {
			$_SERVER['HTTP_IF_MODIFIED_SINCE'] = false;
		}

		$client_last_modified = trim( $_SERVER['HTTP_IF_MODIFIED_SINCE'] );
		// If string is empty, return 0. If not, attempt to parse into a timestamp.
		$client_modified_timestamp = $client_last_modified ? strtotime( $client_last_modified ) : 0;

		// Make a timestamp for our most recent modification...
		$modified_timestamp = strtotime( $last_modified );

		if ( ( $client_last_modified && $client_etag )
			? ( ( $client_modified_timestamp >= $modified_timestamp ) && ( $client_etag == $etag ) )
			: ( ( $client_modified_timestamp >= $modified_timestamp ) || ( $client_etag == $etag ) )
			) {
			status_header( 304 );
			exit;
		}

		// Micro-optimization to shortcut.
		if ( $start === 0 && $end === $size ) {
			readfile( $file );
			die();
		}

		// Open the file and stream it.
		$handle = fopen( $file, 'rb' );
		fseek( $handle, $start );

		$chunk_size = 8 * KB_IN_BYTES;
		$size_left  = $end - $start;

		while (
			$size_left &&
			! feof( $handle ) &&
			$data = fread( $handle, min( $chunk_size, $size_left ) )
		) {
			$size_left -= strlen( $data );
			echo $data;
		}

		fclose( $handle );

		die();
	}, 100 );
}

Despite my earlier reservations for not having to handle this in WordPress core, fixing this is a win for any older sites still using ms_files_rewriting.

Last edited 4 years ago by dd32 (previous) (diff)

This ticket was mentioned in PR #1064 on WordPress/wordpress-develop by dd32.


4 years ago
#19

  • Keywords has-patch added

AlbertMarashi commented on PR #1064:


4 years ago
#20

This is causing issues

Note: See TracTickets for help on using tickets.