search

hyperion-project / hyperion.ng

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

Fix flipmode for BGR24 special handling #1739

Closed Thinner77 closed 4 months ago

Thinner77 commented 4 months ago

Summary

If pixelfomat BGR24 is selected in V4L2 grabber, the image is flipped horizontally (with flipmode set to None). This PR will fix that. However, in my opinion this special treatment is not necessary for BGR24 and can be removed completely. But I'm not 100% sure, maybe a developer can take a look.

wbr

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:

Lord-Grey commented 4 months ago

Hi @Thinner77 I am also wondering why there is a dedicated different behavior on flipmode that is format related. I would expect that the following if is not here.

Just guessing… it could be there because of the Windows MediaFoundation Grabbing. Windows often starts counting images from bottom not from top and the flipping code seems to correct this. But then this might require fixing at the MF grabber and not at the Encoder code.

@Paulchen-Panther Do you remember why it was coded that way?

Thinner77 commented 4 months ago

Hi,

i've spend last few hours setting up an build environment on Windows 10. It did work and hyperion is running now. I've tested internal camera of my laptop and a USB-Webcam, both supporting NV12, YUYV and MJPEG pixelformat, unfortunately not BGR24 to prove special handling is not needed. But all tests with the supported pixelformats will give a correct preview image with current master-branch, so BGR24 special handling is not triggered. Even if MF grabber may be tricky and image handling on Windows may be different, i can't imagine it depends on pixelformat.

Any news from @Paulchen-Panther ?

wbr

Thinner77 commented 4 months ago

Hi,

any news on this? I'm 100% sure we can remove BGR24 special handling. Do you expect anything from me? Please give some feedback.

wbr

Lord-Grey commented 4 months ago

I agree that the special handling should be removed. The question is more what needs to be changed in addition. I still think the Windows MF grabber presents images with lines Bolton to top while v42l lines from top to bottom. It would be inefficient that the MF Grabber presents a flipped image to the image processing and then the image might be flipped again.

Therefore the image processing might need to be enhanced to understand the image direction and considers it indecently from the pixel format when creating the output image. i.e. just removing the code might not be good enough.

Paulchen-Panther commented 4 months ago

You can test the bgr24 format with the DroidCam application. The app is available for android and iOS. The client application is available for Windows.

https://www.dev47apps.com/

Thinner77 commented 4 months ago

Thank you for pointing me to DroidCam. I've installed Android-App and Windows-Client, and you are right, for this setup image is flipped (without special handling). But i still couldn't imagine it depends on pixelformat. So i did some research and came up to this site: https://learn.microsoft.com/en-us/windows/win32/medfound/image-stride

"In addition, there are two ways that an image can be arranged in memory. In a top-down image, the top row of pixels in the image appears first in memory. In a bottom-up image, the last row of pixels appears first in memory."

"A bottom-up image has a negative stride, because stride is defined as the number of bytes need to move down a row of pixels, relative to the displayed image. YUV images should always be top-down, and any image that is contained in a Direct3D surface must be top-down. RGB images in system memory are usually bottom-up."

