Revert "Faster pipelines"

[lboclboc]

Reverts #2

Sorry, but this change actually crashes the imx219 camera instead. Reverting.

[lboclboc]

This was the stack trace from coredump:
#0 0x004e3b2c in Raw10ToBayer16Pipeline::data_received (this=0x7937b0,
data=0x78ccb8 "\020\017\017\017\374\020\020\020\020U\020\020\020\020T\020\020\020\020T\020\020\017\020\060\020\020\020\020D\020\020\017\020\060\017\020\020\020C\020\017\020\020L\020\020\020\020\020\020\020\020\020\001\017\020\020\020C\020\020\020\020", length=26717) at /home/pi/git/indi-3rdparty/indi-rpicam/raw10tobayer16pipeline.cpp:130
#1 0x004e4e20 in Pipeline::forward (this=0x793698, data=0x78ccb3 "\020\020\020\020", length=26717) at /home/pi/git/indi-3rdparty/indi-rpicam/pipeline.cpp:52
#2 0x004e57c4 in BroadcomPipeline::data_received (this=0x793698, data=0x78ccb3 "\020\020\020\020", length=26717) at /home/pi/git/indi-3rdparty/indi-rpicam/broadcompipeline.cpp:43
#3 0x004e4e20 in Pipeline::forward (this=0x7775d8, data=0x784cb2 "@BRCMo", length=59486) at /home/pi/git/indi-3rdparty/indi-rpicam/pipeline.cpp:52
#4 0x004e5034 in JpegPipeline::data_received (this=0x7775d8, data=0x784cb2 "@BRCMo", length=59486) at /home/pi/git/indi-3rdparty/indi-rpicam/jpegpipeline.cpp:51
#5 0x004dfcfc in CameraControl::signal_data_received (this=0x777700,
data=0x77f510 "a\375\255\071F\327wmY\331\335'\247\347\370\234k8\021\355 仩\030\343\030\av\356ܞ\254\027\247\a\245B\031"b\371F\344\b\337h;@\004\034\261=z\363\327\030\357\315\\272(\215$r\251!\267)\v\202K\217\342\306Fؓ\200\253Ѝ\271\352H\251\035\274i\274\063yۆ", length=81920) at /home/pi/git/indi-3rdparty/indi-rpicam/cameracontrol.cpp:119
indilib#6 0x004dfb40 in CameraControl::buffer_received (this=0x777700, port=0x77dc30, buffer=0x77f318) at /home/pi/git/indi-3rdparty/indi-rpicam/cameracontrol.cpp:96
#7 0x004e84ac in MMALComponent::port_callback (this=0x776fa0, port=0x77dc30, buffer=0x77f318) at /home/pi/git/indi-3rdparty/indi-rpicam/mmalcomponent.cpp:86
indilib#8 0x004e83c4 in MMALComponent::c_port_callback (port=0x77dc30, buffer=0x77f318) at /home/pi/git/indi-3rdparty/indi-rpicam/mmalcomponent.cpp:68
#9 0x769e17f4 in mmal_port_buffer_header_callback () from /opt/vc/lib/libmmal_core.so
indilib#10 0x769a7dc0 in mmal_vc_do_callback_loop () from /opt/vc/lib/libmmal_vc_client.so
indilib#11 0x769e49b8 in mmal_component_action_thread_func () from /opt/vc/lib/libmmal_core.so
indilib#12 0x7692dcb0 in vcos_thread_entry (arg=0x77c3e0) at /home/dom/projects/staging/userland/interface/vcos/pthreads/vcos_pthreads.c:144
indilib#13 0x761dd494 in start_thread (arg=0x725fb0c0) at pthread_create.c:486

[QRainman]

Is this with sub-frame?

[lboclboc]

Is this with sub-frame?

No, just a plain exposure full frame.

[QRainman]

Strange, i took about 50 pictures with my V2 (NoIr version) camera now and no crashes. Also tried sub-frames with all kinds of odd offset and shapes and did not get a single crash related to that decoder. If you can reproduce, could you check what bytes_consumed, maxX, maxY and the addresses of frame_buffer and pu32 at the crash site are?

I did get one crash in INDI::CCDChip::setFrameBufferSize though, which i unfortunately could not reproduce, even with the same settings. Since the line that crashes is trying to write to the frame buffer, maybe there is an issue there? The pointer values and bytes_consumed should give us a hint if i'm really writing beyond the frame buffer or if the buffer is too small for some reason.

