OpenPHDGuiding / phd2

PHD2 Guiding
https://openphdguiding.org
BSD 3-Clause "New" or "Revised" License
244 stars 111 forks source link

Minor speed optimizations #1165

Closed Eyeke2 closed 5 months ago

Eyeke2 commented 5 months ago

Includes speed optimization with insignificant impact on image quality when performing rescaling of captured camera image on the main window.

Eyeke2 commented 5 months ago

There is a side-effect of this fix, which makes rescaled images sharper. The PR not only saves CPU time but it also helps to visually differentiate between bad pixels and stars, which due to previously used high quality rescaling makes hot pixels look more like tiny stars than what they really are - hot pixels. Here is a screenshot using camera simulator with 1 star and 10 hot pixels - before the fix. old-sim-with-hotpixels

Eyeke2 commented 5 months ago

A screenshot with 10 hot pixels and 1 star - showing the effect of PR. new-sim-with-hotpixels

agalasso commented 5 months ago

rescaling makes hot pixels look more like tiny stars than what they really are - hot pixels

actually, if you click the star profile image it will toggle to raw mode (uninterpolated) which is the best way to see the zoomed-in raw pixels.

But anyway, I do think this would be a good change for the case of scaling down large frames.

Did you try wxIMAGE_QUALITY_NEAREST? That is actually by far the fastest option. Here are some timings I captured (scaling down a large frame):

box_average = high = 40ms nearest neighbor = 2ms bilinear = 26ms

Personally I prefer nearest neighbor when scaling up a small image as it makes the hot pixels more visible like you said. But scaling down is what this pr is about. Maybe we should use bilinear when scaling down and nearest neighbor when scaling up? (scaling up a small image should be somewhat rare these days with current generation guide cameras). But if we could get away with nearest neighbor for resizing down if the quality is adequate, that would be by far the fastest.

Eyeke2 commented 5 months ago

At first I thought to use nearest neighbor, but for certain types of images it results in stars being displayed pixelized (which can be disturbing to the folks used to see naturally looking smooth star shapes). Box average takes longer than suggested bilinear, but I have overlooked scaling down. I will consider and do some tests in the next days to evaluate this option. Scaling up is not so rare, especially after switching to the 4K screen.

Eyeke2 commented 5 months ago

A screenshot with scaling factor of 1.5 and quality set to wxIMAGE_QUALITY_NEAREST - using SkySimulator ASCOM image

Eyeke2 commented 5 months ago

A screenshot with scaling factor of 1.5 and quality set to wxIMAGE_QUALITY_BILINEAR - using SkySimulator ASCOM image

Eyeke2 commented 5 months ago

A screenshot with scaling factor of 1.5 and quality set to wxIMAGE_QUALITY_BICUBIC - using SkySimulator ASCOM image

Eyeke2 commented 5 months ago

A screenshot with scaling factor of 1.5 and quality set to wxIMAGE_QUALITY_BOX_AVERAGE - using SkySimulator ASCOM image

Eyeke2 commented 5 months ago

A screenshot with scaling factor of 1.5 and quality set to wxIMAGE_QUALITY_HIGH - using SkySimulator ASCOM image

Eyeke2 commented 5 months ago

A screenshot with scaling factor of 0.6 and quality set to wxIMAGE_QUALITY_NEAREST - using SkySimulator ASCOM image

Eyeke2 commented 5 months ago

A screenshot with scaling factor of 0.75 and quality set to wxIMAGE_QUALITY_NEAREST - using SkySimulator ASCOM image

Eyeke2 commented 5 months ago

A screenshot with scaling factor of 0.99 and quality set to wxIMAGE_QUALITY_NEAREST - using SkySimulator ASCOM image

Eyeke2 commented 5 months ago

@agalasso based on the supplied screenshot samples I would suggest to modify this PR to use wxIMAGE_QUALITY_NEAREST (0) for scaling factors < 1 and use wxIMAGE_QUALITY_BILINEAR (1) for scaling factors > 1.

agalasso commented 5 months ago

@Eyeke2 thanks for the screenshots. That sounds reasonable. Do you want to update the PR as proposed?

Eyeke2 commented 5 months ago

@agalasso I still have doubts about using wxIMAGE_QUALITY_NEAREST for scaling factor < 1. After testing it on another computer with a small screen and seeing pixelization it creates when down scaling I tend to leave it always set to wxIMAGE_QUALITY_BILINEAR. Here is some background information which could be helpful for making the decision:

In wxWidgets, when downscaling an image and considering a balance between quality and CPU usage, wxIMAGE_QUALITY_BILINEAR is indeed a recommended option. Bilinear filtering provides a good compromise between the higher computational cost of more sophisticated algorithms (like bicubic resampling) and the lower quality but faster performance of nearest neighbor (wxIMAGE_QUALITY_NEAREST) resampling.

Understanding Rescaling Options: wxIMAGE_QUALITY_NEAREST: This method is the fastest as it simply picks the nearest pixel color to the target position without any interpolation. The result is often pixelated or blocky, especially noticeable in downscaling.

wxIMAGE_QUALITY_BILINEAR: Bilinear filtering averages the colors of the nearest 2x2 neighborhood of pixel squares, providing smoother transitions between pixels compared to nearest neighbor. It strikes a good balance between image quality and processing time, making it suitable for many applications where moderate downscaling quality is acceptable without significant CPU load.

wxIMAGE_QUALITY_BICUBIC (if available) and wxIMAGE_QUALITY_HIGH (typically maps to bicubic or better): These methods offer higher quality by considering more pixels in the interpolation process, which results in smoother images but at a higher computational cost.

bwdev01 commented 5 months ago

I'm not sure where we're going with this. IMO, none of this is important in terms of actual guiding, it's cosmetic. But what is important is that we don't slow down the image_ready processing. We have people with strain-wave mounts who need to do fast-cadence guiding and we don't want to generate a bunch of new complaints about the processing overhead in PHD2. Let's not fix something that isn't broken in a way that matters, and if we do make changes, let's be sure we're looking at real star images from a real guiding system. :-)

agalasso commented 5 months ago

I'm not sure where we're going with this. IMO, none of this is important in terms of actual guiding, it's cosmetic. But what is important is that we don't slow down the image_ready processing.

@bwdev01 I agree 100%. I posted some timings in one of the earlier comments (here). The current value, wxIMAGE_QUALITY_HIGH (alias for wxIMAGE_QUALITY_BOX_AVERAGE) is slower than the proposed change, so if we can get away with with nearest neighbor and/or bilinear it will the reduce cpu cost of image_ready processing in the case of scaling up or scaling down the frame for display.

bwdev01 commented 5 months ago

Ok, thanks for the clarification.

From: Andy Galasso @. Sent: Monday, February 5, 2024 9:37 PM To: OpenPHDGuiding/phd2 @.> Cc: bwdev01 @.>; Mention @.> Subject: Re: [OpenPHDGuiding/phd2] Minor speed optimizations (PR #1165)

I'm not sure where we're going with this. IMO, none of this is important in terms of actual guiding, it's cosmetic. But what is important is that we don't slow down the image_ready processing.

@bwdev01 https://github.com/bwdev01 I agree 100%. I posted some timings in one of the earlier comments (here https://github.com/OpenPHDGuiding/phd2/pull/1165#issuecomment-1913327214 ). The current value, wxIMAGE_QUALITY_HIGH (alias for wxIMAGE_QUALITY_BOX_AVERAGE) is slower than the proposed change, so if we can get away with with nearest neighbor and/or bilinear it will the reduce cpu cost of image_ready processing in the case of scaling up or scaling down the frame for display.

— Reply to this email directly, view it on GitHub https://github.com/OpenPHDGuiding/phd2/pull/1165#issuecomment-1928819563 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ADDHSV5H6PQDJ3JCZV6ZRC3YSG6Q5AVCNFSM6AAAAABCMBDYQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRYHAYTSNJWGM . You are receiving this because you were mentioned. https://github.com/notifications/beacon/ADDHSV4NIP6Y5KLUTPFQEATYSG6Q5A5CNFSM6AAAAABCMBDYQ2WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTS65ZWW.gif Message ID: @. @.> >

Eyeke2 commented 5 months ago

@bwdev01 @agalasso I admit the name of this PR is now misleading, it should say something like "improve speed of image rescaling while making hot pixels less resembling stars". For big 4k screens and cameras with large pixel count this fix is quite noticeable in terms of UI responsiveness (which is even more pronounced in debug mode). The highest rescaling quality used before this PR is an overkill which easily causes hot pixels to be rendered with profiles reminiscent of small stars.

Eyeke2 commented 5 months ago

@agalasso I removed the memset optimizations and left only the rescale quality fix.