Address review comments

Author: RayyanAnsari

camera.c

@@ -21,7 +21,7 @@
 #include "camera.h"
 #include "device.h"
 
-int init_device(const char *dev_name, int *fd_ref, char **driver_ref)
+static int camera_init_device(const char *dev_name, int *fd_ref, char **driver_ref)
 {
 	*fd_ref = open(dev_name, O_RDWR);
 	if (*fd_ref < 0)
@@ -43,7 +43,7 @@ int init_device(const char *dev_name, int *fd_ref, char **driver_ref)
 	return 0;
 }
 
-int configure_format(const int fd, int *width_ref, int *height_ref, uint32_t *format_ref)
+static int camera_configure_format(const int fd, int *width_ref, int *height_ref, uint32_t *format_ref)
 {
 	struct v4l2_format fmt = {0};
 	fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
@@ -80,7 +80,7 @@ int configure_format(const int fd, int *width_ref, int *height_ref, uint32_t *fo
 	return 0;
 }
 
-int request_buffer(const int fd)
+static int camera_request_buffer(const int fd)
 {
 	struct v4l2_requestbuffers reqbuf = {0};
 	reqbuf.count = 1;
@@ -96,7 +96,7 @@ int request_buffer(const int fd)
 	return 0;
 }
 
-int query_buffer(const int fd, uint8_t **buf_ref, struct v4l2_buffer *buf_info_ref, int *size_ref)
+static int camera_query_buffer(const int fd, uint8_t **buf_ref, struct v4l2_buffer *buf_info_ref)
 {
 	buf_info_ref->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	buf_info_ref->memory = V4L2_MEMORY_MMAP;
@@ -115,12 +115,10 @@ int query_buffer(const int fd, uint8_t **buf_ref, struct v4l2_buffer *buf_info_r
 		return -1;
 	}
 
-	*size_ref = buf_info_ref->length;
-
 	return 0;
 }
 
-int capture_frame(const int fd, struct v4l2_buffer *buf_info)
+static int camera_capture_frame(const int fd, struct v4l2_buffer *buf_info)
 {
 	if (ioctl(fd, VIDIOC_QBUF, buf_info))
 	{
@@ -147,41 +145,12 @@ int capture_frame(const int fd, struct v4l2_buffer *buf_info)
 	return 0;
 }
 
-void yuyv_to_rgb(uint8_t *rgb_ref, const uint8_t *yuyv, const int width, const int height)
-{
-	int frame_size = width * height * 2;
-
-	for (int i = 0, j = 0; i < frame_size; i += 4, j += 6)
-	{
-		int Y0 = yuyv[i];
-		int U = yuyv[i + 1] - 128;
-		int Y1 = yuyv[i + 2];
-		int V = yuyv[i + 3] - 128;
-
-		int R1 = Y0 + 1.140 * V;
-		int G1 = Y0 - 0.395 * U - 0.581 * V;
-		int B1 = Y0 + 2.032 * U;
-
-		int R2 = Y1 + 1.140 * V;
-		int G2 = Y1 - 0.395 * U - 0.581 * V;
-		int B2 = Y1 + 2.032 * U;
-
-		rgb_ref[j + 0] = (uint8_t)(R1 < 0 ? 0 : (R1 > 255 ? 255 : R1));
-		rgb_ref[j + 1] = (uint8_t)(G1 < 0 ? 0 : (G1 > 255 ? 255 : G1));
-		rgb_ref[j + 2] = (uint8_t)(B1 < 0 ? 0 : (B1 > 255 ? 255 : B1));
-		rgb_ref[j + 3] = (uint8_t)(R2 < 0 ? 0 : (R2 > 255 ? 255 : R2));
-		rgb_ref[j + 4] = (uint8_t)(G2 < 0 ? 0 : (G2 > 255 ? 255 : G2));
-		rgb_ref[j + 5] = (uint8_t)(B2 < 0 ? 0 : (B2 > 255 ? 255 : B2));
-	}
-}
-
-void rgb_to_jpeg(uint8_t **jpeg_ref, unsigned long *size_ref, const uint8_t *rgb, const int width, const int height)
+static int camera_yuyv_to_jpeg(uint8_t **jpeg_ref, unsigned long *size_ref, const uint8_t *yuyv, const int width, const int height)
 {
 	struct jpeg_compress_struct cinfo;
 	struct jpeg_error_mgr jerr;
 
 	JSAMPROW row_pointer[1];
-	int row_stride;
 
 	cinfo.err = jpeg_std_error(&jerr);
 	jpeg_create_compress(&cinfo);
@@ -191,108 +160,113 @@ void rgb_to_jpeg(uint8_t **jpeg_ref, unsigned long *size_ref, const uint8_t *rgb
 	cinfo.image_width = width;
 	cinfo.image_height = height;
 	cinfo.input_components = 3;
-	cinfo.in_color_space = JCS_RGB;
+	cinfo.in_color_space = JCS_YCbCr;
 
 	jpeg_set_defaults(&cinfo);
-	jpeg_set_quality(&cinfo, 75, TRUE);
+	jpeg_set_quality(&cinfo, 100, TRUE);
 
 	jpeg_start_compress(&cinfo, TRUE);
 
-	row_stride = width * 3;
+	uint8_t* row_buffer = (uint8_t*)malloc(width * 3 * sizeof(uint8_t));
+	if (!row_buffer)
+	{
+		fprintf(stderr, "Memory allocation failed\n");
+		jpeg_destroy_compress(&cinfo);
+		return -1;
+	}
 
-	while (cinfo.next_scanline < cinfo.image_height)
+	while (cinfo.next_scanline < height)
 	{
-		row_pointer[0] = (JSAMPROW)&rgb[cinfo.next_scanline * row_stride];
+		const uint32_t offset = cinfo.next_scanline * width * 2;
+		for (uint32_t i = 0, j = 0; i < width * 2; i += 4, j += 6)
+		{
+			row_buffer[j + 0] = yuyv[offset + i + 0];
+			row_buffer[j + 1] = yuyv[offset + i + 1];
+			row_buffer[j + 2] = yuyv[offset + i + 3];
+			row_buffer[j + 3] = yuyv[offset + i + 2];
+			row_buffer[j + 4] = yuyv[offset + i + 1];
+			row_buffer[j + 5] = yuyv[offset + i + 3];
+		}
+		row_pointer[0] = row_buffer;
 		jpeg_write_scanlines(&cinfo, row_pointer, 1);
 	}
 
 	jpeg_finish_compress(&cinfo);
 	jpeg_destroy_compress(&cinfo);
+
+	free(row_buffer);
+
+	return 0;
 }
 
 int camera_capture_jpeg(uint8_t **buffer_ref, unsigned long *size_ref, const char *video_device)
 {
 	int fd = -1;
 	uint8_t *raw_buffer = NULL;
-	int raw_size = 0;
-
-	void cleanup()
-	{
-		if (fd >= 0)
-		{
-			close(fd);
-		}
-		if (raw_buffer != NULL)
-		{
-			munmap(raw_buffer, raw_size);
-		}
-	}
 
 	int ret = 0;
-	char *driver_str;
+	char *driver_str = NULL;
 
-	fprintf(stderr, "Opening device: %s\n", video_device);
-	ret = init_device(video_device, &fd, &driver_str);
+	int width, height = 0;
+	uint32_t format = 0;
+
+	struct v4l2_buffer buf_info = {0};
+
+	uint8_t *jpeg = NULL;
+
+	ret = camera_init_device(video_device, &fd, &driver_str);
 	if (ret < 0)
 	{
-		cleanup();
-		return -1;
+		goto cleanup;
 	}
 
-	int width, height;
-	uint32_t format;
-	ret = configure_format(fd, &width, &height, &format);
+	ret = camera_configure_format(fd, &width, &height, &format);
 	if (ret < 0)
 	{
-		cleanup();
-		return -1;
+		goto cleanup;
 	}
 
 	fprintf(stderr, "Driver: %s, Resolution: %dx%d, Format: %c%c%c%c\n", driver_str, width, height,
 		(char)((format >> 0) & 0xFF), (char)((format >> 8) & 0xFF), (char)((format >> 16) & 0xFF), (char)((format >> 24) & 0xFF));
 	free(driver_str);
 
-	ret = request_buffer(fd);
+	ret = camera_request_buffer(fd);
 	if (ret < 0)
 	{
-		cleanup();
-		return -1;
+		goto cleanup;
 	}
 
-	struct v4l2_buffer buf_info = {0};
-
-	ret = query_buffer(fd, &raw_buffer, &buf_info, &raw_size);
+	ret = camera_query_buffer(fd, &raw_buffer, &buf_info);
 	if (ret < 0)
 	{
-		cleanup();
-		return -1;
+		goto cleanup;
 	}
 
-	capture_frame(fd, &buf_info);
+	camera_capture_frame(fd, &buf_info);
 
-	uint8_t *jpeg = NULL;
 	switch (format)
 	{
 		case V4L2_PIX_FMT_YUYV:
-		{
-			uint8_t *rgb = (uint8_t *)malloc(width * height * 3);
-
-			yuyv_to_rgb(rgb, raw_buffer, width, height);
-			rgb_to_jpeg(&jpeg, size_ref, rgb, width, height);
-
-			free(rgb);
+			camera_yuyv_to_jpeg(&jpeg, size_ref, raw_buffer, width, height);
 			break;
-		}
 
 		default:
 			fprintf(stderr, "Unsupported format\n");
-			cleanup();
-			return -1;
+			ret = -1;
+			goto cleanup;
 	}
 
 	*buffer_ref = jpeg;
 
-	cleanup();
+cleanup:
+	if (fd >= 0)
+	{
+		close(fd);
+	}
+	if (raw_buffer != NULL)
+	{
+		munmap(raw_buffer, buf_info.length);
+	}
 
-	return 0;
+	return ret;
 }

cdba-server.c

@@ -118,7 +118,22 @@ static void capture_image(struct device *device)
 		return;
 	}
 
-	cdba_send_buf(MSG_CAPTURE_IMAGE, size, jpeg);
+	const size_t chunk_size = 1024;
+	size_t remaining_size = size;
+	const uint8_t *ptr = jpeg;
+
+	while (remaining_size > 0) {
+		size_t send_size = remaining_size > chunk_size ? chunk_size : remaining_size;
+
+		cdba_send_buf(MSG_CAPTURE_IMAGE, send_size, ptr);
+
+		ptr += send_size;
+		remaining_size -= send_size;
+	}
+
+	// Empty message to indicate end of image
+	cdba_send_buf(MSG_CAPTURE_IMAGE, 0, NULL);
+
 	free(jpeg);
 }
 
@@ -212,7 +227,7 @@ static int handle_stdin(int fd, void *buf)
 			capture_image(selected_device);
 			break;
 		default:
-			fprintf(stderr, "unk %d len %u\n", msg->type, msg->len);
+			fprintf(stderr, "unk %d len %d\n", msg->type, msg->len);
 			exit(1);
 		}
 

cdba.c

@@ -487,40 +487,41 @@ static void handle_console(const void *data, size_t len)
 
 static void handle_image_capture(void *data, size_t len)
 {
-	int fd;
-	int ret;
-
-	const char *image_dir = "/tmp/cdba-images";
+	static int fd = -1;
 	const char *filename = "capture.jpg";
+	int ret;
 
-	char image_path[PATH_MAX];
-	ret = snprintf(image_path, PATH_MAX, "%s/%s", image_dir, filename);
-	if (ret < 0)
+	if (len == 0)
 	{
-		err(1, "failed to build image path");
-	}
+		// End of image
+		if (fd >= 0)
+		{
+			close(fd);
+			fd = -1;
 
-	ret = mkdir(image_dir, 0700);
-	if (ret < 0 && errno != EEXIST)
-	{
-		err(1, "failed to create directory %s", image_dir);
+			fprintf(stderr, "Saved captured image to %s\n", filename);
+		}
+		return;
 	}
 
-	fd = open(image_path, O_CREAT | O_WRONLY | O_TRUNC, 0600);
-	if (fd < 0)
-	{
-		err(1, "failed to open %s", filename);
+	if (fd < 0) {
+		// First chunk of image
+		fd = open(filename, O_CREAT | O_WRONLY | O_TRUNC, 0666);
+		if (fd < 0) {
+			err(1, "Failed to open %s", filename);
+		}
 	}
 
-	ret = write(fd, data, len);
-	if (ret != len)
+	while (len > 0)
 	{
-		err(1, "failed to write image to %s", filename);
-	}
-
-	printf("Saved captured image to %s\n", image_path);
+		ret = write(fd, data, len);
+		if (ret < 0) {
+			err(1, "Failed to write to %s", filename);
+		}
 
-	close(fd);
+		data = (uint8_t *)data + ret;
+		len -= ret;
+	}
 }
 
 static bool auto_power_on;
@@ -603,7 +604,7 @@ static int handle_message(struct circ_buf *buf)
 			handle_image_capture(msg->data, msg->len);
 			break;
 		default:
-			fprintf(stderr, "unk %d len %u\n", msg->type, msg->len);
+			fprintf(stderr, "unk %d len %d\n", msg->type, msg->len);
 			return -1;
 		}
 

cdba.h

@@ -10,7 +10,7 @@
 
 struct msg {
 	uint8_t type;
-	uint32_t len;
+	uint16_t len;
 	uint8_t data[];
 } __packed;
 

circ_buf.h

@@ -13,7 +13,7 @@
 #define MIN(x, y) ((x) < (y) ? (x) : (y))
 #endif
 
-#define CIRC_BUF_SIZE 524288
+#define CIRC_BUF_SIZE 16384
 
 struct circ_buf {
 	char buf[CIRC_BUF_SIZE];