Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zero: meta-csp: add image capture capability for scsat1 #53

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

moonlight83340
Copy link
Member

@moonlight83340 moonlight83340 commented Oct 9, 2024

Implemented functionality to capture images (photos) using the camera on scsat1.
Modified the camera service to use libcamerify in front of the program to
ensure compatibility with libcamera.
Utilized v4l2 for direct image capture from the camera.
This adds the capability to capture images from the camera on scsat1.

Change JPEG quality from default value 75 to 90.
Currently set as a define value, not a user configuration.

Implemented functionality to capture images (photos) using the camera on scsat1.

Modified the camera service to use `libcamerify` in front of the program to
ensure compatibility with libcamera.

Utilized `v4l2` for direct image capture from the camera.

This adds the capability to capture images from the camera on scsat1.

Signed-off-by: Gaetan Perrot <[email protected]>
Change JPEG quality from default value 75 to 90.
Currently set as a define value, not a user configuration.

Signed-off-by: Gaetan Perrot <[email protected]>
Copy link
Member

@yashi yashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we encode to JPEG in the first place. Was it a requirement?


#define CAM_FRAME_PATH "/storageA/photo"
#define CAM_FRAME_PREFIX "frame"

static int xioctl(int fd, int request, void *arg)
{
for (;;) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to have infinit loops in space grade software. I know we have it in Zephyr kernel and libcsp right now. But this whole xioctl() thing is a way for ordinal software, such as editors, language runtime, and many others. That's why AI uses it. (I'm assuming it's generated)

static void get_next_filename(char *filename, size_t size)
{
unsigned num = 0;
while (1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

jpeg_destroy_compress(&cinfo);
}

// Function to capture images
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no C++ comment style.

}
close(fd);

rgbbuf = malloc(3 * length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use literal numbers. Define it if you need.

@@ -100,6 +333,11 @@ void capture_frame_service(csp_conn_t *conn)
(void)capture_frame();
}

void capture_jpeg_service(csp_conn_t *conn)
{
(void)camera_jpeg("/dev/video0");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define "/dev/video0".

Comment on lines 14 to 18
#define PORT_T (11U) /* for get temperature */
#define PORT_I (12U) /* for init photo dir */
#define PORT_C (13U) /* for capture frame */
#define PORT_J (15U) /* for capture JPEG*/
#define PORT_F (14U) /* for get frame count */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but can we straighten this out? What's the point of having port definitions with alphabet characters that don't align with unordered port numbers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because I implemented PORT_T, among others, after PORT_A.
I wanted to make the PORT_XX part more meaningful, like using 'T' for Temperature, but I think that it hsn't been very effective.
I think it's better to change it to defines like PORT_GET_TEMPERATUREin a separate PR.


#ifdef __ARM_NEON__
/*
* Based on libyuv/source/row_neon.cc [https://code.google.com/p/libyuv]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Have you checked the license of libyuv?
  2. Why did you copy & past a file from libyuv instead of lintegrate libyuv?

@@ -43,6 +43,10 @@ void *handle_csp_packet(void *param)
csp_buffer_free(packet);
break;

case PORT_J:
capture_jpeg_service(conn);
csp_buffer_free(packet);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR but we must move csp_buffer_free() out of each case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it.
I think it would be better to free it within xx_service, similar to the standard csp_service_handler in CSP.

@sasataku
Copy link
Member

Why do we encode to JPEG in the first place. Was it a requirement?

I asked @moonlight83340 to encode it to JPEG, but this is not a must requirement.
For now, I am requesting JPEG encoding from the following perspectives:

  • I initially tried generating a JPEG file using the Arducam sample, so I believe using the same encoding will make it easier to compare.
  • I thought there might be more V4L2 samples available.

@yashi
Copy link
Member

yashi commented Oct 12, 2024

  • I initially tried generating a JPEG file using the Arducam sample, so I believe using the same encoding will make it easier to compare.

OK. But, to make the code easier, I'm sure it's easier to just write out the buffer to a file; No conversion needed. OTOH, it is indeed easier to check the writen file by image viewer if the file is econded in JPEG.

We should keep all images in raw format. Format conversion should be done on either with downlink command with on-the-fly conversion, or a dedicated command, IMHO.

@sasataku
Copy link
Member

sasataku commented Oct 12, 2024

We should keep all images in raw format. Format conversion should be done on either with downlink command with on-the-fly conversion, or a dedicated command, IMHO.

Thank you for your feedback. I initially thought it was necessary to save in JPEG format in case of low downlink rates, but as you pointed out, I reconsidered and decided to unify all storage in raw format:

  1. Download the files as is and convert them on the ground.
  2. Prepare a command to convert RAW to JPEG or other formats on orbit.

@moonlight83340
Please modify this PR to save in raw format instead of JPEG. Separately, please try converting the saved files to JPEG or other formats, and share the method and results.
(You don't need to implement the downlink command yet, so copy the file you saved via ssh to your local PC and try converting it.)

@yashi
Copy link
Member

yashi commented Oct 12, 2024

Just curious, do you know

  1. What is the native (raw) format of our camera?
  2. does the RPi Zero V4L2 driver support the exact same format? (V4L2 driver might convert to YUV or some sort)

@sasataku
Copy link
Member

Just curious, do you know
What is the native (raw) format of our camera?
does the RPi Zero V4L2 driver support the exact same format? (V4L2 driver might convert to YUV or some sort)

I don't have answers to these questions because I haven't investigate the Arducam camera driver.
(Especially regarding IPS)
Based on the catalog specifications, it is as follows, but I haven't looked into how the driver controls it by default.

https://www.arducam.com/product/arducam-full-hd-color-global-shutter-camera-for-raspberry-pi-2-3mp-ar0234-wide-angle-pivariety-camera-module-b0353/

  • ISP Output Format Image: JPG, YUV420, RAW, DNG/Video: MJPEG, H.264
  • Output Format RAW10

I would like to ask @moonlight83340 to look into these matters as well.

@yashi
Copy link
Member

yashi commented Oct 12, 2024

  • ISP Output Format Image: JPG, YUV420, RAW, DNG/Video: MJPEG, H.264

🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants