×
INDI Library v1.8.5 Released (19 Apr 2020)

April 2020 release of INDI Library v1.8.5 introduces new drivers while providing fixes and improvements to existing devices and core framework.

Bug in fitsdata.cpp

5 months 3 days ago
lock42
Senior Boarder
Senior Boarder
Posts: 41
More
Topic Author
Bug in fitsdata.cpp #50389
When reading a FITS file with X or Y offsets, crash could occur.
With valgrind I have the following error:
==627021== Thread 17 Thread (pooled):
==627021== Invalid read of size 2
==627021==    at 0xBE7A0C: dc1394_bayer_NearestNeighbor_uint16 (bayer.c:978)
==627021==    by 0xBF2176: dc1394_bayer_decoding_16bit (bayer.c:2544)
==627021==    by 0x401A47: FITSData::debayer_16bit() (fitsdata.cpp:3461)
==627021==    by 0x4013E5: FITSData::debayer() (fitsdata.cpp:3347)
==627021==    by 0x3F9676: FITSData::privateLoad(void*, unsigned long, bool) (fitsdata.cpp:347)
==627021==    by 0x45E7B6: QtConcurrent::StoredMemberFunctionPointerCall3<bool, FITSData, void*, decltype(nullptr), unsigned long, int, bool, bool>::runFunctor() (qtconcurrentstoredfunctioncall.h:1275)
==627021==    by 0x222C7A: QtConcurrent::RunFunctionTask<bool>::run() (qtconcurrentrunbase.h:108)
==627021==    by 0x6BB3D31: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.5)
==627021==    by 0x6BB08B1: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.5)
==627021==    by 0x75C6FB6: start_thread (pthread_create.c:486)
==627021==    by 0x7C621AE: clone (clone.S:95)
==627021==  Address 0x37689840 is 0 bytes after a block of size 32,778,240 alloc'd
==627021==    at 0x483750F: operator new[](unsigned long) (vg_replace_malloc.c:433)
==627021==    by 0x3F942D: FITSData::privateLoad(void*, unsigned long, bool) (fitsdata.cpp:325)
==627021==    by 0x45E7B6: QtConcurrent::StoredMemberFunctionPointerCall3<bool, FITSData, void*, decltype(nullptr), unsigned long, int, bool, bool>::runFunctor() (qtconcurrentstoredfunctioncall.h:1275)
==627021==    by 0x222C7A: QtConcurrent::RunFunctionTask<bool>::run() (qtconcurrentrunbase.h:108)
==627021==    by 0x6BB3D31: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.5)
==627021==    by 0x6BB08B1: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.12.5)
==627021==    by 0x75C6FB6: start_thread (pthread_create.c:486)
==627021==    by 0x7C621AE: clone (clone.S:95)
==627021== 

Please Log in or Create an account to join the conversation.

5 months 12 hours ago
lock42
Senior Boarder
Senior Boarder
Posts: 41
More
Topic Author
Bug in fitsdata.cpp #50479
Any idea about this?

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago
Kaczorek
Moderator
Moderator
Posts: 910
Karma: 6
More
Bug in fitsdata.cpp #50505
What's the bug you're referring to?

--
Radek Kaczorek
Astroberry Server | NEQ6 | Atik 460EX | Atik EFW2 | ASI 120MM

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago
lock42
Senior Boarder
Senior Boarder
Posts: 41
More
Topic Author
Bug in fitsdata.cpp #50506
Some crash in the fitsviewer when opening a FITS image containing a field XBAYROFF or YBAYROFF different from 0.

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago
hy
Gold Boarder
Gold Boarder
Posts: 286
Karma: 3
More
Bug in fitsdata.cpp #50511
I've just taken a look, and this is the issue lock42 is reporting:

The bayer pattern, e.g. RGGB,  will look like 
RGRGRGRGR...
GBGBGBGBG...
RGRGRGRGR...
GBGBGBGBG...
...
see  renderingpipeline.com/2013/04/a-look-at-the-bayer-pattern/
and is specified in the header under the fits keyword BAYERPAT.

Looking at checkDebayer() in the KStars source, github.com/KDE/kstars/blob/master/kstars...r/fitsdata.cpp#L3268
you can see the patterns accepted are: RGGB GBRG, GRBG, and BGGR (for fits files--for raw files, the processing will happen in the libraw library). 

It's possible to add the fits keywords XBAYROFF and YBAYROFF, which are only honored in the code if they're 0 [no effect], or 1).
github.com/KDE/kstars/blob/master/kstars...r/fitsdata.cpp#L3373
I assume 1 means that the pattern skips a row or column.

The implementation of the Y offset seems like it will not cause a memory error--it skips the first row and decrements the height by 1,
though the resulting image will probably have a "junk last row" which I suppose can be cropped off.

The main issue seems to be the implementation of the X offset--the code increments the buffer pointer if the offset is 1, but since it uses the same buffer size, it would read past the end of the buffer, and also, the transitions between rows will be odd. It is constrained though, because the function called to do the debayer, dc1394_bayer_decoding_16bit(), doesn't have that X or Y offset in its API.

Perhaps the best thing to do is if the X offset is 1, then:
RGGB should become GRBG,
GBRG should become BGGR
GRBG should become RGGB
BGGR should become GBRG
with no change in X offset or width. That is, we'd continue to process the 0th column, even though the offset implies to ignore it, but all other columns would be processed correctly, and the reading-past-end-of-buffer issue would be resolved.

It will then hopefully not have memory issues anymore with X- or Y-offsets, though the first row and/or first column will have values that should be cropped out.

I'll send this change to Jasem and point him to this thread. If you have any comments or objections, please send them to this thread and/or comment on the PR: phabricator.kde.org/D27907

Hy
The following user(s) said Thank You lock42

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago
knro
Administrator
Administrator
Posts: 8187
Karma: 51
Bug in fitsdata.cpp #50512
Ok I thought that XBAYEROFF being 1 meaning keeping the same pattern as specified but simply skipping a row when reading the buffer. It's not the case?

Jasem Mutlaq
Support INDI & Ekos; Get StellarMate Astrophotography Gadget.
How to Submit Logs when you have problems?
Add your observatory info

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago
lock42
Senior Boarder
Senior Boarder
Posts: 41
More
Topic Author
Bug in fitsdata.cpp #50515
Does 0 and 1 the only values that can take these fields?

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago
hy
Gold Boarder
Gold Boarder
Posts: 286
Karma: 3
More
Bug in fitsdata.cpp #50516
I am not an expert in what those header fields mean, however:

Jasem said: Ok I thought that XBAYEROFF being 1 meaning keeping the same pattern as specified but simply skipping a row when reading the buffer. It's not the case?

Your interpretation is what I thought too. What I implemented is really just that--I switched the pattern but didn't skip the column, but the net effect, given XBAYEROFF=1 and the pattern I switched to, was as if we were using same pattern as we started with starting with column 1. It's just that the column wasn't skipped now, and may have off pixel values and need to be cropped out.

Lock42 said: Does 0 and 1 the only values that can take these fields?

I don't know what the header means, but I kept what was implemented in the code--which was that only 0 and 1 were used. Anything else was treated as 0. We could implement something that cropped the image according to those values, but give that I was unfamiliar with the intent of these header fields, I was really just fixing the memory violation that was the original subject of this thread. However, now if there are values other than 0 and 1, the code will complain.

Lock42: As this has been submitted, Please test to make sure this fixes your issue.

Hy
The following user(s) said Thank You lock42

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago 4 months 4 weeks ago by lock42.
lock42
Senior Boarder
Senior Boarder
Posts: 41
More
Topic Author
Bug in fitsdata.cpp #50517
I think you switched bayer pattern, I would write:
switch (debayerParams.filter)
        {
        case DC1394_COLOR_FILTER_RGGB:
            debayerParams.filter = DC1394_COLOR_FILTER_GBRG;
            break;
        case DC1394_COLOR_FILTER_GBRG:
            debayerParams.filter = DC1394_COLOR_FILTER_RGGB;
            break;
        case DC1394_COLOR_FILTER_GRBG:
            debayerParams.filter = DC1394_COLOR_FILTER_BGGR;
            break;
        case DC1394_COLOR_FILTER_BGGR:
            debayerParams.filter = DC1394_COLOR_FILTER_GRBG;
            break;
        }

EDIT : ok no, but My bayer pattern seems not working.

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago 4 months 4 weeks ago by lock42.
lock42
Senior Boarder
Senior Boarder
Posts: 41
More
Topic Author
Bug in fitsdata.cpp #50518
@hy:

OK, my tests shows (am I wrong?) that:
case DC1394_COLOR_FILTER_GRBG:
            debayerParams.filter = DC1394_COLOR_FILTER_GBRG

Would be the fine transformation. But I don't know why because what you say should be the right answer. (Is it maybe due to the FITS orientation?)

EDIT: I attach the file for testing
filesender.renater.fr/?s=download&token=...86-d0f5-bbb76cef10ff

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago
hy
Gold Boarder
Gold Boarder
Posts: 286
Karma: 3
More
Bug in fitsdata.cpp #50526
Lock42: If you feel strongly about this, please feel free to make the changes yourself and send the change to jasem (mutlaqja would be his kde identity) who will have the final say. You seem to have the necessary skills, and we encourage people to step up and help with development.  I just got involved to help out, fix a potential crash, etc. I have no strong feelings here and don't have a camera where this would apply.  That said, if it is difficult for you to submit a change request, then I'm happy to do it on your behalf.  I started a thread with instructions on how to get started with developement here: indilib.org/forum/development/6171-how-t...ping-for-kstars.html . However, my gut reaction is that the code is fine as is. Please see below.

Back to the topic at hand: 
I looked at the fits file that you sent. It's header includes: 

INSTRUME    'ZWO CCD ASI1600MC-Cool'
BAYERPAT    'GRBG    '
XBAYROFF    1
YBAYROFF    0
COMMENT    Generated by INDI

So, it seems that Indi wrote this file. I'm wondering if the header values are correct. As I don't have this camera, I can't really run Indi with the right camera driver. But, perhaps that driver is misconfigured. It seems from the indi source code that you can specify the values for the Bayer Pattern and the X and Y offsets in the Indi control panel in the "image info tab" for your camera, see  github.com/indilib/indi/blob/master/libs...ase/indiccd.cpp#L228
Since I feel, and you seem to agree, that the code as I wrote it "makes sense", my inclination would be to leave the code as is, and ask you to adjust the values in your indi control panel.  If you just used the defaults, perhaps they are wrong? I don't know if they are manufacturer supplied, or if they are somehow generated within indi.

Note that there does seem to be debate about what the "correct" values should be for that camera, e.g. see:
forums.sharpcap.co.uk/viewtopic.php?t=435
forums.sharpcap.co.uk/viewtopic.php?f=19&t=305

Hy
The following user(s) said Thank You lock42

Please Log in or Create an account to join the conversation.

4 months 4 weeks ago
lock42
Senior Boarder
Senior Boarder
Posts: 41
More
Topic Author
Bug in fitsdata.cpp #50532
No problem Hy, I think you're right. Also, the header of my file is probably wrong too. But, I'm happy because it helps to fix wrong code.
Last remark, maybe it would be better to apply same fix for yoffset? In order to avoid junk row?

Many thanks,
Cheers,

Please Log in or Create an account to join the conversation.

Time to create page: 0.683 seconds