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

Add support for multiple audio tracks #9937

Merged
merged 24 commits into from
May 2, 2023
Merged

Conversation

Theta-Dev
Copy link
Contributor

@Theta-Dev Theta-Dev commented Mar 18, 2023

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Adds an audio language selector next to the video quality selector
  • Default audio track is chosen by the configured content language. If no track matches, it defaults to English.
  • New settings option: Content > Prefer descriptive audio (choose the descriptive audio track by default)

Before/After Screenshots/Screen Record

language_selector

Fixes the following issue(s)

APK testing

Due diligence

@AudricV AudricV added feature request Issue is related to a feature in the app multiservice Issues related to multiple services player Issues related to any player (main, popup and background) labels Mar 18, 2023
@AudricV
Copy link
Member

AudricV commented Mar 18, 2023

Thanks a lot! Here are some changes that I would suggest to do and that you need to do:

  • For the audio settings you introduced, I would move the descriptive audio setting to Video and audio.

  • For the language fallback you use, I would use the default locale instead of the English one, unless it creates some issues.

  • For the audio track label, I am not sure if we should use the label set by the streaming service, as it creates localization issues. Track names provided by YouTube provides whether an audio track is an original or not, but that's only on recent videos, so we can't reliably extract this information on the label (and that's why I didn't added this property on the extractor, as I am not sure whether we should add it).

    Instead, I would use a string with the audio language translated in the app UI language and include the descriptive audio property if it is true with a string format (%1s (%2s) with %1s as the audio language and %2s as descriptive).

You need to fix the following behaviors:

  • the background players (external and in-app) and video downloader ignore the settings you introduced;

  • the downloader do not list the audio language for audio downloads;

  • you should use the default audio language or the fallback language for video downloads and external players at least, or better, if you want to do this in your PR and if other team members agree, add the audio selector below the video one for downloads and create an audio streams dialog for external players: this would give flexibility to users. Someone else could always do the latter part :)

    This audio selector must be only present for demuxed streams and a message should be shown for muxed streams saying something like An audio track should be already present in this stream.
    The introduction of this message also raises the question to show all video streams and not merge video only and muxed video streams in the same list, as for instance, downloading a Mr Beast video in 720p with French audio track would not be possible in this case.

@SameenAhnaf

This comment was marked as duplicate.

@AudricV

This comment was marked as resolved.

@sonarqubecloud

This comment was marked as outdated.

@AudricV
Copy link
Member

AudricV commented Mar 19, 2023

Bug: the audio selection algorithm doesn't seem to take into account anymore the preferred audio format.

When selecting an audio track of latest Mr Beast's video for an external audio player with the M4A codec, I got the URL of a WEBM format (itag 251) where I should have got the itag 140.

Also, I don't know if that's intended or if you did changes since my comment about that, but no audio language is still shown in the downloader audio list.

@Theta-Dev
Copy link
Contributor Author

Hi @AudricV,
I am still working on the downloader, so I have not pushed it yet.
I'll look into the codec selection issue.

@Theta-Dev
Copy link
Contributor Author

Alright, I have everything implemented.

Do you know why the android tests time out?

Stypox

This comment was marked as duplicate.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Things that should be tested before merging, on videos with various combinations of formats and languages. @Theta-Dev it would be great if you could provide a list of videos that have different features (e.g. no audio stream at all (i.e. only video streams with embedded audio), only original audio, descriptive audio, dubbed audio, all audio types in multiple languages, possibly not only from YouTube but from other platforms, too).

  • downloading streams
  • external player
  • audio track menu both in the play queue activity and in the player

Thanks :-)

@Theta-Dev
Copy link
Contributor Author

Theta-Dev commented Apr 21, 2023

Videos for testing:
no external audio: any Peertube or CCC video
original+descriptive audio: https://www.youtube.com/watch?v=TjxC-evzxdk
original+dub: https://www.youtube.com/watch?v=_8W2LIfl5RE
original+dub+descriptive: https://www.youtube.com/watch?v=Kn56bMZ9OE8
multilanguage (no audio stream types): https://www.youtube.com/watch?v=tVWWp1PqDus
multilanguage (with audio stream types): https://www.youtube.com/watch?v=YLt73w6criQ

@Theta-Dev
Copy link
Contributor Author

Okay, I applied the changes and updated the PR.

@Theta-Dev Theta-Dev requested a review from Stypox April 22, 2023 00:41
@AudricV AudricV changed the title Add audio language selector Add audio track selector Apr 25, 2023
Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Please address the following changes and try to avoid in the future that your IDE or yourself does formatting changes which make reviews harder.

@Theta-Dev Theta-Dev requested a review from AudricV April 30, 2023 22:06
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! I tested everything in multiple ways, as said, and everything worked smoothly. The only small inconsistency I found is that the external audio player dialog does not have a "Open in browser" button and opens the audio directly if there is only one audio (which is a different behavior wrt videos). If you agree with me on changing the external audio player dialog to make it consistent with the video one, please do so ;-)

AudricV added 2 commits May 2, 2023 00:11
We don't know if, on muxed video streams we get for all services which support
multiple audio languages, that the audio language returned is the original one
or not, even if it should be the case.

In order to avoid saying potential false information, this word has been
removed from the string resource (ID and value) and the corresponding layout ID
in the download dialog.
This change makes the dialog consistent with the video one.
@AudricV AudricV changed the title Add audio track selector Add support for multiple audio tracks May 1, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Stypox Stypox merged commit 2315b08 into TeamNewPipe:dev May 2, 2023
@DebugKirby
Copy link

All the testing apk links are 404'd
How do I get this apk?

@ShareASmile
Copy link
Collaborator

ShareASmile commented May 28, 2023

As this code has been merged into dev branch. You can Try NewPipe Nightly builds:
https://github.com/TeamNewPipe/NewPipe-nightly/releases

@TobiGr
Copy link
Contributor

TobiGr commented Jun 22, 2023

I found a regression in the latest nightly which was most likely introduced by this PR: #10180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app multiservice Issues related to multiple services player Issues related to any player (main, popup and background)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[STOP OPENING DUPLICATES] Support multiple audio tracks
7 participants