xyzz / vita-moonlight

NVIDIA Gamestream client for PlayStation Vita, based on moonlight-embedded
GNU General Public License v3.0
629 stars 74 forks source link

Fix pairing issue, add more FPS modes and lower resolutions #229

Closed Fewnity closed 1 year ago

Fewnity commented 1 year ago

Hello, I fixed the pairing issue, I added 24, 40 and 50 fps modes and two upscalled low screen resolutions (720x408, 858x480) for better network performance tuning. I'm sorry, I don't know how to update a thirdparty library of a repos (I tried), so if someone can do this for me, please update moonlight-common-c to commit c9426a6a71c4162e65dde8c0c71a25f1dbca46ba https://github.com/moonlight-stream/moonlight-common-c/tree/c9426a6a71c4162e65dde8c0c71a25f1dbca46ba Thanks.

d3m3vilurr commented 1 year ago

thank you for your contribution. before full reviewing, can you keep brace style, indentation, and whitespaces of the comments? current patchsets have a lot of meanless changes and it makes harder reviewing.

and also if you referenced something from upstream(moonlight-embedded), please keep their commit

janfrederick2 commented 1 year ago

Fixed the parining and connection issues for me

KatanaMoro commented 1 year ago

hello, how can i download this version to try it out? can i find installation file here?

HealsCodes commented 1 year ago

hello, how can i download this version to try it out? can i find installation file here?

Here you go, as it happens I built the code from this PR myself this morning because I wanted to give it a try: moonlight-0.9.2a.vpk.zip

(it'll show as 0.9.2 on the vita dashboard but moonlight itself will read '0.9.2a' so you can tell it's installed)

KatanaMoro commented 1 year ago

hello, how can i download this version to try it out? can i find installation file here?

Here you go, as it happens I built the code from this PR myself this morning because I wanted to give it a try: moonlight-0.9.2a.vpk.zip

(it'll show as 0.9.2 on the vita dashboard but moonlight itself will read '0.9.2a' so you can tell it's installed)

thank you very much <3

KatanaMoro commented 1 year ago

hello, ive been testing 0.9.2a for the past few days, it connects and runs each time flawlessly on the latest geforce driver installed. many thanks!!!

rhobiusvoid commented 1 year ago

Tried this version out. Whatever you did to the H264 code greatly increased stream quality and responsiveness (somehow it also reduced input lag drastically). Instead of stuttering it just slightly drops quality and then returns to normal, which heavily mitigates the audio desync bug that occurs when a heavy stutter happens. Unfortunately, I don't know how to help locate the source of the bug, but it's still there.

articnova commented 1 year ago

I haven't put too many hours into this version but I can confirm it works much better than the main branch. It definitely fixed my issue with the handshake & pairing issue I had when trying to pair with sunshine on my steam deck.

KatanaMoro commented 1 year ago

Hello, is there a way to download 0.9.3 .vpk file? I would be happy to try latest version <3 thank you

HealsCodes commented 1 year ago

@Fewnity - I don't mind to build in-progress vpk snapshots for people following this issue that wish to test your changes.

And If you want to update the submodule reference and add that to your PR the steps are:

git submodule update
cd third_party/moonlight-common-c
git checkout c9426a6a
cd -
git add third_party/moonlight-common-c
git commit -m 'Bump moonlight-common-c to c9426a6a' third_party/moonlight-common-c

Here's also a small patch that improves the build versioning and includes the git commit ref as well. (you only set it once in CMakeLists.txt now and it updates the live-area template as well)

0001-Include-git-commit-hash-in-build-versions.patch

HealsCodes commented 1 year ago

Hello, is there a way to download 0.9.3 .vpk file? I would be happy to try latest version <3 thank you

Here, please note that the high-quality audio option does not yet function (as the original author wrote in their commit message "does not compile") so I disabled it.

moonlight_v0.9.3.6ba382b.vpk.zip

KatanaMoro commented 1 year ago

Hello, is there a way to download 0.9.3 .vpk file? I would be happy to try latest version <3 thank you

Here, please note that the high-quality audio option does not yet function (as the original author wrote in their commit message "does not compile") so I disabled it.

moonlight_v0.9.3.6ba382b.vpk.zip

Noted. Thank you very much for uploading .vpk file. And many thanks to @Fewnity for doing amazing work, keeping vita-moonlight alive!!!

Fewnity commented 1 year ago

@Fewnity - I don't mind to build in-progress vpk snapshots for people following this issue that wish to test your changes.

And If you want to update the submodule reference and add that to your PR the steps are: ... Here's also a small patch that improves the build versioning and includes the git commit ref as well. (you only set it once in CMakeLists.txt now and it updates the live-area template as well)

Thank you for the build and for the tips!

Fewnity commented 1 year ago

Here is the .vpk with the high audio quality option if someone wants to test : moonlight 0.9.3.zip

For the pullrequest I will make a new one when I will have the time. It will be easier for me for the whitespaces and submodules problem.

d3m3vilurr commented 1 year ago

to @Fewnity first, you have to refer the original commit of the patch. for example a9092b1f7 would contains https://github.com/moonlight-stream/moonlight-embedded/commit/0325a3b88c5 this patchset lost contributing informations of upstream branches

second, probably already you know, keep code style is really important than your thinking. this project's coding style is not mine. I'm really prefer 4space softtab. but it's not. this project was forked from moonlight-embedded. so this project also can get a patch from upstream. broken coding style is harmful than fixing bugs.

third, each patches should be able to compile. I'm tried to apply your patch into main codeset. btw first commit (3c8fc73822380) is requires third commit (a9092b1f74d8) or mores. imo, first commit's main thing is just adding two resolutions 720x408 , 858x480 and removing 1280x540 do you expect this from first commit codeset? :(

forth, just make the one pr for one feature. multiple patch makes harder revewing. you want to add new resolutions, fixing pairing and now you want to improve audio quality. really thank you and I'm glad to your contribution. but sorry I can't review this.

I'll open this pr just couple of more days then close the thread. please check & reflect my review thing.

if you are new to use the git, please tell me. I can share multiple things


to users. I already said, I can't merge this pr until resolving problems. and please don't request get the built packages in this thread. if you really want built package, please share it in discord or email.

it doesn't help to @Fewnity. because if devs think this has good progress, he would be focus to make more things than fixing problems. it just makes more delay to merge.

HealsCodes commented 1 year ago

OP could have split this into multiple PRs, I agree, however at the same time you can't blame anyone presently trying to use vita-moonlight for asking for a vpkg of this PR as there has been little to no visible activity otherwise for the past months.

0.9.1 has major issues streaming once paired and 0.9.2 can't pair anymore at all so users are forced to hop back and forward between the two version only to try and play at a sub-par quality. Is it really so weird that those same users find this issue with a modified version that actually works as expected, solves their problems and provides better stability than what is currently "last release" to want to use this?

This PR has made good progress, even if it didn't follow the best practices for a git PR - that's not something the dev would think but rather it's confirmed by multiple people that compiled and tested this on their actual devices. Yes, the code-style isn't consistent but that could be fixed. The rest could be re-formatted or broken down into smaller PRs but the fact that OP fixed issues which were open in this project for months and some for even longer can't be talked away so it would be the wise move to work with OP on getting this up to shape to be merged instead of announcing you're going to close it.

The CONTRIBUTION guides also really need a revamp, "the same coding style as the rest of this project" can't be applied to a project that has about 3 different coding styles in the current code-base already. A clang-tidy definition or at least .editorconfig would benefit this greatly.

d3m3vilurr commented 1 year ago

OP could have split this into multiple PRs, I agree, however at the same time you can't blame anyone presently trying to use vita-moonlight for asking for a vpkg of this PR as there has been little to no visible activity otherwise for the past months.

I didn't blame about this. I'm just request to change this for merging & reviewing.

you know? my first comment is same day as made day of this pr. I'm waiting a month for this review. now, he pushed another changes instead reflecting the comment.

0.9.1 has major issues streaming once paired and 0.9.2 can't pair anymore at all so users are forced to hop back and forward between the two version only to try and play at a sub-par quality. Is it really so weird that those same users find this issue with a modified version that actually works as expected, solves their problems and provides better stability than what is currently "last release" to want to use this?

well... I don't think so. sometimes users said ah this version resolved a problem. but at that time code just updated base lib. it means nothing changed.

maybe this patch contains real patch set. but i can't review this pr right now. this patchset is not a ready to review so idk this patch is contains actual patch things or not.

This PR has made good progress, even if it didn't follow the best practices for a git PR - that's not something the dev would think but rather it's confirmed by multiple people that compiled and tested this on their actual devices. Yes, the code-style isn't consistent but that could be fixed. The rest could be re-formatted or broken down into smaller PRs but the fact that OP fixed issues which were open in this project for months and some for even longer can't be talked away so it would be the wise move to work with OP on getting this up to shape to be merged instead of announcing you're going to close it.

yes. progress is progress. so I told really thank you and I'm glad to your contribution. and also I have a reason why I will close this. he also told, he'll make a new pr For the pullrequest I will make a new one when I will have the time. It will be easier for me for the whitespaces and submodules problem.

if you think I have to help this thing. I already tried this before wrote a comment.

The CONTRIBUTION guides also really need a revamp, "the same coding style as the rest of this project" can't be applied to a project that has about 3 different coding styles in the current code-base already. A clang-tidy definition or at least .editorconfig would benefit this greatly.

and yes, updating contribution guide, introduce linting and share the editor configure set might be reducing the problem. but you also have to know this project is mixed with 3 different projects. and it doesn't mean contributor can do smash the old code. if you cannot detect the style by project, it means, you have to follow it by file instead project.


ok guys. you everyone can blame me that I didn't care the project within several month. it's true. but it means nobody contributed during that time. so I'm also one who want to review this pr and merge this pr.

and I'm sorry I can't find more nicely word in english for explain the stage of this pr. so, I only can say this pr is not ready to go to next step. and I just say this thread is not good place to share the built package if you really want to merge this code into mainline.

Fewnity commented 1 year ago

Thank you @d3m3vilurr for your message, you were totally right to list everything that went wrong! I will make sure to apply all this and ask for help if needed. I had not ignored your first message, I wanted to apply it on the next pullrequests. Yes I'm a beginner on github, I'm not used to make pullrequests to help people. I only use github for my personal projects so I do very basic stuff with it. I didn't expect this kind of problem.

I made new commits because I wanted to make more things, create new branches and start from scratch just by copying stuff I had done to make new pr. I didn't know that the commits would add themselves to the current pullrequest. At least now I know a bit better how to make better pullrequests, I'll try not to make these mistakes anymore. I'm taking time for the pullrequest because I'm a student and I don't have a lot of time to do it. Sorry!

d3m3vilurr commented 1 year ago

@Fewnity you don't need to say sorry. we'll be going better. thank you for your contribution.

simple tip; use git add -p than git add :)