There is a MF_MT_DEFAULT_STRIDE attribute (https://learn.microsoft.com/en-us/windows/win32/medfound/mf-mt-default-stride-attribute) and there a two ways to get it (https://learn.microsoft.com/en-us/windows/win32/medfound/uncompressed-video-buffers).

I've added some code to MFGrabber.cpp, MFGrabber::enumVideoCaptureDevices() to get default stride using both methods.

                                            properties.denominator = denominator;
                                            properties.pf = pixelformat;
                                            properties.guid = format;
                                            devicePropertyList.append(properties);
                                            LONG stride1 = 0;
                                            LONG stride2 = 0;
                                            HRESULT hr = pType->GetUINT32(MF_MT_DEFAULT_STRIDE, (UINT32*)&stride1);
                                            if (FAILED(hr))
                                                DebugIf (verbose, _log, "failed to get stride 1");

                                            hr = MFGetStrideForBitmapInfoHeader(format.Data1, width, &stride2);
                                            if (FAILED(hr))
                                                DebugIf (verbose, _log, "failed to get stride 2");

                                            DebugIf (verbose, _log, "%s %d x %d @ %d fps (%s) stride 1=%d/2=%d", QSTRING_CSTR(dev), properties.width, properties.height, properties.fps, QSTRING_CSTR(pixelFormatToString(properties.pf)), stride1, stride2);

Log: 2024-05-23T17:28:12.943Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:392:MFGrabber::enumVideoCaptureDevices()) Detected devices: 2 2024-05-23T17:28:13.184Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:409:MFGrabber::enumVideoCaptureDevices()) Found capture device: FJ Camera 2024-05-23T17:28:13.185Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:452:MFGrabber::enumVideoCaptureDevices()) FJ Camera 640 x 480 @ 30 fps (YUYV) stride 1=1280/2=1280 2024-05-23T17:28:13.185Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:452:MFGrabber::enumVideoCaptureDevices()) FJ Camera 640 x 480 @ 15 fps (YUYV) stride 1=1280/2=1280 ... 2024-05-23T17:28:13.186Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:446:MFGrabber::enumVideoCaptureDevices()) failed to get stride 1 2024-05-23T17:28:13.186Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:452:MFGrabber::enumVideoCaptureDevices()) FJ Camera 640 x 480 @ 30 fps (NV12) stride 1=0/2=640 2024-05-23T17:28:13.186Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:446:MFGrabber::enumVideoCaptureDevices()) failed to get stride 1 2024-05-23T17:28:13.186Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:452:MFGrabber::enumVideoCaptureDevices()) FJ Camera 640 x 480 @ 30 fps (MJPEG) stride 1=0/2=1920 ... 2024-05-23T17:28:13.285Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:409:MFGrabber::enumVideoCaptureDevices()) Found capture device: DroidCam Source 3 2024-05-23T17:28:13.285Z [V4L2:MEDIA_FOUNDATION] (DEBUG) (MFGrabber.cpp:452:MFGrabber::enumVideoCaptureDevices()) DroidCam Source 3 640 x 480 @ 29 fps (BGR24) stride 1=-1920/2=-1920

As you can see method 2 is more reliable and stride on DroidCam is negative, meaning image is send bottom-up and maybe needs to be flipped. So stride may a better indicator than pixelformat.

On the other hand: The flipmode can always be set by the user to correct the output if needed, why adding quirks to the code?

jm2c wbr

Lord-Grey commented 4 months ago

@Thinner77 Thanks for your investigation. This exactly what I was referring as the assumption. I agree that has nothing to do with pixelformat and the current implementation is a design issue.

"why adding quirks to the code" Our intend is to make things as easy as possible for users, i.e. if they setup the system it should work out of the box as much as possible. There is of course a balance between ease of use and maintainability/complexity.

In this specific case, I would say, if we already know the image is the other way round, then let's consider it during processing. Nevertheless, it should not be the limiting factor cleaning up the current design 😀 Fixing the design should be the ultimate priority...

Thank you for your effort looking into the grabber space!

Thinner77 commented 4 months ago

Hi,

i understand your point about making it as easy as possible for the user and i definitely agree with that.

But using DroidCam as input device seems like a very special usecase, isn't it? How many Windows-Users are affected? Are there any other devices sending RGB-images bottom-up? I don't know it... Let's make a poll... :smile:

Nethertheless, where is the correct place to get stride-information and how this information can be transfered to imageresampler, because IMHO this is the right place to handle it? I don't know it. I don't know anything about Windows Media Foundation and i don't have a deep knowledge of Hyperion.ng-code.

The easiest thing would be to close this PR because no one hasn't complained. Sorry for your time.

wbr

Lord-Grey commented 4 months ago

@Thinner77 let's put this to bed. Your PR highlights a design issue and that needs to be fixed. Therefore, I am happy to merge the PR to have it addressed. As you highlighted a flipping solution is in place, therefore there should no breaks. In case there is demand to improve usability, let us address this (jointly?) via an additional feature PR.

Could you just do me one favor and remove the whole if "bgr" code block ? If the bgr format is treated as everything else than this conditional code should not exit. After that I will execute on the merge.

... and THANK YOU for your time and contribution!

Thinner77 commented 4 months ago

@Lord-Grey Hi, thanks, i have removed the code block. wbr

Lord-Grey commented 4 months ago

@Thinner77 Thanks for the quick update!