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

Extract metadata for youtube, soundcloud & mediaccc #306

Merged
merged 3 commits into from
Mar 27, 2021

Conversation

B0pol
Copy link
Member

@B0pol B0pol commented Apr 9, 2020

  • 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.

Extraction for metadata which were added not so long ago (now), but only for PeerTube

fixes #393

@B0pol B0pol requested a review from wb9688 April 17, 2020 11:27
@B0pol B0pol added multiservice Issues related to multiple services enhancement New feature or request labels Apr 20, 2020
@wb9688 wb9688 added this to the 0.19.4 milestone Apr 20, 2020
Copy link
Contributor

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

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

According to https://github.com/voc/voctoweb/blob/master/app/admin/event.rb#L115, media.ccc.de does actually use ISO 639-2. Since they have e.g. deu, it means they're using ISO 639-2/T.

@B0pol B0pol force-pushed the metadata branch 2 times, most recently from 5bb7234 to de7515d Compare April 22, 2020 09:47
@Stypox
Copy link
Member

Stypox commented May 7, 2020

Why is the license function called "getLicence" and not "getLicense"? I think it should be renamed

@B0pol
Copy link
Member Author

B0pol commented May 7, 2020

Why is the license function called "getLicence" and not "getLicense"? I think it should be renamed

Because I'm learning UK english, not American version. In contribution guidelines, it's not stated to use american english.

@wb9688
Copy link
Contributor

wb9688 commented May 7, 2020

@Stypox: I generally prefer US English as well for programming, as that's basically the default. I learnt UK English at school though.

@Stypox
Copy link
Member

Stypox commented May 7, 2020

Oh, didn't know licence was a word, I only ever came across license. Then don't consider my complaint ;-)

@Stypox
Copy link
Member

Stypox commented May 16, 2020

While you are at this, could you move all of the metadata methods in the base StreamExtractor.java above the "Helper" section? It is confusing to find a helper function sorrounded by interface functions

@Stypox Stypox modified the milestones: 0.19.4, 0.19.5 May 29, 2020
@wb9688
Copy link
Contributor

wb9688 commented Jun 27, 2020

@B0pol: Could you rebase and fix the issue for YouTube and SoundCloud? Don't bother making it an enum for now.

@wb9688 wb9688 modified the milestones: 0.19.6, 0.20.0 Jul 2, 2020
@B0pol
Copy link
Member Author

B0pol commented Jul 15, 2020

@wb9688 done

Copy link
Contributor

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

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

Could you clean up the commits?

@B0pol B0pol force-pushed the metadata branch 9 times, most recently from b0434ec to eb8411f Compare February 20, 2021 11:26
@Stypox
Copy link
Member

Stypox commented Mar 18, 2021

@B0pol could you rebase this one last time? Then it should be absolutely merged 🙈

@Stypox Stypox self-assigned this Mar 18, 2021
@TobiGr
Copy link
Contributor

TobiGr commented Mar 18, 2021

Yes, that just needs to be rebased. I reviewed the code before, but forgot to approve it

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 rebased; the related tests run well on my end; I think this can be merged. @TobiGr

I also included a fix for SoundCloudStreamExtractorTest, which contained two test-initializing functions which were nor static neither marked as @BeforeClass, but instead were marked as @Test. This caused them to be run at a random time, so sometimes the extractor would be initialized beforehand, other times not, causing NullPointerExceptions. Furthermore, such @Tests were expected to throw GeographicRestrictionException and SoundCloudGoPlusContentException inside fetchPage(), so I added a try-catch that ignores those exceptions. You may ask how the tests can pass if fetchPage() fails: they pass since the exceptions are thrown after everything has been extracted (see these lines). I don't know why an exception is thrown if everything can be extracted without issues (@TiA4f8R could you investigate?), but this is unrelated to the scope of this PR.

@AudricV
Copy link
Member

AudricV commented Mar 19, 2021

I don't know why an exception is thrown if everything can be extracted without issues (@TiA4f8R could you investigate?), but this is unrelated to the scope of this PR.

For what tests/streams/tracks? I don't understand.

@Stypox
Copy link
Member

Stypox commented Mar 19, 2021

@TiA4f8R take a look at the changes in SoundCloudStreamExtractorTest

@AudricV
Copy link
Member

AudricV commented Mar 19, 2021

It throws an exception because no streams are available for these tracks.

@Stypox
Copy link
Member

Stypox commented Mar 19, 2021

Then the test for video streams should fail, but it doesn't (unless it is suppressed, I didn't make sure that was the case)

@Stypox Stypox merged commit b4dee6d into TeamNewPipe:dev Mar 27, 2021
@B0pol B0pol deleted the metadata branch April 5, 2021 18: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 multiservice Issues related to multiple services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is this possible to fetch/extract youtube video tags? streamInfo.getCategory() is empty.
8 participants