Here's the stack trace:
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x766af230 in __GI_abort () at abort.c:79
#2 0x766ff51c in __libc_message (action=action@entry=do_abort, fmt=) at ../sysdeps/posix/libc_fatal.c:181
#3 0x76706044 in malloc_printerr (str=) at malloc.c:5341
#4 0x76708034 in _int_free (av=0x767e27d4 <main_arena>, p=0xb44410, have_lock=) at malloc.c:4314
#5 0x76e0a2e0 in INDI::CCDChip::setFrameBufferSize(unsigned int, bool) () from /usr/lib/arm-linux-gnueabihf/libindidriver.so.1
indilib#6 0x0049001c in MMALDriver::UpdateCCDFrame (this=0xb17888, x=102, y=101, w=302, h=301) at /home/astroberry/indi-source/indi-3rdparty-qrainman/indi-rpicam/mmaldriver.cpp:302
#7 0x76defd68 in INDI::CCD::ISNewNumber(char const*, char const*, double*, char**, int) () from /usr/lib/arm-linux-gnueabihf/libindidriver.so.1
indilib#8 0x004911a0 in MMALDriver::ISNewNumber (this=0xb17888, dev=0xb28958 "RPI Camera", name=0xb27a00 "CCD_FRAME", values=0xb443d8, names=0xb44400, n=4)
at /home/astroberry/indi-source/indi-3rdparty-qrainman/indi-rpicam/mmaldriver.cpp:513
#9 0x0048d754 in ISNewNumber (dev=0xb28958 "RPI Camera", name=0xb27a00 "CCD_FRAME", values=0xb443d8, names=0xb44400, n=4)
at /home/astroberry/indi-source/indi-3rdparty-qrainman/indi-rpicam/indi_rpicam.cpp:56
indilib#10 0x76dcf5a4 in dispatch () from /usr/lib/arm-linux-gnueabihf/libindidriver.so.1
indilib#11 0x76dd04c0 in clientMsgCB () from /usr/lib/arm-linux-gnueabihf/libindidriver.so.1
indilib#12 0x76dd2168 in ?? () from /usr/lib/arm-linux-gnueabihf/libindidriver.so.1

[lboclboc]

NoIr version

That is strange, perhaps my setup was faulty in some way then. Ill do some more testings with your PR..
What version of libindi and raspberry OS are you building with? Could be there somewhere also?

