×

INDI Library v2.0.7 is Released (01 Apr 2024)

Bi-monthly release with minor bug fixes and improvements

Read More...

Bug in fitsdata.cpp

  • Posts: 57
  • Thank you received: 5

Bug in fitsdata.cpp was created by lock

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==
4 years 1 month ago #50389

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

  • Posts: 57
  • Thank you received: 5

Replied by lock on topic Bug in fitsdata.cpp

Any idea about this?
4 years 1 month ago #50479

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

  • Posts: 983
  • Thank you received: 375

Replied by Radek Kaczorek on topic Bug in fitsdata.cpp

What's the bug you're referring to?
4 years 1 month ago #50505

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

  • Posts: 57
  • Thank you received: 5

Replied by lock on topic Bug in fitsdata.cpp

Some crash in the fitsviewer when opening a FITS image containing a field XBAYROFF or YBAYROFF different from 0.
4 years 1 month ago #50506

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

  • Posts: 1220
  • Thank you received: 565

Replied by Hy Murveit on topic Bug in fitsdata.cpp

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: lock
4 years 1 month ago #50511

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

Replied by Jasem Mutlaq on topic Bug in fitsdata.cpp

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?
4 years 1 month ago #50512

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

  • Posts: 57
  • Thank you received: 5

Replied by lock on topic Bug in fitsdata.cpp

Does 0 and 1 the only values that can take these fields?
4 years 1 month ago #50515

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

  • Posts: 1220
  • Thank you received: 565

Replied by Hy Murveit on topic Bug in fitsdata.cpp

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: lock
4 years 1 month ago #50516

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

  • Posts: 57
  • Thank you received: 5

Replied by lock on topic Bug in fitsdata.cpp

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.
Last edit: 4 years 1 month ago by lock.
4 years 1 month ago #50517

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

  • Posts: 57
  • Thank you received: 5

Replied by lock on topic Bug in fitsdata.cpp

@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
Last edit: 4 years 1 month ago by lock.
4 years 1 month ago #50518

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

  • Posts: 1220
  • Thank you received: 565

Replied by Hy Murveit on topic Bug in fitsdata.cpp

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: lock
4 years 1 month ago #50526

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

  • Posts: 57
  • Thank you received: 5

Replied by lock on topic Bug in fitsdata.cpp

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,
4 years 1 month ago #50532

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

Time to create page: 0.882 seconds