Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MP3 audio playback capability checking with HLS #5419

Merged
merged 6 commits into from
Apr 30, 2024

Conversation

gnattu
Copy link
Member

@gnattu gnattu commented Apr 25, 2024

Changes

The previous check was too naive, assuming that most browsers that support playing MP3 directly in an mp4 file can support MP3 with HLS. However, this assumption is wrong. In fact, most browsers won’t play MP3 with HLS, with Safari being the only exception. Even on Safari, MP3 support with HLS is limited to the mpegts container, and it won’t play MP3 in the fmp4 container.

Issues

See forum comment: https://forum.jellyfin.org/t-directplay-not-working-but-transcoding-no-issues?pid=21248#pid21248

The previous check was too naive, assuming that most browsers that support playing MP3 directly in an mp4 file can support MP3 with HLS. However, this assumption is wrong. In fact, most browsers won’t play MP3 with HLS, with Safari being the only exception. Even on Safari, MP3 support with HLS is limited to the mpegts container, and it won’t play MP3 in the fmp4 container.
@gnattu gnattu requested a review from a team as a code owner April 25, 2024 22:03
@felix920506
Copy link
Member

When I was testing #5221 I found that this also happens if vorbis -> mp3 in TS, wouldn't this PR cause problems?

@gnattu
Copy link
Member Author

gnattu commented Apr 26, 2024

When I was testing #5221 I found that this also happens if vorbis -> mp3 in TS, wouldn't this PR cause problems?

Very very unlikely because:

  • mp3 is never prioritized for transcoding because Safari supports aac and prefers aac
  • 10.9 use fmp4 as default container which does not support mp3 anyway

For some specific client that insists use mp3 for weird reasons, ask the client's developer to make a change.

@dmitrylyzo
Copy link
Contributor

For some specific client that insists use mp3 for weird reasons, ask the client's developer to make a change.

To make this possible for the web-based client (Tizen, webOS, Android, Expo), you need to add a property to options as is done for TrueHD, Dolby Vision.

@gnattu
Copy link
Member Author

gnattu commented Apr 26, 2024

For some specific client that insists use mp3 for weird reasons, ask the client's developer to make a change.

To make this possible for the web-based client (Tizen, webOS, Android, Expo), you need to add a property to options as is done for TrueHD, Dolby Vision.

Do clients like this really exist? Not supporting AAC with HLS but supporting MP3 with HLS?

@dmitrylyzo
Copy link
Contributor

Do clients like this really exist? Not supporting AAC with HLS but supporting MP3 with HLS?

Even if AAC is supported, MP3 can be copied (remux) if it is also supported.

@gnattu
Copy link
Member Author

gnattu commented Apr 26, 2024

Do clients like this really exist? Not supporting AAC with HLS but supporting MP3 with HLS?

Even if AAC is supported, MP3 can be copied (remux) if it is also supported.

No, because most browsers do not support MP3 with HLS at all, and this is the whole point of this PR. We are causing browsers playing videos without audio because we are trying feeding MP3 stream with HLS, and then the browser simply drops the audio.

@felix920506
Copy link
Member

@dmitrylyzo I think I will need to clarify a bit. The issue I linked ONLY happens when transcoding from Vorbis to AAC or MP3 in HLS using TS container.

@dmitrylyzo
Copy link
Contributor

@dmitrylyzo I think I will need to clarify a bit. The issue I linked ONLY happens when transcoding from Vorbis to AAC or MP3 in HLS using TS container.

This PR is just blocking MP3 in TS (if not supported), not just Vorbis to MP3 transcoding.

My concern is that gnattu left no option for the web-based clients to specify "MP3 to TS" support.

@dmitrylyzo
Copy link
Contributor

No, because most browsers do not support MP3 with HLS at all, and this is the whole point of this PR. We are causing browsers playing videos without audio because we are trying feeding MP3 stream with HLS, and then the browser simply drops the audio.

Tizen and webOS are also browsers, but they may violate standards.

@gnattu
Copy link
Member Author

gnattu commented Apr 26, 2024

My concern is that gnattu left no option for the web-based clients to specify "MP3 to TS" support.
Tizen and webOS are also browsers, but they may violate standards.

This is something very different from DTS and TrueHD. MP3 is a very common audio format, and users will have very wrong expectations about the supporting status of this codec without realizing that this considered legacy codec is not supported by most browsers in HLS, which will cause confusion and false positive bug reports like "MP3 audio is not played". If you can confirm that Tizen and WebOS do support MP3 in TS/fmp4, then we can do a hardcode like we already did with Safari, but I really don't want this to to be exposed as a user-configurable option. This will only confuse our user.

@dmitrylyzo
Copy link
Contributor

dmitrylyzo commented Apr 26, 2024

I really don't want this to to be exposed as a user-configurable option.

Not a user-configurable option - add supportsMp3InTs to options. This will allow apps (wrappers) to pass the state of the "MP3 in TS" support. Currently, they will have to generate the entire device profile manually or fix unspecified support in the generated device profile.

UPD:
Ahh, I see why you started thinking about a user-configurable option - my recent PR. HDR/DV would be better to use as an example. 😄

@gnattu
Copy link
Member Author

gnattu commented Apr 26, 2024

options.supportsMp3InTs

Added this to TS check to allow overriding.

@dmitrylyzo
Copy link
Contributor

If you can confirm that Tizen and WebOS do support MP3 in TS/fmp4

My Tizen 4 TV can play HLS-TS remux (H264+MP3).
HLS-fMP4 remux (H264+MP3) - no sound, but fMP4 doesn't work well on my TV.

@gnattu
Copy link
Member Author

gnattu commented Apr 26, 2024

My Tizen 4 TV can play HLS-TS remux (H264+MP3). HLS-fMP4 remux (H264+MP3) - no sound, but fMP4 doesn't work well on my TV.

Then it is similar to Safari. Most browsers will have no sound with HLS-fMP4 remux with mp3 so ti is not your TV's fault. I have not found one browser that can play sound normally with this setting yet.

@dmitrylyzo
Copy link
Contributor

dmitrylyzo commented Apr 29, 2024

Tested Chrome 79, 112 - they play MP3 in HLS-TS, even though canPlayType returns an empty string.
Firefox 113 plays MP3 in HLS-TS as well.

@gnattu
Copy link
Member Author

gnattu commented Apr 30, 2024

Tested Chrome 79, 112 - they play MP3 in HLS-TS, even though canPlayType returns an empty string.

How could you find chrome this old ? Just curious. On Chrome 122 it straight errors out if you are having mp3 as the audio track of a video stream in HLS.

@dmitrylyzo
Copy link
Contributor

How could you find chrome this old ? Just curious.

VMs with old Ubuntu. 😉

On Chrome 122 it straight errors out if you are having mp3 as the audio track of a video stream in HLS.

Google probably broke their player in a several places:

from jellyfin/jellyfin#10756
Chrome 115+ detects only default audio tracks (bug?).

@gnattu
Copy link
Member Author

gnattu commented Apr 30, 2024

Verified with Chrome 123 and it seems to play fine with ffmpeg generated sample video.

I relaxed the mp3 playback in ts container so that any player reporting it supports mp3 plus safari can play it in ts. I don't know if this is the desired but this should allow remux for most browsers. I cannot verify all browsers so I will call it a day.

The TS container has never been my main focus for this PR anyway. I'm not particularly concerned about whether MP3 can be played in TS, as it is no longer our default container, and modern video codecs cannot be muxed in the TS container. My main focus for this PR is to prevent MP3 from being included in the fMP4 container, as this is causing playback issues for our users, especially since most browsers cannot play mp3 in fMP4.

@nielsvanvelzen nielsvanvelzen added this to the v10.9.0 milestone Apr 30, 2024
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main focus for this PR is to prevent MP3 from being included in the fMP4 container, as this is causing playback issues for our users, especially since most browsers cannot play mp3 in fMP4.

Then we should probably revert the changes affecting TS.

But we can't fix MP3 in fMP4 for DirectPlay HLS-fMP4 because there is no subcontainer property in DirectPlayProfile:

// HACK: Since there is no filter for TS/MP4 in the API, specify HLS support in general and rely on retry after DirectPlay error
// FIXME: Need support for {Container: 'mp4', Protocol: 'hls'} or {Container: 'hls', SubContainer: 'mp4'}

src/scripts/browserDeviceProfile.js Outdated Show resolved Hide resolved
@gnattu
Copy link
Member Author

gnattu commented Apr 30, 2024

Then we should probably revert the changes affecting TS.

But we can't fix MP3 in fMP4 for DirectPlay HLS-fMP4 because there is no subcontainer property in DirectPlayProfile:

If TS related code will affect fMP4 then my suggesting would be always transcoding and drop the MP3 direct play totally. But it seems like it is not for now.

Also, we should not care for non-exist browser this much.

@dmitrylyzo
Copy link
Contributor

If TS related code will affect fMP4 then my suggesting would be always transcoding and drop the MP3 direct play totally. But it seems like it is not for now.

Then I would drop fMP4 direct playback.
That code is for LiveTV direct playback.

Also, we should not care for non-exist browser this much.

