Opened 8 years ago
Last modified 3 years ago
#40921 assigned defect (bug)
Detect and Handle MP4 and OGG audio files properly
Reported by: | toddhalfpenny | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 3.6 |
Component: | Media | Keywords: | has-screenshots has-unit-tests needs-patch reporter-feedback |
Focuses: | Cc: |
Description
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).
Attachments (10)
Change History (65)
#2
@
8 years ago
- Component changed from Widgets to Media
- Keywords needs-patch added; has-patch removed
- Version changed from trunk to 3.6
@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
#3
@
8 years ago
@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?
#4
@
8 years ago
- Milestone changed from Awaiting Review to Future Release
@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.
#5
@
8 years ago
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.
This ticket was mentioned in Slack in #core by toddhalfpenny. View the logs.
7 years ago
#8
follow-up:
↓ 10
@
7 years ago
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.
This ticket was mentioned in Slack in #core by timmyc. View the logs.
7 years ago
#10
in reply to:
↑ 8
@
7 years ago
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?
This ticket was mentioned in Slack in #core-customize by obenland. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by toddhalfpenny. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
7 years ago
#16
@
7 years ago
If file extensions are ultimately determining widgetability for the frontend, we should add a few more while we're at it:
- MP4:
mp4a
andm4b
- MP3:
mpga
- OGG:
oga
andopus
There are also Flash versions of MP4 that use f4a
and f4b
, but that's probably not something we should encourage. Haha.
This ticket was mentioned in Slack in #core by toddhalfpenny. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-media by toddhalfpenny. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-media by desrosj. View the logs.
7 years ago
#24
@
7 years ago
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. :)
#25
@
7 years ago
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.
#26
@
7 years ago
@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
.
#28
@
7 years ago
- Keywords has-patch needs-unit-tests added
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.
#29
follow-ups:
↓ 30
↓ 37
@
7 years ago
Wouldn't checking the file like this be a problem for performance?
#30
in reply to:
↑ 29
@
7 years ago
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. :)
#31
@
7 years ago
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.
#32
follow-up:
↓ 33
@
7 years ago
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.
#33
in reply to:
↑ 32
@
7 years ago
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.
This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.
7 years ago
#37
in reply to:
↑ 29
@
7 years ago
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.
This ticket was mentioned in Slack in #core-media by mike. View the logs.
7 years ago
#39
@
7 years ago
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).
#40
@
7 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
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
This ticket was mentioned in Slack in #core-media by mike. View the logs.
6 years ago
#43
@
6 years ago
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.
#44
@
6 years ago
- Milestone changed from 5.1 to 5.2
- Owner set to mikeschroder
- Status changed from new to assigned
- Summary changed from Inconsistent Handling of mp4 by Audio Widget to Detect and Handle MP4 and OGG audio files properly
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.
#45
@
6 years ago
- Keywords early added; 2nd-opinion removed
- Milestone changed from 5.2 to 5.3
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.
This ticket was mentioned in Slack in #core by mike. View the logs.
5 years ago
#48
@
5 years ago
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.
This ticket was mentioned in Slack in #core-media by mike. View the logs.
5 years ago
#50
@
5 years ago
@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!
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
#52
@
5 years ago
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.
#53
@
5 years ago
- Keywords needs-patch reporter-feedback added; dev-feedback has-patch removed
- Owner mikeschroder deleted
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 for 5.3.
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.
Animated gif of viewing the widget in the frontend.