-
Notifications
You must be signed in to change notification settings - Fork 417
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
AudioStream: remove unused youtube-specific fields #1150
base: dev
Are you sure you want to change the base?
AudioStream: remove unused youtube-specific fields #1150
Conversation
These fields were added in TeamNewPipe#810 in preparation for a DASH support that never materialized … well it did materialize, but did not use any of these variables. Since they are youtube-specific, it does not make much sense to export them in the generic API, lest people start depending on them as if they belonged to all backends. Well, 4 variables are actually already depended on in places, they will have to be checked separately. But this is the low-hanging fruit.
a719b7d
to
8448f0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
YoutubeDashManifestCreatorsTest.testProgressiveStreams()
fails locally.
// Fields for DASH | ||
private int itag = ITAG_NOT_AVAILABLE_OR_NOT_APPLICABLE; | ||
private int bitrate; | ||
private int initStart; | ||
private int initEnd; | ||
private int indexStart; | ||
private int indexEnd; | ||
private String quality; | ||
private String codec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I was the one who added these fields originally, they are used in Piped's backend for providing sidX boxes for streaming with DASH.
Here's how an audio representation is created by a client: https://github.com/TeamPiped/Piped/blob/8ee2a1c20737ce9db84186e00849b405d6c097af/src/utils/DashUtils.js#L99-L117
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe a comment should be added that these were added for Piped? Similar confusion can take place whenever someone compares NP and NPE code in the future.
These fields were added in
#810 in preparation for a DASH support that never materialized … well it did materialize, but did not use any of these variables.
Since they are youtube-specific, it does not make much sense to export them in the generic API, lest people start depending on them as if they belonged to all backends.
Well, 4 variables are actually already depended on in places, they will have to be checked separately. But this is the low-hanging fruit.