hyperion-project / hyperion.ng

The successor to Hyperion aka Hyperion Next Generation
https://hyperion-project.org/
MIT License
2.95k stars 371 forks source link

RGB24 and BGR24 cleanup #1749

Closed Lord-Grey closed 1 month ago

Lord-Grey commented 1 month ago

Summary

What kind of change does this PR introduce? (check at least one)

If changing the UI of web configuration, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

If yes, please describe the impact and migration path for existing setups:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

PLEASE DON'T FORGET TO ADD YOUR CHANGES TO CHANGELOG.MD

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

Thinner77 commented 1 month ago

Great! Can you also complete support for BGR16 and BGR32? Pixelformat.h and ImageResampler already know about these formats, so it's only adding some more "if"s in V4L2 gabber πŸ˜„. wbr

PS: Looks like MF doesn't know about BGR-Formats: https://learn.microsoft.com/de-de/windows/win32/medfound/video-subtype-guids

Lord-Grey commented 1 month ago

Can you also complete support for BGR16 and BGR32?

Done.

It stepped away from BGR16, as I do not think a grey-scale format is too relevant and ImageResampler does not have a implementation yet.

Windows MF is now fixed, too. Was a good opportunity to get my Windows dev environment up and running again πŸ˜€

Thinner77 commented 1 month ago

Hi @Lord-Grey,

you are great, and fast πŸ˜„ . But BGR16 isn't a grayscale format and ImageResampler already knows it, i'm confused: https://github.com/hyperion-project/hyperion.ng/blob/76fff98f5cc1f20c2c3e059bf9ded9ee73d564e1/libsrc/utils/ImageResampler.cpp#L120

wbr

Lord-Grey commented 1 month ago

ImageResampler already knows it

I am sorry, I confused myself. I was referring to RGB16… :( I will add it

Thanks for a 2nd pair of eyes reviewing!

Thinner77 commented 1 month ago

Hi,

the other option would be to remove support for BGR16 completely because it hasn't been usable so far and nobody seems to have needed it. I don't know, it's your decision. ;-)

wbr

Lord-Grey commented 1 month ago

the other option would be to remove support for BGR16 completely because it hasn't been usable so far and nobody seems to have needed it.

I do not know if there are side effects on other grabbers.