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

AudioStream: remove unused youtube-specific fields #1150

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Profpatsch
Copy link
Contributor

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.

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

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.
@Profpatsch Profpatsch force-pushed the AudioStream-remove-unused-fields branch from a719b7d to 8448f0a Compare January 7, 2024 00:08
@TobiGr TobiGr added youtube service, https://www.youtube.com/ codequality Improvements to the codebase to improve the code quality labels Mar 18, 2024
Copy link
Member

@TobiGr TobiGr left a 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!

⚠️ The important tests were not run on this PR for some reason

YoutubeDashManifestCreatorsTest.testProgressiveStreams() fails locally.

Comment on lines -36 to -44
// 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;
Copy link
Member

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

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality youtube service, https://www.youtube.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants