Detect and Handle MP4 and OGG audio files properly

[toddhalfpenny]

The audio media widget does not render the audio player when displaying the widget on the front-end when an mp4 file is referenced; it is rendered (and plays) as expected in the admin screens.

This behaviour is the same whether using the wp-core-media-widget plugin or built-in widgets in 4.8.

Attaching a gif (hope these are OK).

[toddhalfpenny]

Animated gif of viewing the widget in the frontend.

[toddhalfpenny]

Adding mp4 to wp_audio_extensions

[westonruter]

@toddhalfpenny thanks for the report. Are you uploading the MP4 audio to the media library and then adding it from the media library as an attachment? Or are you adding it by URL?

Unfortunately just adding the mp4 extension to wp_get_audio_extensions() is not enough. Note that if you are in the post editor and you upload an MP4 audio file, and then try adding that media to a post it will incorrectly think that it is a video. The media logic I think is incorrectly only looking at the file extension to determine the media type.

Also see: ​https://en.wikipedia.org/wiki/MPEG-4_Part_14#.MP4_versus_.M4A

[toddhalfpenny]

@westonruter I initially spotted this when uploading from inside the upload dialog from the audio widget itself, rather than from an already uploaded file in the media library, but yes this is the scenario of media upload rather then linking by URL.

And yes, you're right, that the default handling of audio mp4s is also probably wrong, in the case of directly inserting into posts. Would that scenario not be separate though to this ticket and patch, such that this ticket applies to the use of the audio widget?

[westonruter]

@toddhalfpenny I don't think the issue is specific to the Audio widget actually. It is a general issue in Media that impacts MP4 audio everywhere.

[toddhalfpenny]

Yes, there is the issue that the mp4 is not handled throughout, but this patch does mean that the Audio Widget itself does now function correctly... could this not be handled as a distinct ticket as it does not itself impact on the rest of the mp4 handling.

[timmydcrawford]

Thanks for the report and diff @toddhalfpenny. I think this is the correct way to add support for mp4 files in the audio player, but also think the real question here is why the format has never been on the list of supported audio extensions in the first place.

Looking at Mozilla's documentation around the HTML5 audio player, they have an interesting section about .mp4 encoded files in the context of the browser's audio players: ​https://developer.mozilla.org/en-US/docs/Web/HTML/Supported_media_formats#MP4_H.264_(AAC_or_MP3). TIL there are two different codecs for .mp4, both with differing levels of browser support.

As 40921.diff​ shows though, the mediaelement player does support playback of these files, though not certain if non-supported browsers mentioned in the Mozilla doc if there would potentially be issues there.

My .02 is that if the HTML5 audio player supports a file format, it makes sense to include it in wp_audio_extensions - perhaps someone from the media squad could chime in on the subject that might know some history behind the current extension list, and mp4 not being included thus far.

[obenland]

Replying to timmydcrawford:

perhaps someone from the media squad could chime in on the subject that might know some history behind the current extension list, and mp4 not being included thus far.

@joemcgill @wonderboymusic Is there anything that comes to mind for you?

[toddhalfpenny]

Thanks for the input, all.
Do you have any updated views on this, @westonruter ?

[toddhalfpenny]

Audio mp4 file - this is the one I was using when I spotted the bug.

[blobfolio]

If file extensions are ultimately determining widgetability for the frontend, we should add a few more while we're at it:

• MP4: mp4a and m4b
• MP3: mpga
• OGG: oga and opus

There are also Flash versions of MP4 that use f4a and f4b, but that's probably not something we should encourage. Haha.

[blobfolio]

Unfortunately this approach isn't going to work. WordPress registers embed handlers based on a regular expression; there's no content-based parsing to determine what streams a file might include.

Adding mp4 to the list of audio extensions would result in such URLs being bound to both audio and video embed handlers, and since the audio one gets executed first, it would cause every MP4 file to render using the audio widget, even if a file has video streams.

So basically the opposite problem. :)

[kirasong]

Agreed @blobfolio @westonruter -- WordPress assumes an extension can be either audio or video, but not both.
This means strange things are going to happen if an extension is allowed as both.

One example that seems to cause the symptom @blobfolio noted:
​https://developer.wordpress.org/reference/functions/wp_attachment_is/
Specifically, https://core.trac.wordpress.org/browser/tags/4.8/src/wp-includes/post.php#L5215 defaults to audio over video when checking what an attachment is, solely based on what the extension is.

I definitely think we should fix it, but will need to also handle it in terms of detecting media types more accurately on upload, rather than only adding types that might be either to the audio or video extension lists.

In the meantime, it's not ideal, but folks should be able to work around this by renaming .mp4 audio files to .m4a before upload. I tested, and this works for the example file above, Audio 2017-05-14 at 12.58.14.mp4.

[blobfolio]

@mikeschroder I agree the sanest approach is probably to detect audio/video when a file is first uploaded. A bit of overhead once definitely beats overhead every time a post's content is parsed for embeddables. :)

I should have time to get an alternative patch together in the next couple days for testing.

If/when the patch lands, I'll start a new ticket to cover other ambiguous extensions like ogg and asf.

[blobfolio]

Rename MP4 and OGG at upload as needed.

[blobfolio]

The new patch 40921.2.diff includes two functions that late-hook into the wp_check_filetype_and_ext filter.

wp_check_mp4_filetype_and_ext() analyses ID3 data for files with the generic .mp4 extension, renaming audio-only content to the more appropriate .m4a.

wp_check_ogg_filetype_and_ext() analyses ID3 data for files with the generic .ogg extension, renaming video-only content to the more appropriate .ogv.

How does everyone feel about this approach? It feels right to me.

• Handling these checks as part of the media upload process ensures correct handling thereafter (widget/icon/embed associations will always be accurate).
• Using separate filters for each type keeps the per-function line counts within reason, while also providing an easy template to follow for additional formats (looking at you, .asf!).

@mikeschroder, @westonruter any thoughts? If you think this is the way forward, I'll add in some unit tests.

[westonruter]

Wouldn't checking the file like this be a problem for performance?

[blobfolio]

Replying to westonruter:

Wouldn't checking the file like this be a problem for performance?

This patch does result in an extra call to the getid3.php library for files named *.mp4 or *.ogg, but the overhead from that library is not a very significant percentage of the overall request. Multimedia files might be on the larger side, but their metadata is small, and that's all that actually ends up getting read into memory (mostly).

I ran some quick tests on a fresh twentyseventeen site, and it seems that uploading a regular image file takes about 1.5x longer to process.

Not that there isn't room for improvement. :)

[blobfolio]

Also, just in case it wasn't clear, this patch only fixes the embed handling for local media (files users have uploaded), and only going forward; it won't retroactively fix any existing files that aren't named correctly.

One kind of overhead we definitely want to avoid is any sort of detailed parsing inside embed or shortcode handlers. Since those are run in loops, and run every time post content is parsed, inefficiencies there could scale to monstrous levels. Haha.

[toddhalfpenny]

This "upfront cost" seems like a much better way forward, thanks @blobfolio!

My 2p is that although this may have a perf hit, it makes it more robust, which I'd view as more important.

[blobfolio]

Replying to toddhalfpenny:

This "upfront cost" seems like a much better way forward, thanks @blobfolio!
My 2p is that although this may have a perf hit, it makes it more robust, which I'd view as more important.

Thanks @toddhalfpenny, I think so too.

Doing this out-of-the-gate not only fixes the widget issue you discovered, but also ensures that the correct metadata parsing functions get called, fixes a bunch of issues with the Media Library (filtering by type, the thumbnail icon, etc), and gives the server a better chance of correctly reporting the content type, making it easier for browsers to render direct streams correctly (particularly in cases where sniffing is disabled).

My personal crusade is to eventually get WP to do this for all files (and in a central way), but that battle will take a lot longer to wage. Haha.

[blobfolio]

Refresh against current src

[kirasong]

Replying to westonruter:

Wouldn't checking the file like this be a problem for performance?

As far as I'm aware, getID3 is pretty smart about the way it reads from files, and does not load a whole file into memory.

I suspect that doing this detection won't be a significant increase in time/resources spent, but profiling/testing there is always good.

[soulseekah]

Some simple benchmarks on the test mp4 file by performing 1000 calls to wp_check_filetype_and_ext:

Average without patch: 0.006 seconds / call
Average with patch: 0.015 seconds / call

So about 230% increase on SSD, PHP 7.2. In the context of a 100ms vanilla core load time - 6% per call, vs 15% per call. Quite heavy actually, but only if we're checking mp4/ogg files. Calls to wp_check_filetype_and_ext do not impact performance for other filetypes. Performance is mostly disk-latency bound.

The core never calls it in a loop, only once during upload. And anyone who does any sort of looping over mp4/ogg files and detecting en masse is asking for trouble anyway.

Memory usage is minimal, the library, as @mikeschroder mentioned, does file stream seeking never loading more into memory more than it needs. Filesize does not impact runtimes or memory consumption.

I think this is a great feature. While the performance costs are non-negligible, they are pretty bearable, impacting only upload handling times of mp4/ogg files (which is an even smaller drop in the total load time since the upload would take tens of seconds for the smallest files).

[soulseekah]

ogg, mp4 deep inspection tests

[soulseekah]

100ms sinewave ogg

[soulseekah]

1x1 ogg conversion of small-video.mp4

[soulseekah]

100ms sine wave mp4 (libvorbis codec)

[soulseekah]

The media files for 40921.tests.diff test have to go into tests/phpunit/data/uploads

small-audio.ogg, small-video.ogg, small-audio.mp4

[blobfolio]

Merge unit tests into feature patch.

[aaroncampbell]

Looked through this as part of a media ticket triage and just wanted to drop a note saying that I like the approach here but think it would be best to push this back to at least 5.2 so it can get in earlier in the cycle.

[kirasong]

Punting because we're late in the cycle, but self-assigning for 5.2.

I like the approach and think this will be helpful for users.

[desrosj]

I'm going to move this to the 5.3 milestone. 5.2 beta is in less than a week and the sentiment above is for this to land early in the cycle.

[kirasong]

I'll take a look for 5.3 this week.

[digitalchild]

Passed grunt tests. Tested on a host site. Mp4 is embedding correctly however ogg is not a default allowed file type so I wasn't able to test that embed with the new docker environment.

[kirasong]

@digitalchild and I were looking at this in #core-media (thanks!).

The tests aren't currently passing for either of us.

I'll loop back to this later, but would appreciate a look at it by whoever has the chance!

[antpb]

Hi @mikeschroder ! :) This ticket came up in triage and I was curious if this is still in scope of 5.3. If not, I think this could go future release with the early tag.

[kirasong]

Thanks @antpb! It's within scope, but needs someone to dig into why the tests aren't passing. I'd prefer this land before Beta 3 if it's going to.

I don't know if I'll have time, so I'm going to remove myself as owner.
I'll move it to Future Release if it doesn't see a commit before Beta 3.

[kirasong]

As noted a few days ago, since we're at right before Beta 3, going to go ahead and move this to Future Release.

I'd still love to see this fix land, so if anyone has a chance to dig into it, happy to help it move forward!