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

[YouTube] Extract trends from A/B tested "Videos" tab and fix extraction of trends name from A/B tested new title design #1045

Merged
merged 3 commits into from
May 19, 2023

Conversation

Theta-Dev
Copy link
Contributor

@Theta-Dev Theta-Dev commented Apr 2, 2023

YouTube is A/B testing a new layout for the trending page, where they moved the list of trending videos into a separate tab.

Issue reported on Reddit: https://www.reddit.com/r/NewPipe/comments/127tyh8/no_v%C3%ADdeos_is_it_a_bug/

Pictures here: https://code.thetadev.de/ThetaDev/rustypipe/src/branch/main/notes/AB_Tests.md#4-video-tab-on-the-trending-page

I have changed the extractor to fetch this new tab. If the new layout is not enabled, YT simple returns the Now page like before.

The parser had to be adjusted, because we cannot filter out all shelves with titles any more (the shelves in the Videos tab have titles).

Fixes #1046.

@AudricV AudricV changed the title Extract YouTube trends from new "Videos" tab [YouTube] Extract trends from new "Videos" tab Apr 2, 2023
@AudricV AudricV added bug Issue is related to a bug youtube service, https://www.youtube.com/ labels Apr 2, 2023
Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

Except the following comments, your changes look good to me.

However, I cannot reproduce the A/B test anymore. Do you think YouTube reverted their test or stopped it? This may lead to the removal of this parameter, and may break trends extraction instead of fixing it in the future if using these parameters return a bad response (unlikely to happen, as wrong parameters seem to be ignored).

@Theta-Dev
Copy link
Contributor Author

I cannot get the A/B test with the videos tab any more, too. Maybe YouTube did scrap this idea, maybe it's just the end of the test run and they are evaluating the results.

I would keep the change for now, if the new layout does not appear again within half a year we could take it out again for the sake of simplicity.

Requesting non-existant tabs will simply return the default tab (it's the same thing for channel tabs), so I dont see any issues.

@FireMasterK
Copy link
Member

I'm seeing an increased error rate on Piped when fetching the trending page since the 27th of April, maybe there's a new A/B test? Just speculating

image

@Theta-Dev
Copy link
Contributor Author

Theta-Dev commented May 1, 2023

are you using this PR or the regular version of the Extractor?

Edit: oh, the error is about the title ("Trending") not being extracted. So it looks like a new header layout or something like that. Btw, why do we throw an exception when we fail to extract a text that is always the same?

Edit2: Here we go: they changed the header renderer. Should be an easy fix.

https://code.thetadev.de/ThetaDev/rustypipe/src/branch/main/notes/AB_Tests.md#5-page-header-renderer-on-the-trending-page

@AudricV
Copy link
Member

AudricV commented May 1, 2023

Btw, why do we throw an exception when we fail to extract a text that is always the same?

For localization purposes I think. When support of YouTube extraction in other content languages than English will be added again, an incorrect information would be returned for these non-English languages.

@Theta-Dev
Copy link
Contributor Author

Theta-Dev commented May 1, 2023

I see. Falling back to "Trending" if the title could not be extracted would not be bad either, so I changed it.

When and why was the extraction of other languages removed, though?

@AudricV AudricV changed the title [YouTube] Extract trends from new "Videos" tab [YouTube] Extract trends from new "Videos" tab and fix extraction of trends name May 2, 2023
@AudricV AudricV force-pushed the fix/trending-video-tab branch from 35f0ee4 to 3673d4a Compare May 3, 2023 19:20
@AudricV
Copy link
Member

AudricV commented May 3, 2023

Falling back to "Trending" if the title could not be extracted would not be bad either, so I changed it.

That's a very bad idea to return an English-only string, due to the reasons explained above: if YouTube changes their Trending title and how it is accessed, the extractor would return a value which would always pass the test. That's why I reverted this change.

When and why was the extraction of other languages removed, though?

It was removed in #158, due to parsing issues, as several parts of the YouTube extractor relies on English strings (age-restricted and paid content checks, accessibility data parsing, ...). I worked a few months ago on trying to restoring this support and I will try to resume my work after we have finished with major features on the extractor.

@AudricV AudricV changed the title [YouTube] Extract trends from new "Videos" tab and fix extraction of trends name [YouTube] Extract trends from A/B tested "Videos" tab and fix extraction of trends name May 19, 2023
@AudricV AudricV changed the title [YouTube] Extract trends from A/B tested "Videos" tab and fix extraction of trends name [YouTube] Extract trends from A/B tested "Videos" tab and fix extraction of trends name from A/B tested new title design May 19, 2023
@AudricV AudricV merged commit 533121f into TeamNewPipe:dev May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug youtube service, https://www.youtube.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[YouTube] Trends are not returned sometimes
4 participants