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

Allow using background player when there are no separate audio streams #9562

Conversation

evermind-zz
Copy link
Contributor

Some services may only have video streams and no separate audio streams available. This commit will add audio background playback support for those services. It uses the video source as audio source for background playback.

Note I think currently NewPipe is in no need to support this feature but maybe in the future. I use it in BraveNewPipe as some services do not support audio only streams. Would be great if it could be merged.
Thx

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

  • audio playback for services that do not support audio streams.
  • use audio from video stream for audio only background playback

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

Some services may only have video streams and no separate audio streams available.
This commit will add audio background playback support for those services.
It uses the video source as audio source for background playback.
@ShareASmile
Copy link
Collaborator

ShareASmile commented Dec 17, 2022

Yes Thanks, it works on Such YouTube Link with only 360p Video no separate audio.
https://www.youtube.com/watch?v=cw2gJxNx6xs

My NewPipe usecase mostly is for background listening & on above Link when i long press into background it spews following error on current dev of NewPipe

Exception

  • User Action: play stream
  • Request: Loading failed for [Pahle Nachiye [Full Song] Nachiye Gayiye Shagan Manayiye]: https://www.youtube.com/watch?v=cw2gJxNx6xs
  • Content Country:
  • Content Language: en-
  • App Language: en
  • Service: YouTube
  • Version: 0.24.1
  • OS: Linux samsung/a22xnnxx/a22x:11/RP1A.200720.012/A226BXXU4AVB1:user/release-keys 11 - 30
Crash log

org.schabi.newpipe.player.mediasource.FailedMediaSource$MediaSourceResolutionException: Unable to resolve source from stream info. URL: https://www.youtube.com/watch?v=cw2gJxNx6xs, audio count: 0, video count: 0, 1
	at org.schabi.newpipe.player.playback.MediaSourceManager.lambda$getLoadedMediaSource$3$org-schabi-newpipe-player-playback-MediaSourceManager(MediaSourceManager.java:432)
	at org.schabi.newpipe.player.playback.MediaSourceManager$$ExternalSyntheticLambda0.apply(Unknown Source:6)
	at io.reactivex.rxjava3.internal.operators.single.SingleMap$MapSingleObserver.onSuccess(SingleMap.java:58)
	at io.reactivex.rxjava3.internal.operators.single.SingleDoOnError$DoOnError.onSuccess(SingleDoOnError.java:52)
	at io.reactivex.rxjava3.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.onSuccess(SingleSubscribeOn.java:68)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeToSingle$ToSingleMaybeSubscriber.onSuccess(MaybeToSingle.java:83)
	at io.reactivex.rxjava3.internal.operators.flowable.FlowableElementAtMaybe$ElementAtSubscriber.onNext(FlowableElementAtMaybe.java:80)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeConcatArray$ConcatMaybeObserver.drain(MaybeConcatArray.java:136)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeConcatArray$ConcatMaybeObserver.request(MaybeConcatArray.java:78)
	at io.reactivex.rxjava3.internal.operators.flowable.FlowableElementAtMaybe$ElementAtSubscriber.onSubscribe(FlowableElementAtMaybe.java:66)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeConcatArray.subscribeActual(MaybeConcatArray.java:42)
	at io.reactivex.rxjava3.core.Flowable.subscribe(Flowable.java:15868)
	at io.reactivex.rxjava3.internal.operators.flowable.FlowableElementAtMaybe.subscribeActual(FlowableElementAtMaybe.java:36)
	at io.reactivex.rxjava3.core.Maybe.subscribe(Maybe.java:5330)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeToSingle.subscribeActual(MaybeToSingle.java:46)
	at io.reactivex.rxjava3.core.Single.subscribe(Single.java:4813)
	at io.reactivex.rxjava3.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
	at io.reactivex.rxjava3.core.Scheduler$DisposeTask.run(Scheduler.java:644)
	at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:65)
	at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:56)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
	at java.lang.Thread.run(Thread.java:923)


@evermind-zz
Copy link
Contributor Author

great so NewPipe has also a use case for this PR

@AudricV
Copy link
Member

AudricV commented Dec 17, 2022

Note: I already made a fix for this bug in #8603.

@opusforlife2 opusforlife2 added feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background) labels Dec 18, 2022
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! Code looks good to me. I tested and this seems to work fine, by switching to background from the play queue on the video provided by @ShareASmile. However, on that video, the background button is still hidden in the video details fragment. Could you fix that, please?

@AudricV can this be merged even though #8603 also fixes the same issue? If you think it's better to just finish off #8603 then maybe we could do that and avoid some rebasing issues.

@evermind-zz
Copy link
Contributor Author

Thank you! Code looks good to me. I tested and this seems to work fine, by switching to background from the play queue on the video provided by @ShareASmile. However, on that video, the background button is still hidden in the video details fragment. Could you fix that, please?

I'd rather wait for @AudricV to comment. AudricV's could best compare the different approaches. I think mine is more lower level meaning it is contained in AudioPlaybackResolver. transparently to upper layers.

@Stypox
Copy link
Member

Stypox commented Jan 1, 2023

@AudricV said #8603 will not be merged for a while (it's a draft atm), so for me it's fine to merge this PR before. Your changes will at some point be overwritten by #8603, but in the meantime this will solve some issues. Thanks!

@sonarcloud
Copy link

sonarcloud bot commented Jan 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 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

I tested again, also taking a look at the network inspector, and it seems to work well. Thank you!

@Stypox Stypox merged commit 7454b31 into TeamNewPipe:dev Jan 2, 2023
@Stypox Stypox changed the title Support audio only background for services only supporting video streams Allow using background player when there are no separate audio streams Jan 2, 2023
@Stypox Stypox mentioned this pull request Jan 22, 2023
3 tasks
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 player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants