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

NV12 UI #89

Closed
wants to merge 23 commits into from
Closed

NV12 UI #89

wants to merge 23 commits into from

Conversation

satgit62
Copy link

@satgit62 satgit62 commented Dec 3, 2024

NV12 settings selection added

In the new hyperion-webos, the backends have been extended to include nv12 video format, so that NV12/YUV can also be passed on to the Hyperion or HyperHDR instead of RGB.
As a result, I have added a NV12 selection checkbox to the UI. In addition, hyperion-webos submodules would have to be linked to the latest https://github.com/webosbrew/hyperion-webos.

PicCap_nv12

PicCap hyperion-webos submodule

@Dak0r
Copy link

Dak0r commented Dec 26, 2024

Hey, code looks good to me. I built it locally and seems to work, with the correct version of the webos backend.
Any reason, why you didn't include the submodule update in this branch? In my opinion, it's a requirement to merge this.

I am not sure, if we want to mention somewhere that HyperHDR v21 is required for this, as it won't work with the current v20 stable release?

@@ -298,6 +299,7 @@ window.serviceResetSettings = () => {

vsync: true,
autostart: false,
nv12: true,
Copy link

@Dak0r Dak0r Dec 26, 2024

Choose a reason for hiding this comment

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

Because the hyperhdr stable release (v20) does not support it, yet, defaulting to false would be a more fitting default for now, imo

Copy link
Owner

@TBSniller TBSniller Dec 29, 2024

Choose a reason for hiding this comment

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

Would agree on that. In backend service it also defaults to false: https://github.com/webosbrew/hyperion-webos/blob/main/src/settings.c#L19

@satgit62
Copy link
Author

Hey, code looks good to me. I built it locally and seems to work, with the correct version of the webos backend. Any reason, why you didn't include the submodule update in this branch? In my opinion, it's a requirement to merge this.

I am not sure, if we want to mention somewhere that HyperHDR v21 is required for this, as it won't work with the current v20 stable release?

Hello,
I originally assigned the submodules from @sundermann, but there were errors when compiling, and so I removed them again, hoping @TBSniller would comment.
See: https://github.com/satgit62/piccap/tree/nv12
In the end, I swapped the backends manually.

@Dak0r
Copy link

Dak0r commented Dec 26, 2024

I was able to build your piccap branch with the submodule updated to the latest main branch commit from sundermann, without problems. Which error did you get?

(Note: I had to update cmake in my WSL to >=3.24, because hyperion-webos is now using DOWNLOAD_EXTRACT_TIMESTAMP in its CMakeFile)

@satgit62
Copy link
Author

I was able to build your piccap branch with the submodule updated to the latest main branch commit from sundermann, without problems. Which error did you get?

(Note: I had to update cmake in my WSL to >=3.24, because hyperion-webos is now using DOWNLOAD_EXTRACT_TIMESTAMP in its CMakeFile)

Yes, if you build on PC it works with cmake update good (hint) but in GitHub when pushing an action the IPK is not built. See: https://github.com/satgit62/piccap/actions/runs/12156806035/job/33901240141

README.md Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

The readme looks a bit strange with this comment.
Maybe move it with its explaination here https://github.com/TBSniller/piccap?tab=readme-ov-file#advanced-settings ?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm sorry I was quite inactive and haven't followed the new changes.
How big is this new feature? Does it make sense to be on the main settings page, or has it a better place in a seperate section on the advanced settings page?
I think it would be the best to leave the main settings page as simple as possible

Copy link
Owner

Choose a reason for hiding this comment

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

Would also suggest to rename the label to uppercase NV12 instead of nv12.

package.json Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

If we bump hyperion-webos submodule we could also go to 0.5.0

@TBSniller
Copy link
Owner

Thank you very much for contribution!
As the new hyperion-webos version is needed for NV12 support, I think there is also a need to bump the submodule to latest version

@satgit62
Copy link
Author

Thank you very much for contribution! As the new hyperion-webos version is needed for NV12 support, I think there is also a need to bump the submodule to latest version

Hi @TBSniller,
yes, I also think it is very good to update the submodules and update the PicCap version to 5.0. Thanks a lot for that.

@satgit62 satgit62 changed the title Nv12 UI nv12 UI Dec 29, 2024
@satgit62 satgit62 changed the title nv12 UI NV12 UI Dec 29, 2024
@satgit62 satgit62 closed this Dec 29, 2024
@satgit62 satgit62 deleted the nv12-UI branch December 29, 2024 17:45
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.

3 participants