WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 4 months ago

#40921 new defect (bug)

Inconsistent Handling of mp4 by Audio Widget

Reported by: toddhalfpenny Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 3.6
Component: Media Keywords: 2nd-opinion dev-feedback has-screenshots has-patch has-unit-tests
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)

mediawidgets-01.gif (74.1 KB) - added by toddhalfpenny 13 months ago.
Animated gif of viewing the widget in the frontend.
40921.diff (534 bytes) - added by toddhalfpenny 13 months ago.
Adding mp4 to wp_audio_extensions
WhatsApp Audio 2017-05-14 at 12.58.14.mp4 (5.3 MB) - added by toddhalfpenny 11 months ago.
Audio mp4 file - this is the one I was using when I spotted the bug.
40921.2.diff (4.0 KB) - added by blobfolio 8 months ago.
Rename MP4 and OGG at upload as needed.
40921.3.diff (4.0 KB) - added by blobfolio 7 months ago.
Refresh against current src
40921.tests.diff (1.3 KB) - added by soulseekah 4 months ago.
ogg, mp4 deep inspection tests
small-audio.ogg (4.2 KB) - added by soulseekah 4 months ago.
100ms sinewave ogg
small-video.ogg (39.6 KB) - added by soulseekah 4 months ago.
1x1 ogg conversion of small-video.mp4
small-audio.mp4 (5.4 KB) - added by soulseekah 4 months ago.
100ms sine wave mp4 (libvorbis codec)
40921.4.diff (6.0 KB) - added by blobfolio 3 months ago.
Merge unit tests into feature patch.

Change History (50)

@toddhalfpenny
13 months ago

Animated gif of viewing the widget in the frontend.

@toddhalfpenny
13 months ago

Adding mp4 to wp_audio_extensions

#1 @toddhalfpenny
13 months ago

  • Keywords has-patch added

#2 @westonruter
13 months 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 @toddhalfpenny
13 months 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 @westonruter
13 months 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 @toddhalfpenny
13 months 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.

#6 @westonruter
13 months ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core by toddhalfpenny. View the logs.


12 months ago

#8 follow-up: @timmydcrawford
12 months 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.


12 months ago

#10 in reply to: ↑ 8 @obenland
12 months 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.


12 months ago

#12 @toddhalfpenny
12 months ago

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

This ticket was mentioned in Slack in #core by toddhalfpenny. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


11 months ago

@toddhalfpenny
11 months ago

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

#15 @toddhalfpenny
11 months ago

  • Keywords dev-feedback has-screenshots added

#16 @blobfolio
11 months ago

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.

This ticket was mentioned in Slack in #core by toddhalfpenny. View the logs.


9 months ago

#18 @SergeyBiryukov
9 months ago

  • Milestone changed from Future Release to 4.9

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


9 months ago

#20 @toddhalfpenny
9 months ago

  • Keywords needs-patch removed

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-media by toddhalfpenny. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


8 months ago

#24 @blobfolio
8 months 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. :)

Last edited 8 months ago by blobfolio (previous) (diff)

#25 @mikeschroder
8 months 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 @blobfolio
8 months 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.

#27 @westonruter
8 months ago

  • Milestone changed from 4.9 to Future Release

@blobfolio
8 months ago

Rename MP4 and OGG at upload as needed.

#28 @blobfolio
8 months 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: @westonruter
8 months ago

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

#30 in reply to: ↑ 29 @blobfolio
8 months 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 @blobfolio
8 months 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: @toddhalfpenny
8 months 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 @blobfolio
8 months 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.


8 months ago

#35 @joemcgill
8 months ago

  • Milestone changed from Future Release to 4.9.1

#36 @johnbillion
7 months ago

  • Milestone changed from 4.9.1 to 5.0

@blobfolio
7 months ago

Refresh against current src

#37 in reply to: ↑ 29 @mikeschroder
5 months 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.


5 months ago

#39 @soulseekah
4 months 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).

@soulseekah
4 months ago

ogg, mp4 deep inspection tests

@soulseekah
4 months ago

100ms sinewave ogg

@soulseekah
4 months ago

1x1 ogg conversion of small-video.mp4

@soulseekah
4 months ago

100ms sine wave mp4 (libvorbis codec)

#40 @soulseekah
4 months 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

@blobfolio
3 months ago

Merge unit tests into feature patch.

Note: See TracTickets for help on using tickets.