Also, what exact version of the camera do you have, 2.1 or just 2? (wasn't even aware that there was multiple versions...)

[QRainman]

I'm on:
Linux astroberry 5.4.79-v7
libindi 1.8.7

[lboclboc]

Forget it. When i recompile everything now from scratch its all working fine on both imx219 and ov5647. Not sure what happened. I did upgrade one of my RPI:s to latest version of raspian. Ill try to bring in your patch on master and the create a PR for inidi-3rdparty. Sorry for the inconveniance.

[lboclboc]

Ok, forget the forget (sorry for being so confused). Seems your original PR with final commit 29989b6 works fine for both cameras, but after updating it with my latest master then resulting in commit 5287b79 does actually crash on V2 camera....
I suspect the subframing here is the problem. It does how ever crash using no-subframing....

[lboclboc]

Here are the printouts from gdb you requested from the core-file:
(gdb) print bytes_consumed
$8 = 0
(gdb) print maxX
$9 = 3280
(gdb) print maxY
$10 =
(gdb) print frame_buffer
$11 = (uint16_t *) 0x0
(gdb) print pu32
$12 = (uint32_t *) 0x4

frame_buffer = 0 does not sound too good. Maybe the pipeline reset it not being done properly here... I saw you had some comments about that in raw10...::reset()

[QRainman]

Hehe, no worries, i know the feeling 👍
I'll re-check out that commit and have a look.

[QRainman]

Ah, ok. Can you try to put
frame_buffer = reinterpret_cast<uint16_t *>(ccd->getFrameBuffer());
back to the head of data_received?

[QRainman]

I originally thought all those states where there in the reset, because you have ccd->getSubX() etc in there, but apparently this is not always the case. It looks like there is a race condition between the setup thread and the callback thread. On my instance it works fine with the frame_buffer assign in the reset function but, yeah, i saw it fail in this manner before but thought that was fixed with your latest master, so i put it back up there.

[lboclboc]

I just did, but same result, frame_buffer is 0 at crash.

[lboclboc]

I get the feeling the frame_buffer in the ccd-object is overwritten somehow?

[lboclboc]

I added an assertion of the CCD framebuffer in mmaldriver::StartExposure and its framebuffer is null there already.
Seems to be some initialization of the CCD missing now:
bool MMALDriver::StartExposure(float): Assertion `PrimaryCCD.getFrameBuffer() != 0' failed.

[lboclboc]

Found it, the error was actually allready in my branch. I commented out call to UpdateCCDFrame because I did not think that the driver should call that itself. Seems the frame buffer was missing if not done. This change made it work. Ill test on all other cameras also and try to merge it on top of master:

diff --git a/indi-rpicam/mmaldriver.cpp b/indi-rpicam/mmaldriver.cpp
index 00a7b199..82fbeab2 100644
--- a/indi-rpicam/mmaldriver.cpp
+++ b/indi-rpicam/mmaldriver.cpp
@@ -157,9 +157,9 @@ bool MMALDriver::Connect()
camera_control->get_camera()->xPixelSize,
camera_control->get_camera()->yPixelSize);

-// // Should probably not be called by the subclass of CCD - not clear.
-// UpdateCCDFrame(0, 0, static_cast(camera_control->get_camera()->get_width()),
-// static_cast(camera_control->get_camera()->get_height()));

• // Should probably not be called by the subclass of CCD - not clear.
• UpdateCCDFrame(0, 0, static_cast(camera_control->get_camera()->get_width()),

[QRainman]

Also this here:
It looks liek they are using some asynchronous buffer manager in the background, that's why i saw it succeed sometimes and fail other times. You have to put this buffer lock around startExposure()
Edit: it has to go before camera->startCapture(); and the other half has to go into signal_complete()

INDI::CCD and INDI::StreamManager both upload frames asynchrounously in a worker thread.
 * The CCD Buffer data is protected by the ccdBufferLock mutex. When reading the camera data
 * and writing to the buffer, it must be first locked by the mutex. After the write is complete
 * release the lock. For example:
 *
 * \code{.cpp}
 * std::unique_lock<std::mutex> guard(ccdBufferLock);
 * get_ccd_frame(PrimaryCCD.getFrameBuffer);
 * guard.unlock();
 * ExposureComplete();
 * \endcode
 *

[lboclboc]

Also this here:
It looks liek they are using some asynchronous buffer manager in the background, that's why i saw it succeed sometimes and fail other times. You have to put this buffer lock around startExposure()
Edit: it has to go before camera->startCapture(); and the other half has to go into signal_complete()

INDI::CCD and INDI::StreamManager both upload frames asynchrounously in a worker thread.
 * The CCD Buffer data is protected by the ccdBufferLock mutex. When reading the camera data
 * and writing to the buffer, it must be first locked by the mutex. After the write is complete
 * release the lock. For example:
 *
 * \code{.cpp}
 * std::unique_lock<std::mutex> guard(ccdBufferLock);
 * get_ccd_frame(PrimaryCCD.getFrameBuffer);
 * guard.unlock();
 * ExposureComplete();
 * \endcode
 *

I think I am doing that already?
grep -n lock mmaldriver.cpp
347: ccdBufferLock.lock();
383: ccdBufferLock.unlock();
449: ccdBufferLock.unlock();

[QRainman]

Whoops sorry, missed that.
But then the question remains: I have the same code with UpdateCCDFrame also commented out, but it works for me??

[lboclboc]

Whoops sorry, missed that.
But then the question remains: I have the same code with UpdateCCDFrame also commented out, but it works for me??

Not sure no, but my code for sure was missing an explicit allocation of the buffer. Perhaps your kstars or other software is actually causing a call to UpdateCCDFrame for some other reason and that would have allocated the buffer.
I have just pushed to by branch a merge of your branch and that extra explicit allocation. You are welcome to test of course. Ill test it now and then submit a PR to Jasem

[lboclboc]

I've tested on all cameras now and created a pr upstream. There still seems to be a problem with long exposure. It didn't work at start but then after just restarting I started to work again. I suspect there are some parameter default setting missing some where but I'll have to look into that later. I also saw that the raw10... Does not nudge the msb ccd bit to bit15 in the imag buffer resulting in quite dark images.. I'll check that later also

[QRainman]

Yes, i've been trying to figure out that exposure problem too. For me, the v1 does not go beyond 1s and the v2 does not go beyond 6s, but raspistill does not seem to have any problems with that.

I also saw that the raw10... Does not nudge the msb ccd bit to bit15 in the imag buffer resulting in quite dark images

Ah, i did not realize that was on purpose. I thought that was a typo/bug coming over from the 12bit decoder (never actually looked at that one) and put it back to bit10.

[lboclboc]

I think I found the problem with long exposure for the other two cameras in commit d265fc2 . I also stretched bit9 to bit15, not sure I got your code correct so you are very welcome to test that on both v1 and v2 camera. I think the color on v2 is not correct, but then my v2 camera is partly broken so Im not sure.

[lboclboc]

Seems to be something wrong with the alignmen on V2, the bayer info gets shifted one pixel somehow.

[QRainman]

Just a sec, i need to hook up the other camera

[QRainman]

Ok i see what you mean. Don't they have the same pixel layout? The V1 has the right colors. Is that shift in Y or in X?

[QRainman]

Great, looks like they changed the filter pattern on the sensor between v1 and v2 ...
V1

                u32_01 = (*data++ << 18);
                u32_01 |= (*data++ << 2); // each 32-bit value will hold 2 source bytes spread out to 16-bits
                u32_23 = (*data++ << 18); // and shifted over 2 to make room for lower 2 bits
                u32_23 |= (*data++ << 2);`

V2

                u32_01 = (*data++ << 2);
                u32_01 |= (*data++ << 18); // each 32-bit value will hold 2 source bytes spread out to 16-bits
                u32_23 = (*data++ << 2); // and shifted over 2 to make room for lower 2 bits
                u32_23 |= (*data++ << 18);`

Plus the lsb pattern has to be changed too. I'll prepare another PR...

[lboclboc]

Wow, ok . just to make life easier for us I presume. I suspect this information is somehow available in the broadcom 32K header block. But not sure where.

Thanks.

[QRainman]

It's not ...
It is however in the application notice for the sensors:
v1: http://cdn.sparkfun.com/datasheets/Dev/RaspberryPi/ov5647_full.pdf Page 25
v2: https://raw.githubusercontent.com/circuitvalley/mipi_csi_receiver_FPGA/master/Camera%20Datasheet/RASPBERRY%20PI%20CAMERA%20V2%20DATASHEET%20IMX219PQH5_7.0.0_Datasheet_XXX.PDF Page 52/53

Funnily, the V2 version is GBRG and the original decoder should have produced color shifted outputs on V2. Coincidentally there seems to be also an offset error of 5 pixels to the left, which turns the GBRG into BGGR which makes the picture correct again, albeit shifted to the left. I'm trying to find out where that offset is coming from now and then it's just a question of setting BayerT[2] in the ccd chip parent class to the correct string. This also works for the indi settings page of the camera. Setting your V2 to GBRG should fix the color shift you are seing right now.

[QRainman]

Never mind about that shift, it looks like i'm just not handling the end of a line properly and the last few pixels are cut off somehow. There is still something wrong though, because the old, byte wise decoding should be correct for the V1 and wrong for the V2 but it's exactly the other way around...

[QRainman]

I created another pull request for the color fixes which also fixed the problem with the cut off pixels at the right side of the frame. I'm not really happy that the color pattern is exactly the opposite of the documentation but i also can't find out where there that +/- 1 offset is introduced, so i'm leaving i suggest to leave it like that for now.

Does your master already contain the fix for the exposure time? My cameras still don't go beyond 1s for V1 and 6s for V2.

[QRainman]

One more thing: I am not sure if the 2 LSBs for every pixel are set correctly. The only test i can think of right now would be to put an appropriate color filter in front of the camera and check which bits go up. However the pedestal is already > 2 bit and the pixel noise would make this almost impossible.

[lboclboc]

One more thing: I am not sure if the 2 LSBs for every pixel are set correctly. The only test i can think of right now would be to put an appropriate color filter in front of the camera and check which bits go up. However the pedestal is already > 2 bit and the pixel noise would make this almost impossible.

Thanks for the PR3, checking shortly (no more holiday sadly). I created a discussion board for my fork, lets move discussion there instead of this closed PR... Its here https://github.com/lboclboc/indi-3rdparty/discussions