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

Support SoundCloud HLS-only tracks by using a workaround #526

Merged
merged 8 commits into from
Mar 19, 2021

Conversation

AudricV
Copy link
Member

@AudricV AudricV commented Jan 23, 2021

Support SoundCloud HLS-only streams by parsing M3U manifests, get the last segment URL (in order to get track length) and request a segment URL equals to track's duration so it's a single URL (0/track_length/).

Related issue: #273

Test APK: app.zip (from my fork with GitHub CI, app based on dev branch)

To do: add tests, if there is no risk of takedown of this repo by RIAA or by other music rights management companies by the use of SoundCloud HLS-only tracks.

@AudricV AudricV force-pushed the snd-hls-workaround branch 2 times, most recently from e81ce7a to 636c4cd Compare January 23, 2021 17:26
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.

The issue is apparently that the streams that you open always end up in a continue; case, and therefore at the end of getAudioStreams() audioStreams.isEmpty() is true.

@AudricV AudricV force-pushed the snd-hls-workaround branch 3 times, most recently from 1e9728c to 02a6008 Compare January 23, 2021 17:44
@Stypox
Copy link
Member

Stypox commented Jan 23, 2021

@TiA4f8R ok, I'm gonna try to debug this myself. Since I'm pretty busy in the next few days, if I forget about this feel free to ping me ;-)

@AudricV
Copy link
Member Author

AudricV commented Jan 24, 2021

@Stypox I've seen what's wrong: it's my regex.

@AudricV AudricV marked this pull request as ready for review January 24, 2021 13:59
@AudricV
Copy link
Member Author

AudricV commented Jan 24, 2021

@Stypox Code ready for review and HLS-only tracks are now streamable.

@AudricV AudricV force-pushed the snd-hls-workaround branch 2 times, most recently from dee7ac0 to 228a12e Compare February 5, 2021 18:11
@AudricV AudricV force-pushed the snd-hls-workaround branch 2 times, most recently from df34ab8 to 0ad8bdf Compare February 19, 2021 17:35
Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

Overall the code is very hard to read and there are no unit tests for the added functionality.

There is a for loop which spans across nearly 100 lines (I think, cant really see where it ends). Add to that if-else if-else, more loops inside and sudden jumps with `continue´ where you have to know in which loop you are.


Try to extract out some methods. You already have comments describing that the section is for a specific type of endpoint. Make a method out of that.

I just looked at it until the my last comment. I did not look at the rest.

@XiangRongLin
Copy link
Collaborator

Something along the lines of having getAudioStream be an integration method and the extracted method containing the actual operation. The integration method can then be "read" like normal text and not programming code, to get to know what is done.

see https://clean-code-developer.com/grades/grade-1-red/#Integration_Operation_Segregation_Principle_IOSP

@B0pol
Copy link
Member

B0pol commented Feb 19, 2021

Also extract the regex pattern to a constant and give it a proper name.

TobiGr added a commit to AudricV/NewPipeExtractor that referenced this pull request Feb 22, 2021
The ContentNotSupportedException is thrown because no supported audio streams where extracted. However, SoundCLoud does not check, whether there are any streams available. 
This commit should be reverted in TeamNewPipe#526
AudricV pushed a commit to AudricV/NewPipeExtractor that referenced this pull request Feb 26, 2021
The ContentNotSupportedException is thrown because no supported audio streams where extracted. However, SoundCLoud does not check, whether there are any streams available. 
This commit should be reverted in TeamNewPipe#526
@XiangRongLin
Copy link
Collaborator

@TiA4f8R I'm still not seeing any unit tests, please add those.
Before i review the code for quality, i would like to know beforehand that it actually works by seeing unit tests.

@AudricV AudricV force-pushed the snd-hls-workaround branch 4 times, most recently from e75bcf2 to 1d2b35e Compare March 13, 2021 16:47
@AudricV
Copy link
Member Author

AudricV commented Mar 13, 2021

@XiangRongLin What do you think now?

@XiangRongLin
Copy link
Collaborator

@TiA4f8R Grab yourself the SonarLint plugin. It gives you an objective review of bad practices, code smell and much more. In this case cyclomatic complexity is the important one.

From a quick scan it seems only a minor problem is present in the part that you touched. Decide for yourself if it is worth fixing, since it's probably you that has to touch that code again and try to comprehend it. I would just skip it for now. Come back when you have ideas on how to tackle it, because i don't

https://plugins.jetbrains.com/plugin/7973-sonarlint

@AudricV
Copy link
Member Author

AudricV commented Mar 13, 2021

@XiangRongLin Thank you.

The only thing that I want to implement but I don't know how to is the support of SoundCloud downloadable files (original files uploaded by the artist, when the artist allows downloads of his/her/its track).

I know how to get the URL and add an audioStream but there are two missing informations: the bitrate and the format. Here is my initial work:

I was thinking of returning -1 when bitrate is unknown and MediaFormat.ORIGINAL_FORMAT when format is unknown.

        // If the track can be downloadable on SoundCloud's website, add the original file to the audioStreams only if there is standard streams available in order to prevent a high data consumption
        if (!audioStreams.isEmpty()) {
            if (track.getBoolean("downloadable")) {
                try {
                    final String dl_res = dl.get("https://api-v2.soundcloud.com/tracks/" + track.getInt("id") +
                            "/download?client_id=" + SoundcloudParsingHelper.clientId()).responseBody();
                    final JsonObject downloadUrlObject = JsonParser.object().from(dl_res);
                    audioStreams.add(new AudioStream(downloadUrlObject.getString("redirectUri"), MediaFormat.ORIGINAL_FORMAT, -1));
                } catch (final NullPointerException | IOException | JsonParserException ignored) {
                }
            }
        }

What do you think about what I should do?

@AudricV
Copy link
Member Author

AudricV commented Mar 13, 2021

I installed the plugin and here are the issues:
Issues

However, I don't understand how to fix it (sorry for my current programming knowledge). Can you help me please? Thank you in advance!

@XiangRongLin
Copy link
Collaborator

However, I don't understand how to fix it (sorry for my current programming knowledge). Can you help me please? Thank you in advance!

Most of the issues are not from you, so don't worry about them.

The issues should all have an explanation with examples breaking and examples adhering to the rules. As for how to fix them: I don't know.

As I already mentioned it in my previous post. Come back to them when you know more.

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.

Structure looks good. Thank you for having a look!

@AudricV
Copy link
Member Author

AudricV commented Mar 13, 2021

Thank you for your suggestions @TobiGr. I will apply it soon.

This commit tries to support SoundCloud HLS streams by parsing M3U manifests, get the last segment URL (in order to get track length) and request a segment URL equals to track's duration so it's a single URL.
…ion when fetching the HLS Manifest

If a progressive stream is present in the transcodings, it's unnecessary to show twice an MP3 128kbps stream so if this is the case, the MP3 HLS stream will be not added to the audioStreams, else it will.
This commit also catch fetching errors in HLS manifests parsing and don't add the corresponding stream if an error occurs.
…reamExtractor

This commit moved the HLS parsing task to a separate method, did little performance improvements and used final where possible in the SoundcloudStreamExtractor file.
…WithOthers test

It should be only two audio streams for track "Plays Well with Others, Ep 2: What Do an Army of Ants and an Online Encyclopedia Have in Common?" by Creative Commons (https://soundcloud.com/wearecc/plays-well-with-others-ep-2-what-do-an-army-of-ants-and-an-online-encyclopedia-have-in-common):
- one which is a progressive stream, in MP3 format with a bitrate of 128 kbps
- one which is an HLS stream, in OPUS format with a bitrate of 64 kbps.
Split the method into private methods, in order to have a better reading.
…de improvements

Apply suggestions provided in the PR and remove a redundant import.
@Stypox Stypox merged commit f71cfd4 into TeamNewPipe:dev Mar 19, 2021
@AudricV AudricV removed the request for review from XiangRongLin March 19, 2021 10:10
@AudricV AudricV deleted the snd-hls-workaround branch March 19, 2021 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request soundcloud service, https://soundcloud.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants