-
-
Notifications
You must be signed in to change notification settings - Fork 462
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
feat(StreamsExtractor): generate PoToken #7069
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as spam.
This comment was marked as spam.
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.
|
The crash log is a temporary DNS issue on your side, that's not LibreTube related. |
3a637c3
to
98a0bf1
Compare
A source error still occurs when starting a video in HLS.XRecorder_20250205_01.mp4 |
c0c210f
to
817521a
Compare
A few questions:
|
This issues seem to be NewPipeExtractor bug itself, since HLS don't need
Uhm u should port it also |
With the current code the purpose of I suggest to do one of these to make it more obvious it is about po tokens:
NewPipe did the first option here and put everything po token related into a sub package: |
It shouldn't be completely removed because it might still work with some private Piped instances, however disabling it by default is possible.
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. |
5b9b297
to
6ecad37
Compare
HLS does not work by default, leading to a bad user expierence, as videos won't play, disable it. Ref: libre-tube#7069 (comment)
HLS does not work by default, leading to a bad user expierence, as videos won't play, disable it. Ref: libre-tube#7069 (comment)
Disabled and ported.
Sure, no pressure. I would also like to clean up the commits before merging. Good luck with your exams. |
app/src/main/java/com/github/libretube/api/local/JavaScriptUtil.kt
Outdated
Show resolved
Hide resolved
There should be some better error handling. |
There is a NewPipe PR which ports po token code to coroutines Maybe this could be used as a base. |
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.
|
HLS does not work by default, leading to a bad user expierence, as videos won't play, disable it. Ref: libre-tube#7069 (comment)
app/src/main/java/com/github/libretube/api/local/PoTokenWebView.kt
Outdated
Show resolved
Hide resolved
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
6ecad37
to
34de9f9
Compare
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 } |
There was a problem hiding this comment.
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
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 |
There was a problem hiding this comment.
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
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:
PoTokenGenerator
interface. As there is only one implementation right now, this is skippedminSdk
in BREAKING CHANGE: bump minSdk to 26 #7047, I'm hoping we can get away withoutIt 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