Skip to content

Conversation

@FineFindus
Copy link
Collaborator

@FineFindus FineFindus commented Feb 3, 2025

Implements support for locally generating PoTokens using the device web view. This is a direct port of
TeamNewPipe/NewPipe#11955 to native Kotlin.
However, there are some notable differences:

  • NewPipe use an additional PoTokenGenerator interface. As there is only one implementation right now, this is skipped
  • NewPipe handles broken web views on older devices. Since we're increasing minSdk in BREAKING CHANGE: bump minSdk to 26 #7047, I'm hoping we can get away without

It is marked as a draft, since while it does generate valid PoTokens, playback does not work (Source error: open failed: ENOENT; help is appreciated).

Closes: #7065

@Figim

This comment was marked as spam.

@FineFindus
Copy link
Collaborator Author

Please don't add unnecessary comments. Linking an issue to be closed is fine, but the rest just adds noise. I'm already well aware of the NewPipe PR (see description above).

@Figim
Copy link
Contributor

Figim commented Feb 4, 2025

Please don't add unnecessary comments. Linking an issue to be closed is fine, but the rest just adds noise. I'm already well aware of the NewPipe PR (see description above).

Sorry.

  1. "Source Error" only occurs if "HLS" is enabled. DASH works great. tested

  2. Live streams also play smoothly on HLS

@Bnyro
Copy link
Member

Bnyro commented Feb 4, 2025

The crash log is a temporary DNS issue on your side, that's not LibreTube related.

@Figim
Copy link
Contributor

Figim commented Feb 4, 2025

A source error still occurs when starting a video in HLS.

XRecorder_20250205_01.mp4

@FineFindus
Copy link
Collaborator Author

FineFindus commented Feb 5, 2025

A few questions:

  • Since HLS does not work with potokens either, should it be removed/disabled?
  • JavaScriptUtil.kt uses nanojson, should it be ported as well? I left it for now, as it would make it easier to follow future changes. There isn't any noticable impact on filesize, since it is already included in NewPipeExtractor.

@YT-Advanced
Copy link

  • Since HLS does not work with potokens either, should it be removed/disabled?

This issues seem to be NewPipeExtractor bug itself, since HLS don't need pot but it still be appended in streaming url

  • JavaScriptUtil.kt uses nanojson, should it be ported as well? I left it for now, as it would make it easier to follow future changes.

Uhm u should port it also

@gechoto
Copy link

gechoto commented Feb 5, 2025

With the current code the purpose of JavaScriptUtil.kt is not perfectly clear at first look and it might take a while to notice that this is for po tokens.
The class and function names are pretty generic.
For example in the first function it says something about some "challenge data". Yeah cool, which challenge data? Someone new to the project won't know.

I suggest to do one of these to make it more obvious it is about po tokens:

  • Move JavaScriptUtil.kt and PoTokenWebView.kt to a sub-package named potoken
  • Rename JavaScriptUtil.kt to something like PoTokenJavaScriptUtil.kt
  • Rename the functions and extend the documentation in JavaScriptUtil.kt to make it clear this is for po tokens

NewPipe did the first option here and put everything po token related into a sub package:
https://github.com/TeamNewPipe/NewPipe/tree/dev/app/src/main/java/org/schabi/newpipe/util/potoken

@Bnyro
Copy link
Member

Bnyro commented Feb 15, 2025

Since HLS does not work with potokens either, should it be removed/disabled?

It shouldn't be completely removed because it might still work with some private Piped instances, however disabling it by default is possible.

JavaScriptUtil.kt uses nanojson, should it be ported as well? I left it for now, as it would make it easier to follow future changes.

Yes, I would like to keep the used libraries consistent across the code. Since the NanoJSON code part is only very small and the code differs already due to the fact that it's ported to Kotlin, I think it's reasonable to port it.

I don't have much time to look into the problems you mentioned at the moment (and the next few weeks) unfortunately due to tons of exams at university. I'll see if I can find some time to allocate for reviewing soon, but no guarantees.

FineFindus added a commit to FineFindus/LibreTube that referenced this pull request Feb 15, 2025
HLS does not work by default, leading to a bad user expierence, as
videos won't play, disable it.

Ref: libre-tube#7069 (comment)
FineFindus added a commit to FineFindus/LibreTube that referenced this pull request Feb 15, 2025
HLS does not work by default, leading to a bad user expierence, as
videos won't play, disable it.

Ref: libre-tube#7069 (comment)
@FineFindus
Copy link
Collaborator Author

Disabled and ported.

I don't have much time to look into the problems you mentioned at the moment (and the next few weeks) unfortunately due to tons of exams at university. I'll see if I can find some time to allocate for reviewing soon, but no guarantees.

Sure, no pressure. I would also like to clean up the commits before merging. Good luck with your exams.

@gechoto
Copy link

gechoto commented Feb 17, 2025

There should be some better error handling.
Currently if something in parseChallengeData or parseIntegrityTokenData fails the app crashes.

@gechoto
Copy link

gechoto commented Feb 17, 2025

There is a NewPipe PR which ports po token code to coroutines

TeamNewPipe/NewPipe#12028

Maybe this could be used as a base.
With the code in the linked PR the app does not crash.
It does handle exceptions better.

@FineFindus
Copy link
Collaborator Author

Currently if something in parseChallengeData or parseIntegrityTokenData fails the app crashes.

Please attach a crash log/reproducible case and don't post vague comments about 'something' crashing.

@gechoto
Copy link

gechoto commented Feb 17, 2025

Currently if something in parseChallengeData or parseIntegrityTokenData fails the app crashes.

Please attach a crash log/reproducible case and don't post vague comments about 'something' crashing.

One specific crash log won't be useful. It crashes on any error.
Just do something obviously wrong in parseChallengeData, could be corrupted challenge data or for example try to access something which does not exist:

val messageId = challengeData[10].jsonPrimitive.content

FineFindus added a commit to FineFindus/LibreTube that referenced this pull request Feb 18, 2025
HLS does not work by default, leading to a bad user expierence, as
videos won't play, disable it.

Ref: libre-tube#7069 (comment)
@Figim Figim mentioned this pull request Mar 20, 2025
3 tasks
@FineFindus FineFindus marked this pull request as ready for review March 23, 2025 21:22
Comment on lines +35 to +43
val privateDoNotAccessOrElseSafeScriptWrappedValue = challengeData[1]
.takeIf { it !is JsonNull }
?.jsonArray
?.find { it.jsonPrimitive.isString }

val privateDoNotAccessOrElseTrustedResourceUrlWrappedValue = challengeData[2]
.takeIf { it !is JsonNull }
?.jsonArray
?.find { it.jsonPrimitive.isString }
Copy link

@gechoto gechoto Mar 25, 2025

Choose a reason for hiding this comment

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

since you access challengeData null-safe now
ae23ba3

this could also be applied to these two

but I don't think this is a perfect solution
because there are still other things which can go wrong and make the app crash

accessing something on challengeData out of bounds was only one example

there should be better error handling in general instead
maybe this whole parseChallengeData should be ran in try/catch

there is this NewPipe PR now which seems to handle errors better:
TeamNewPipe/NewPipe#12028

Comment on lines 28 to 32
val messageId = challengeData.getOrNull(0)?.jsonPrimitive?.content
val interpreterHash = challengeData.getOrNull(3)?.jsonPrimitive?.content
val program = challengeData.getOrNull(4)?.jsonPrimitive?.content
val globalName = challengeData.getOrNull(5)?.jsonPrimitive?.content
val clientExperimentsStateBlob = challengeData.getOrNull(7)?.jsonPrimitive?.content
Copy link

Choose a reason for hiding this comment

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

idk maybe it is better to not use getOrNull here and instead log an exception

problem is that currently LibreTube crashes instead of logging the exception and showing the user an error message
all of this stuff should be wrapped in an error handler
not just these few lines

from my experience integrating po token support in OuterTune error handling works better with the changes from this PR:
TeamNewPipe/NewPipe#12028

@Bnyro
Copy link
Member

Bnyro commented Apr 2, 2025

First of all, thank you for all your work spent into this PR! Everything works very nicely, the only downside is the increased loading time (as you mentioned), but that's certainly worth it.

About my review:

  • I've added/pushed some small changes to abuse the Kotlin syntax, please feel free to just squash them if they're okay
  • I'm a bit worried about the GOOGLE_API_KEY and the SECRET_KEY used here? I assume they are from NewPipe? If we hardcode them, this might make it easy for YouTube to fingerprint our client because they are publicly visible and YouTube could just block all request using them. That's nothing blocking this PR, it's just a side note that we maybe could consider making a build option for providing these keys in the future (if obtaining them is documented somewhere).
  • I've left some other small nitpicks/questions, feel free to answer or just ignore/resolve them if they're irrelevant

With that being mentioned, I think that there's nothing in the way from merging this and creating a release soon after (if I find time for it).

@Bnyro
Copy link
Member

Bnyro commented Apr 2, 2025

@gechoto I agree on you that we could still improve the errror handling here in the future, but that's nothing important enough to block this PR and could still be done with follow-up pull requests.

@FineFindus
Copy link
Collaborator Author

I'm a bit worried about the GOOGLE_API_KEY and the SECRET_KEY used here? I assume they are from NewPipe?

They are, but I doubt there is a way without using them, since they're required for the request. They have been in use since at least 2023 and are included in dozens of repos at this point, so I doubt there is any use in hiding them now :)

@FineFindus FineFindus requested a review from Bnyro April 2, 2025 18:57
Implements support for locally generating PoTokens using the device
webview. This is a direct port of
TeamNewPipe/NewPipe#11955 to native Kotlin.

Closes: libre-tube#7065
Copy link
Member

@Bnyro Bnyro 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 looking into the review and rebasing!

@Bnyro Bnyro merged commit d562df3 into libre-tube:master Apr 2, 2025
2 of 3 checks passed
@FineFindus FineFindus deleted the feat/potoken branch April 2, 2025 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

High priority: Support PR like NewPipe to use Extractor PoToken

5 participants