WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 4 weeks 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 needs-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 (5)

mediawidgets-01.gif (74.1 KB) - added by toddhalfpenny 7 months ago.
Animated gif of viewing the widget in the frontend.
40921.diff (534 bytes) - added by toddhalfpenny 7 months ago.
Adding mp4 to wp_audio_extensions
WhatsApp Audio 2017-05-14 at 12.58.14.mp4 (5.3 MB) - added by toddhalfpenny 5 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 2 months ago.
Rename MP4 and OGG at upload as needed.
40921.3.diff (4.0 KB) - added by blobfolio 2 weeks ago.
Refresh against current src

Change History (41)

@toddhalfpenny
7 months ago

Animated gif of viewing the widget in the frontend.

@toddhalfpenny
7 months ago

Adding mp4 to wp_audio_extensions

#1 @toddhalfpenny
7 months ago

  • Keywords has-patch added

#2 @westonruter
7 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
7 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
7 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
7 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
7 months ago

  • Keywords 2nd-opinion added

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


6 months ago

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


6 months ago

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


6 months ago

#12 @toddhalfpenny
6 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.


5 months ago

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


5 months ago

@toddhalfpenny
5 months ago

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

#15 @toddhalfpenny
5 months ago

  • Keywords dev-feedback has-screenshots added

#16 @blobfolio
5 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.


3 months ago

#18 @SergeyBiryukov
3 months ago

  • Milestone changed from Future Release to 4.9

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


3 months ago

#20 @toddhalfpenny
3 months ago

  • Keywords needs-patch removed

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


2 months ago

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


2 months ago

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


2 months ago

#24 @blobfolio
2 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 2 months ago by blobfolio (previous) (diff)

#25 @mikeschroder
2 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
2 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
2 months ago

  • Milestone changed from 4.9 to Future Release

@blobfolio
2 months ago

Rename MP4 and OGG at upload as needed.

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

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

#30 in reply to: ↑ 29 @blobfolio
2 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
2 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
2 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
2 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 weeks ago

#35 @joemcgill
8 weeks ago

  • Milestone changed from Future Release to 4.9.1

#36 @johnbillion
4 weeks ago

  • Milestone changed from 4.9.1 to 5.0

@blobfolio
2 weeks ago

Refresh against current src

Note: See TracTickets for help on using tickets.