Don't forget about smart TVs - in most cases they are outdated browsers.

@gnattu
Copy link
Member Author

gnattu commented Apr 30, 2024

Then I would drop fMP4 direct playback.

Current code is already very similar to "drop fMP4 direct playback" because most browsers will not return true for application/vnd.apple.mpegURL; codecs="avc1.64001E, mp4a.40.34, this is just in case. Or should I remove this check as well?

Don't forget about smart TVs - in most cases they are outdated browsers.

I don't think this has meaningful impact on smart TVs either, because fmp4 is required to have modern fancy stuff like HEVC and HDR, which is not compatible with MP3 anyway. This codec is usually only included in some legacy videos and I highly doubt if any user would switch to mpegts just for mp3 direct play.

@dmitrylyzo
Copy link
Contributor

Current code is already very similar to "drop fMP4 direct playback" because most browsers will not return true for application/vnd.apple.mpegURL; codecs="avc1.64001E, mp4a.40.34, this is just in case. Or should I remove this check as well?

The problem is that we generate DirectPlay profiles for both, HLS-TS and HLS-fMP4, but they cannot be distinguished from each other. So if the client supports MP3 in HLS-TS, the server will also use that profile for HLS-fMP4.

I don't think this has meaningful impact on smart TVs either, because fmp4 is required to have modern fancy stuff like HEVC and HDR, which is not compatible with MP3 anyway.

My Tizen 4 TV can play HEVC in HLS-TS. As I said, TV manufacturers don't always follow standards.

This codec is usually only included in some legacy videos and I highly doubt if any user would switch to mpegts just for mp3 direct play.

Or if fMP4 doesn't work (my case).

@gnattu
Copy link
Member Author

gnattu commented Apr 30, 2024

So if the client supports MP3 in HLS-TS, the server will also use that profile for HLS-fMP4.

From my own testing, it does not, at least not for videos with the "ContainerNotSupported" flag. The fmp4 profile is prioritized over the mpegts profile(because fmp4 profile is placed in front of mpegts profile), and the server selects the profile based on the video codec rather than the audio codec. For example, for an H.264 video with MP3 audio in an unsupported container, the server will choose the fmp4 profile because it is the first one in the order that supports the H.264 video codec. After that, the server will found that it does not support mp3, and then it will perform a transcode.

@dmitrylyzo
Copy link
Contributor

From my own testing, it does not, at least not for videos with the "ContainerNotSupported" flag. The fmp4 profile is prioritized over the mpegts profile(because fmp4 profile is placed in front of mpegts profile), and the server selects the profile based on the video codec rather than the audio codec. For example, for an H.264 video with MP3 audio in an unsupported container, the server will choose the fmp4 profile because it is the first one in the order that supports the H.264 video codec. After that, the server will found that it does not support mp3, and then it will perform a transcode.

Yes, it works fine when it can't direct play.

I am taking about direct play of HLS-TS and HLS-fMP4 (LiveTV). The server looks at each DirectPlayProfile to select the most compatible one, and it can choose HLS-TS direct profile for HLS-fMP4 because, as I said, there is no additional label (subcontainer).

Sometimes I think we could merge DirectPlayProfiles and TranscodingProfiles (leaving the TranscodingProfile class) so that the client only has a list of supported formats that we can transcode to if needed.
At the same time, having them separate gives more flexibility.

@gnattu
Copy link
Member Author

gnattu commented Apr 30, 2024

I am taking about direct play of HLS-TS and HLS-fMP4 (LiveTV). The server looks at each DirectPlayProfile to select the most compatible one, and it can choose HLS-TS direct profile for HLS-fMP4 because, as I said, there is no additional label (subcontainer).

  • LiveTV is broken in many ways
  • LiveTV is unlikely to have fmp4 streams due to complicated reasons and the profile here will be more or less meaningless
  • Even if the LiveTV has fmp4 container it is very unlikely to have mp3 in it due to compatibility reasons.

I don't see any problems here.

Co-authored-by: Dmitry Lyzo <56478732+dmitrylyzo@users.noreply.github.com>
Copy link

sonarcloud bot commented Apr 30, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 762ea7f
Status ✅ Deployed!
Preview URL https://4f9fbd22.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs
View bot logs

Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes

  • Add MP3 in HLS-TS for Safari.
  • Add strict validation for MP3 in HLS-fMP4.

@nielsvanvelzen nielsvanvelzen added bug Something isn't working playback This PR or issue mainly concerns playback labels Apr 30, 2024
@thornbill thornbill merged commit 66e25df into jellyfin:master Apr 30, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working playback This PR or issue mainly concerns playback
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants