v4l2-ctl: Add support for CROP selection in m2m streaming

PATCH Superseded Dafna Hirschfeld 2 participants 9 messages Dec 18, 2018 – Dec 22, 2018 1 message not in the archive Media (V4L/DVB)

lore

Share & tools

lore

Dafna Hirschfeld

author

2018-12-18 11:12 UTC #0 lore raw

PATCH 1/1 v4l2-ctl: Add support for CROP selection in m2m streaming

Add support for crop in the selection api for m2m encoder. This includes reading and writing raw frames with padding.

Signed-off-by Dafna Hirschfeld <[email protected]>

---
Tested with the jellyfish video, with this script:
https://github.com/kamomil/outreachy/blob/master/test-v4l2-utils-with-jelly.sh
Tested on formats yuv420, rgb24, nv12

 utils/common/v4l-stream.h             |   2 +-
 utils/v4l2-ctl/v4l2-ctl-streaming.cpp | 257 +++++++++++++++++++++++++-
 utils/v4l2-ctl/v4l2-ctl-vidcap.cpp    |   6 +
 utils/v4l2-ctl/v4l2-ctl-vidout.cpp    |   5 +
 utils/v4l2-ctl/v4l2-ctl.h             |   2 +
 5 files changed, 261 insertions(+), 11 deletions(-)

utils/common/v4l-stream.h +1 -1

index c235150b..a03d4790 100644
@@ -9,11 +9,11 @@
 #define _V4L_STREAM_H_
 
 #include <linux/videodev2.h>
-#include <codec-v4l2-fwht.h>
 
 #ifdef __cplusplus
 extern "C" {
 #endif /* __cplusplus */
+#include <codec-v4l2-fwht.h>
 
 /* Default port */
 #define V4L_STREAM_PORT 8362

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +247 ~~-10~~

331 lines not on this page · raw message

utils/v4l2-ctl/v4l2-ctl-vidcap.cpp +6 -0

index dc17a868..932f1fd2 100644
@@ -244,6 +244,12 @@ void vidcap_get(cv4l_fd &fd)
 	}
 }
 
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+	r_height = height;
+	r_width = width;
+}
+
+
 void vidcap_list(cv4l_fd &fd)
 {
 	if (options[OptListFormats]) {

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +5 -0

index 5823df9c..05bd43ed 100644
@@ -208,6 +208,11 @@ void vidout_get(cv4l_fd &fd)
 	}
 }
 
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+	r_height = height;
+	r_width = width;
+}
+
 void vidout_list(cv4l_fd &fd)
 {
 	if (options[OptListOutFormats]) {

utils/v4l2-ctl/v4l2-ctl.h +2 -0

index 5a52a0a4..ab2994b2 100644
@@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg);
 void vidout_set(cv4l_fd &fd);
 void vidout_get(cv4l_fd &fd);
 void vidout_list(cv4l_fd &fd);
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
 
 // v4l2-ctl-overlay.cpp
 void overlay_usage(void);

Signature · 1 line

2.17.1

Hans Verkuil

2018-12-18 12:43 UTC #1 lore raw

Quoted 21 lines · Dafna Hirschfeld

Add support for crop in the selection api for m2m encoder. This includes reading and writing raw frames with padding.
Signed-off-by Dafna Hirschfeld <[email protected]>
---
Tested with the jellyfish video, with this script:
https://github.com/kamomil/outreachy/blob/master/test-v4l2-utils-with-jelly.sh
Tested on formats yuv420, rgb24, nv12
 utils/common/v4l-stream.h             |   2 +-
 utils/v4l2-ctl/v4l2-ctl-streaming.cpp | 257 +++++++++++++++++++++++++-
 utils/v4l2-ctl/v4l2-ctl-vidcap.cpp    |   6 +
 utils/v4l2-ctl/v4l2-ctl-vidout.cpp    |   5 +
 utils/v4l2-ctl/v4l2-ctl.h             |   2 +
 5 files changed, 261 insertions(+), 11 deletions(-)
utils/common/v4l-stream.h +0 -0
index c235150b..a03d4790 100644

utils/common/v4l-stream.h line 15

Quoted 8 lines

utils/common/v4l-stream.h +0 -1

@@ -9,11 +9,11 @@
 #define _V4L_STREAM_H_
 
 #include <linux/videodev2.h>
-#include <codec-v4l2-fwht.h>
 
 #ifdef __cplusplus
 extern "C" {
 #endif /* __cplusplus */

Add an empty line here.

Quoted 9 lines

utils/common/v4l-stream.h +1 -0
+#include <codec-v4l2-fwht.h>
 
 /* Default port */
 #define V4L_STREAM_PORT 8362
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -0
index dee104d7..759577dd 100644

utils/v4l2-ctl/v4l2-ctl-streaming.cpp line 25

Quoted 7 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -20,9 +20,9 @@
 
 #include "v4l2-ctl.h"
 #include "v4l-stream.h"
-#include "codec-fwht.h"
 
 extern "C" {
+#include "codec-v4l2-fwht.h"

No need for this since it is already included by v4l-stream.h

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static unsigned bpl_out[VIDEO_MAX_PLANES]; · line 79

Quoted 11 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +4 -0

 #include "v4l2-tpg.h"
 }

@@ -73,6 +73,12 @@ static unsigned bpl_out[VIDEO_MAX_PLANES];
 static bool last_buffer = false;
 static codec_ctx *ctx;
 
+static unsigned int visible_width = 0;
+static unsigned int visible_height = 0;
+static unsigned int frame_width = 0;
+static unsigned int frame_height = 0;

What is 'frame_width/height'? Is that what VIDIOC_G_FMT returns?

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static unsigned bpl_out[VIDEO_MAX_PLANES]; · line 80

Quoted 1 line

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+bool is_m2m_enc = false;

This should be static.

I'm assuming that in a future patch we'll get a is_m2m_dec as well?

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 117

Quoted 9 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0

+
 #define TS_WINDOW 241
 #define FILE_HDR_ID			v4l2_fourcc('V', 'h', 'd', 'r')

@@ -108,6 +114,84 @@ public:
 	unsigned dropped();
 };
 
+static int get_codec_type(int fd, bool &is_enc) {

{ on the next line.

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 120

Quoted 3 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +3 -0
+	struct v4l2_capability vcap;
+
+	memset(&vcap,0,sizeof(vcap));

Space after ,

Please use the kernel coding style for these v4l utilities.

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 122

Quoted 2 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+
+	int ret = ioctl(fd, VIDIOC_QUERYCAP, &vcap);

Please use the cv4l_fd class. It comes with lots of helpers for all these ioctls and it already used in v4l2-ctl-streaming.cpp.

In this function you can just do:

	if (!fd.has_vid_m2m())
		return -1;

Quoted 13 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +13 -0

+	if(ret) {
+		fprintf(stderr, "get_codec_type: VIDIOC_QUERYCAP failed: %d\n", ret);
+		return ret;
+	}
+	unsigned int caps = vcap.capabilities;
+	if (caps & V4L2_CAP_DEVICE_CAPS)
+		caps = vcap.device_caps;
+	if(!(caps & V4L2_CAP_VIDEO_M2M) && !(caps & V4L2_CAP_VIDEO_M2M_MPLANE)) {
+		is_enc = false;
+		fprintf(stderr,"get_codec_type: not an M2M device\n");
+		return -1;
+	}
+

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 145

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+	struct v4l2_fmtdesc fmt;
+	memset(&fmt,0,sizeof(fmt));
+	fmt.index = 0;
+	fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+
+	while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+		if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+			break;
+		fmt.index++;
+	}

These tests aren't good enough. You need to enumerate over all formats. Easiest is to keep a tally of the total number of formats and how many are compressed.

An encoder is a device where all output formats are uncompressed and all capture formats are compressed. It's the reverse for a decoder.

If you get a mix on either side, or both sides are raw or both sides are compressed, then it isn't a codec.

Quoted 61 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +49 -1

+	if (ret) {
+		is_enc = true;
+		return 0;
+	}
+	memset(&fmt,0,sizeof(fmt));
+	fmt.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+		if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+			break;
+		fmt.index++;
+	}
+	if (ret) {
+		is_enc = false;
+		return 0;
+	}
+	fprintf(stderr, "get_codec_type: could no determine codec type\n");
+	return -1;
+}
+
+static void get_frame_dims(unsigned int &frame_width, unsigned int &frame_height) {
+
+	if(is_m2m_enc)
+		vidout_get_orig_from_set(frame_width, frame_height);
+	else
+		vidcap_get_orig_from_set(frame_width, frame_height);
+}
+
+static int get_visible_format(int fd, unsigned int &width, unsigned int &height) {
+	int ret = 0;
+	if(is_m2m_enc) {
+		struct v4l2_selection in_selection;
+
+		in_selection.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+		in_selection.target = V4L2_SEL_TGT_CROP;
+
+		if ( (ret = ioctl(fd, VIDIOC_G_SELECTION, &in_selection)) != 0) {
+			fprintf(stderr,"get_visible_format: error in g_selection ioctl: %d\n",ret);
+			return ret;
+		}
+		width = in_selection.r.width;
+		height = in_selection.r.height;
+	}
+	else { //TODO - g_selection with COMPOSE should be used here when implemented in driver
+		vidcap_get_orig_from_set(width, height);
+	}
+	return 0;
+}
+
+
 void fps_timestamps::determine_field(int fd, unsigned type)
 {
 	struct v4l2_format fmt = { };

@@ -419,7 +503,6 @@ static void print_buffer(FILE *f, struct v4l2_buffer &buf)
 			fprintf(f, "\t\tData Offset: %u\n", p->data_offset);
 		}
 	}
-			
 	fprintf(f, "\n");
 }

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 743

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -657,7 +740,131 @@ void streaming_cmd(int ch, char *optarg)
 	}
 }
 
-static bool fill_buffer_from_file(cv4l_queue &q, cv4l_buffer &b, FILE *fin)
+bool padding(cv4l_fd &fd, cv4l_queue &q, unsigned char* buf, FILE *fpointer, unsigned &sz, unsigned &len, bool is_read)

This should definitely be a static function. Also, this is not a very good name.

Why not call it fill_padded_buffer_from_file()?

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 745

Quoted 2 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+{
+	cv4l_fmt fmt(q.g_type());

No need to use q.g_type(). 'cv4l_fmt fmt;' is sufficient.

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 746

Quoted 1 line

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+	fd.g_fmt(fmt, q.g_type());

After all, it's filled here.

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 747

Quoted 1 line

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+	const struct v4l2_fwht_pixfmt_info *vic_fmt = v4l2_fwht_find_pixfmt(fmt.g_pixelformat());

This test should be moved to fill_buffer_from_file. If it is not an encoder and the pixelformat is not known (v4l2_fwht_find_pixfmt() returns NULL), then it should fallback to the old behavior. So this function should only be called when you have all the information about the pixelformat.

Quoted 21 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +21 -0

+	unsigned coded_width = fmt.g_width();
+	unsigned coded_height = fmt.g_height();
+	unsigned real_width;
+	unsigned real_height;
+	unsigned char *buf_p = (unsigned char*) buf;
+
+	if(is_read) {
+		real_width  = frame_width;
+		real_height = frame_height;
+	}
+	else {
+		real_width  = visible_width;
+		real_height = visible_height;
+	}
+	sz = 0;
+	len = real_width * real_height * vic_fmt->sizeimage_mult / vic_fmt->sizeimage_div;
+	switch(vic_fmt->id) {
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_VYUY:

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 778

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+	case V4L2_PIX_FMT_RGB24:
+	case V4L2_PIX_FMT_HSV24:
+	case V4L2_PIX_FMT_BGR24:
+	case V4L2_PIX_FMT_RGB32:
+	case V4L2_PIX_FMT_XRGB32:
+	case V4L2_PIX_FMT_HSV32:
+	case V4L2_PIX_FMT_BGR32:
+	case V4L2_PIX_FMT_XBGR32:
+	case V4L2_PIX_FMT_ARGB32:
+	case V4L2_PIX_FMT_ABGR32:

I'd put this all under a 'default' case. I think GREY can also be added here.

What I would really like to see is that only the information from v4l2_fwht_pixfmt_info can be used here without requiring a switch.

I think all that is needed to do that is that struct v4l2_fwht_pixfmt_info is extended with a 'planes_num' field, which is 1 for interleaved formats, 2 for luma/interleaved chroma planar formats and 3 for luma/cr/cb planar formats.

Quoted 119 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +99 -2

+		for(unsigned i=0; i < real_height; i++) {
+			unsigned int consume_sz = vic_fmt->bytesperline_mult*real_width;
+			unsigned int wsz = 0;
+			if(is_read)
+				wsz = fread(buf_p, 1, consume_sz, fpointer);
+			else
+				wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+			sz += wsz;
+			if(wsz == 0 && i == 0)
+				break;
+			if(wsz != consume_sz) {
+				fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+				return false;
+			}
+			buf_p += vic_fmt->chroma_step*coded_width;
+		}
+	break;
+
+	case V4L2_PIX_FMT_NV12:
+	case V4L2_PIX_FMT_NV16:
+	case V4L2_PIX_FMT_NV24:
+	case V4L2_PIX_FMT_NV21:
+	case V4L2_PIX_FMT_NV61:
+	case V4L2_PIX_FMT_NV42:
+		for(unsigned plane_idx = 0; plane_idx < 2; plane_idx++) {
+			unsigned h_div = (plane_idx == 0) ? 1 : vic_fmt->height_div;
+			unsigned w_div = (plane_idx == 0) ? 1 : vic_fmt->width_div;
+			unsigned step  =  (plane_idx == 0) ? vic_fmt->luma_alpha_step : vic_fmt->chroma_step;
+
+			for(unsigned i=0; i <  real_height/h_div; i++) {
+				unsigned int wsz = 0;
+				unsigned int consume_sz = step * real_width / w_div;
+				if(is_read)
+					wsz = fread(buf_p, 1,  consume_sz, fpointer);
+				else
+					wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+				if(wsz == 0 && i == 0 && plane_idx == 0)
+					break;
+				if(wsz != consume_sz) {
+					fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+					return true;
+				}
+				sz += wsz;
+				buf_p += step*coded_width/w_div;
+			}
+			buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+			if(sz == 0)
+				break;
+		}
+	break;
+	case V4L2_PIX_FMT_YUV420:
+	case V4L2_PIX_FMT_YUV422P:
+	case V4L2_PIX_FMT_YVU420:
+	case V4L2_PIX_FMT_GREY:
+		for(unsigned comp_idx = 0; comp_idx < vic_fmt->components_num; comp_idx++) {
+			unsigned h_div = (comp_idx == 0) ? 1 : vic_fmt->height_div;
+			unsigned w_div = (comp_idx == 0) ? 1 : vic_fmt->width_div;
+
+			for(unsigned i=0; i < real_height/h_div; i++) {
+				unsigned int wsz = 0;
+				unsigned int consume_sz = real_width/w_div;
+				if(is_read)
+					wsz = fread(buf_p, 1, consume_sz, fpointer);
+				else
+					wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+				if(wsz == 0 && i == 0 && comp_idx == 0)
+					break;
+				if(wsz != consume_sz) {
+					fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+					return true;
+				}
+				sz += wsz;
+				buf_p += coded_width/w_div;
+			}
+			buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+			if(sz == 0)
+				break;
+		}
+		break;
+	default:
+		fprintf(stderr,"the format is not supported yet\n");
+		return false;
+	}
+	return true;
+}
+
+static bool fill_buffer_from_file(cv4l_fd &fd, cv4l_queue &q, cv4l_buffer &b, FILE *fin)
 {
 	static bool first = true;
 	static bool is_fwht = false;

@@ -785,7 +992,15 @@ restart:
 				return false;
 			}
 		}
-		sz = fread(buf, 1, len, fin);
+
+		if(is_m2m_enc) {
+			if(!padding(fd, q, (unsigned char*) buf, fin, sz, len, true))
+				return false;
+		}
+		else {
+			sz = fread(buf, 1, len, fin);
+		}
+
 		if (first && sz != len) {
 			fprintf(stderr, "Insufficient data\n");
 			return false;

@@ -908,7 +1123,7 @@ static int do_setup_out_buffers(cv4l_fd &fd, cv4l_queue &q, FILE *fin, bool qbuf
 					tpg_fillbuffer(&tpg, stream_out_std, j, (u8 *)q.g_dataptr(i, j));
 			}
 		}
-		if (fin && !fill_buffer_from_file(q, buf, fin))
+		if (fin && !fill_buffer_from_file(fd, q, buf, fin))
 			return -2;
 
 		if (qbuf) {

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1178

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -960,7 +1175,7 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
 		if (fd.qbuf(buf))
 			return -1;
 	}
-	
+

Seems to be a whitespace only change, just drop this change.

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -1

 	double ts_secs = buf.g_timestamp().tv_sec + buf.g_timestamp().tv_usec / 1000000.0;
 	fps_ts.add_ts(ts_secs, buf.g_sequence(), buf.g_field());

@@ -1023,8 +1238,15 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
 			}
 			if (host_fd_to >= 0)
 				sz = fwrite(comp_ptr[j] + offset, 1, used, fout);
-			else

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1249

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +9 -1

-				sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+			else {
+				if(!is_m2m_enc) {
+					if(!padding(fd, q, (u8 *)q.g_dataptr(buf.g_index(), j) + offset, fout, sz, used, false))
+						return false;
+				}
+				else {
+					sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+				}
+			}

This doesn't feel right.

I think a write_buffer_to_file() function should be introduced that deals with these variations.

Quoted 65 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +17 -2

 			if (sz != used)
 				fprintf(stderr, "%u != %u\n", sz, used);

@@ -1130,7 +1352,7 @@ static int do_handle_out(cv4l_fd &fd, cv4l_queue &q, FILE *fin, cv4l_buffer *cap
 			output_field = V4L2_FIELD_TOP;
 	}
 
-	if (fin && !fill_buffer_from_file(q, buf, fin))
+	if (fin && !fill_buffer_from_file(fd, q, buf, fin))
 		return -2;
 
 	if (!fin && stream_out_refresh) {

@@ -1227,7 +1449,7 @@ static void streaming_set_cap(cv4l_fd &fd)
 		}
 		break;
 	}
-	
+
 	memset(&sub, 0, sizeof(sub));
 	sub.type = V4L2_EVENT_EOS;
 	fd.subscribe_event(sub);

@@ -2031,6 +2253,21 @@ void streaming_set(cv4l_fd &fd, cv4l_fd &out_fd)
 	int do_cap = options[OptStreamMmap] + options[OptStreamUser] + options[OptStreamDmaBuf];
 	int do_out = options[OptStreamOutMmap] + options[OptStreamOutUser] + options[OptStreamOutDmaBuf];
 
+	int r = get_codec_type(fd.g_fd(), is_m2m_enc);
+	if(r) {
+		fprintf(stderr, "error checking codec type\n");
+		return;
+	}
+
+	r = get_visible_format(fd.g_fd(), visible_width, visible_height);
+
+	if(r) {
+		fprintf(stderr, "error getting the visible width\n");
+		return;
+	}
+
+	get_frame_dims(frame_width, frame_height);
+
 	if (out_fd.g_fd() < 0) {
 		out_capabilities = capabilities;
 		out_priv_magic = priv_magic;

utils/v4l2-ctl/v4l2-ctl-vidcap.cpp +6 -0

index dc17a868..932f1fd2 100644
@@ -244,6 +244,12 @@ void vidcap_get(cv4l_fd &fd)
 	}
 }
 
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+	r_height = height;
+	r_width = width;
+}
+
+
 void vidcap_list(cv4l_fd &fd)
 {
 	if (options[OptListFormats]) {

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +0 -0

index 5823df9c..05bd43ed 100644

utils/v4l2-ctl/v4l2-ctl-vidout.cpp void vidout_get(cv4l_fd &fd) · line 214

Quoted 7 lines

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +4 -0

@@ -208,6 +208,11 @@ void vidout_get(cv4l_fd &fd)
 	}
 }
 
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+	r_height = height;
+	r_width = width;
+}

Don't do this (same for vidcap_get_orig_from_set).

I think you just want to get the width and height from VIDIOC_G_FMT here, so why not just call that?

Remember that you can call v4l2-ctl without setting the output width and height if the defaults that the driver sets are already fine. In that case the width and height variables in this source are just 0.

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +1 -0
+
 void vidout_list(cv4l_fd &fd)
 {
 	if (options[OptListOutFormats]) {
utils/v4l2-ctl/v4l2-ctl.h +0 -0
index 5a52a0a4..ab2994b2 100644

utils/v4l2-ctl/v4l2-ctl.h void vidout_cmd(int ch, char *optarg); · line 364

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl.h +2 -0

@@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg);
 void vidout_set(cv4l_fd &fd);
 void vidout_get(cv4l_fd &fd);
 void vidout_list(cv4l_fd &fd);
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
 
 // v4l2-ctl-overlay.cpp
 void overlay_usage(void);

This patch needs more work (not surprisingly, since it takes a bit of time to understand the v4l2-ctl source code).

Please stick to the kernel coding style! Using a different style makes it harder for me to review since my pattern matches routines in my brain no longer work optimally. It's like reading text with spelling mistakes, you cn stil undrstant iT, but it tekes moore teem. :-)

Regards,

Hans

Dafna Hirschfeld

author

2018-12-19 08:34 UTC #2 lore raw

Quoted 60 lines · Hans Verkuil

Quoted 21 lines · Dafna Hirschfeld
Add support for crop in the selection api for m2m encoder. This includes reading and writing raw frames with padding.
Signed-off-by Dafna Hirschfeld <[email protected]>
---
Tested with the jellyfish video, with this script:
https://github.com/kamomil/outreachy/blob/master/test-v4l2-utils-with-jelly.sh
Tested on formats yuv420, rgb24, nv12
 utils/common/v4l-stream.h             |   2 +-
 utils/v4l2-ctl/v4l2-ctl-streaming.cpp | 257 +++++++++++++++++++++++++-
 utils/v4l2-ctl/v4l2-ctl-vidcap.cpp    |   6 +
 utils/v4l2-ctl/v4l2-ctl-vidout.cpp    |   5 +
 utils/v4l2-ctl/v4l2-ctl.h             |   2 +
 5 files changed, 261 insertions(+), 11 deletions(-)
utils/common/v4l-stream.h +0 -0
index c235150b..a03d4790 100644
utils/common/v4l-stream.h line 15
Quoted 8 lines
utils/common/v4l-stream.h +0 -1
@@ -9,11 +9,11 @@
 #define _V4L_STREAM_H_
 
 #include <linux/videodev2.h>
-#include <codec-v4l2-fwht.h>
 
 #ifdef __cplusplus
 extern "C" {
 #endif /* __cplusplus */
Add an empty line here.
Quoted 9 lines
utils/common/v4l-stream.h +1 -0
+#include <codec-v4l2-fwht.h>
 
 /* Default port */
 #define V4L_STREAM_PORT 8362
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -0
index dee104d7..759577dd 100644
utils/v4l2-ctl/v4l2-ctl-streaming.cpp line 25
Quoted 7 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1
@@ -20,9 +20,9 @@
 
 #include "v4l2-ctl.h"
 #include "v4l-stream.h"
-#include "codec-fwht.h"
 
 extern "C" {
+#include "codec-v4l2-fwht.h"
No need for this since it is already included by v4l-stream.h
Quoted 7 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -0
 #include "v4l2-tpg.h"
 }
 
@@ -73,6 +73,12 @@ static unsigned bpl_out[VIDEO_MAX_PLANES];
 static bool last_buffer = false;
 static codec_ctx *ctx;
 

Quoted 6 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static unsigned bpl_out[VIDEO_MAX_PLANES]; · line 79
Quoted 4 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +4 -0
+static unsigned int visible_width = 0;
+static unsigned int visible_height = 0;
+static unsigned int frame_width = 0;
+static unsigned int frame_height = 0;
What is 'frame_width/height'? Is that what VIDIOC_G_FMT returns?

Those are the actual dimensions of the image as given from the user command. It is needed for reading from raw video file, line by line. Actually these vars are used only by the encoder and the visible_width/height are used only by the decoder, so I can drop frame_width/height and use the visible for both

Quoted 6 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static unsigned bpl_out[VIDEO_MAX_PLANES]; · line 80
Quoted 2 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+bool is_m2m_enc = false;
This should be static.
I'm assuming that in a future patch we'll get a is_m2m_dec as well?

I forgot that there can be more options other than m2m_enc/_dec. So actually adding is_m2m_dec is needed. Or I can define an enum with 3 possible values
IS_M2M_ENC, IS_M2M_DEC, NOT_M2M_DEV

Quoted 13 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 117
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+
 #define TS_WINDOW 241
 #define FILE_HDR_ID                  v4l2_fourcc('V', 'h', 'd', 'r')
 
@@ -108,6 +114,84 @@ public:
      unsigned dropped();
 };
 
+static int get_codec_type(int fd, bool &is_enc) {
{ on the next line.

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 120
Quoted 3 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +3 -0
+     struct v4l2_capability vcap;
+
+     memset(&vcap,0,sizeof(vcap));
Space after ,
Please use the kernel coding style for these v4l utilities.

I ran the checkpatch script on this file and it didn't catch theses things. Do you use checkpatch for v4l-utils ?

Quoted 109 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 122

Quoted 2 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+
+     int ret = ioctl(fd, VIDIOC_QUERYCAP, &vcap);

Please use the cv4l_fd class. It comes with lots of helpers for all these ioctls and it already used in v4l2-ctl-streaming.cpp.

In this function you can just do:

        if (!fd.has_vid_m2m())
                return -1;

Quoted 13 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +13 -0

+     if(ret) {
+             fprintf(stderr, "get_codec_type: VIDIOC_QUERYCAP failed: %d\n", ret);
+             return ret;
+     }
+     unsigned int caps = vcap.capabilities;
+     if (caps & V4L2_CAP_DEVICE_CAPS)
+             caps = vcap.device_caps;
+     if(!(caps & V4L2_CAP_VIDEO_M2M) && !(caps & V4L2_CAP_VIDEO_M2M_MPLANE)) {
+             is_enc = false;
+             fprintf(stderr,"get_codec_type: not an M2M device\n");
+             return -1;
+     }
+

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 145

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+     struct v4l2_fmtdesc fmt;
+     memset(&fmt,0,sizeof(fmt));
+     fmt.index = 0;
+     fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }

These tests aren't good enough. You need to enumerate over all formats. Easiest is to keep a tally of the total number of formats and how many are compressed.

An encoder is a device where all output formats are uncompressed and all capture formats are compressed. It's the reverse for a decoder.

If you get a mix on either side, or both sides are raw or both sides are compressed, then it isn't a codec.

Quoted 64 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +49 -1

+     if (ret) {
+             is_enc = true;
+             return 0;
+     }
+     memset(&fmt,0,sizeof(fmt));
+     fmt.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }
+     if (ret) {
+             is_enc = false;
+             return 0;
+     }
+     fprintf(stderr, "get_codec_type: could no determine codec type\n");
+     return -1;
+}
+
+static void get_frame_dims(unsigned int &frame_width, unsigned int &frame_height) {
+
+     if(is_m2m_enc)
+             vidout_get_orig_from_set(frame_width, frame_height);
+     else
+             vidcap_get_orig_from_set(frame_width, frame_height);
+}
+
+static int get_visible_format(int fd, unsigned int &width, unsigned int &height) {
+     int ret = 0;
+     if(is_m2m_enc) {
+             struct v4l2_selection in_selection;
+
+             in_selection.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+             in_selection.target = V4L2_SEL_TGT_CROP;
+
+             if ( (ret = ioctl(fd, VIDIOC_G_SELECTION, &in_selection)) != 0) {
+                     fprintf(stderr,"get_visible_format: error in g_selection ioctl: %d\n",ret);
+                     return ret;
+             }
+             width = in_selection.r.width;
+             height = in_selection.r.height;
+     }
+     else { //TODO - g_selection with COMPOSE should be used here when implemented in driver
+             vidcap_get_orig_from_set(width, height);
+     }
+     return 0;
+}
+
+
 void fps_timestamps::determine_field(int fd, unsigned type)
 {
      struct v4l2_format fmt = { };

@@ -419,7 +503,6 @@ static void print_buffer(FILE *f, struct v4l2_buffer &buf)
                      fprintf(f, "\t\tData Offset: %u\n", p->data_offset);
              }
      }
-
      fprintf(f, "\n");
 }

@@ -657,7 +740,131 @@ void streaming_cmd(int ch, char *optarg)
      }
 }

Quoted 6 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 743
Quoted 2 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1
-static bool fill_buffer_from_file(cv4l_queue &q, cv4l_buffer &b, FILE *fin)
+bool padding(cv4l_fd &fd, cv4l_queue &q, unsigned char* buf, FILE *fpointer, unsigned &sz, unsigned &len, bool is_read)
This should definitely be a static function. Also, this is not a very good name.
Why not call it fill_padded_buffer_from_file()?

This function is used both for reading from file for the encoder and writing to file for the decoder.
Maybe it can be called read_write_padded_frame ?

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 745
Quoted 3 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+{
+     cv4l_fmt fmt(q.g_type());
No need to use q.g_type(). 'cv4l_fmt fmt;' is sufficient.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 746
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     fd.g_fmt(fmt, q.g_type());

Quoted 9 lines

After all, it's filled here.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 747
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     const struct v4l2_fwht_pixfmt_info *vic_fmt = v4l2_fwht_find_pixfmt(fmt.g_pixelformat());
This test should be moved to fill_buffer_from_file. If it is not an encoder and the pixelformat is not known (v4l2_fwht_find_pixfmt() returns NULL), then it should fallback to the old behavior. So this function should only be called when you have all the information about the pixelformat.

This function is supposed to be called only for m2m encoder on the output buffer and m2m decoder on the capture buffer
so vic_format is not NULL in those case.

Quoted 32 lines

Quoted 21 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +21 -0

+     unsigned coded_width = fmt.g_width();
+     unsigned coded_height = fmt.g_height();
+     unsigned real_width;
+     unsigned real_height;
+     unsigned char *buf_p = (unsigned char*) buf;
+
+     if(is_read) {
+             real_width  = frame_width;
+             real_height = frame_height;
+     }
+     else {
+             real_width  = visible_width;
+             real_height = visible_height;
+     }
+     sz = 0;
+     len = real_width * real_height * vic_fmt->sizeimage_mult / vic_fmt->sizeimage_div;
+     switch(vic_fmt->id) {
+     case V4L2_PIX_FMT_YUYV:
+     case V4L2_PIX_FMT_YVYU:
+     case V4L2_PIX_FMT_UYVY:
+     case V4L2_PIX_FMT_VYUY:

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 778

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+     case V4L2_PIX_FMT_RGB24:
+     case V4L2_PIX_FMT_HSV24:
+     case V4L2_PIX_FMT_BGR24:
+     case V4L2_PIX_FMT_RGB32:
+     case V4L2_PIX_FMT_XRGB32:
+     case V4L2_PIX_FMT_HSV32:
+     case V4L2_PIX_FMT_BGR32:
+     case V4L2_PIX_FMT_XBGR32:
+     case V4L2_PIX_FMT_ARGB32:
+     case V4L2_PIX_FMT_ABGR32:

Quoted 9 lines

I'd put this all under a 'default' case. I think GREY can also be added here.
What I would really like to see is that only the information from v4l2_fwht_pixfmt_info can be used here without requiring a switch.
I think all that is needed to do that is that struct v4l2_fwht_pixfmt_info is extended with a 'planes_num' field, which is 1 for interleaved formats, 2 for luma/interleaved chroma planar formats and 3 for luma/cr/cb planar formats.

So I should send a patch to the kernel code, adding this field ?

Quoted 148 lines

Quoted 119 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +99 -2

+             for(unsigned i=0; i < real_height; i++) {
+                     unsigned int consume_sz = vic_fmt->bytesperline_mult*real_width;
+                     unsigned int wsz = 0;
+                     if(is_read)
+                             wsz = fread(buf_p, 1, consume_sz, fpointer);
+                     else
+                             wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                     sz += wsz;
+                     if(wsz == 0 && i == 0)
+                             break;
+                     if(wsz != consume_sz) {
+                             fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                             return false;
+                     }
+                     buf_p += vic_fmt->chroma_step*coded_width;
+             }
+     break;
+
+     case V4L2_PIX_FMT_NV12:
+     case V4L2_PIX_FMT_NV16:
+     case V4L2_PIX_FMT_NV24:
+     case V4L2_PIX_FMT_NV21:
+     case V4L2_PIX_FMT_NV61:
+     case V4L2_PIX_FMT_NV42:
+             for(unsigned plane_idx = 0; plane_idx < 2; plane_idx++) {
+                     unsigned h_div = (plane_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (plane_idx == 0) ? 1 : vic_fmt->width_div;
+                     unsigned step  =  (plane_idx == 0) ? vic_fmt->luma_alpha_step : vic_fmt->chroma_step;
+
+                     for(unsigned i=0; i <  real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = step * real_width / w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1,  consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && plane_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += step*coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+     break;
+     case V4L2_PIX_FMT_YUV420:
+     case V4L2_PIX_FMT_YUV422P:
+     case V4L2_PIX_FMT_YVU420:
+     case V4L2_PIX_FMT_GREY:
+             for(unsigned comp_idx = 0; comp_idx < vic_fmt->components_num; comp_idx++) {
+                     unsigned h_div = (comp_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (comp_idx == 0) ? 1 : vic_fmt->width_div;
+
+                     for(unsigned i=0; i < real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = real_width/w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1, consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && comp_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+             break;
+     default:
+             fprintf(stderr,"the format is not supported yet\n");
+             return false;
+     }
+     return true;
+}
+
+static bool fill_buffer_from_file(cv4l_fd &fd, cv4l_queue &q, cv4l_buffer &b, FILE *fin)
 {
      static bool first = true;
      static bool is_fwht = false;

@@ -785,7 +992,15 @@ restart:
                              return false;
                      }
              }
-             sz = fread(buf, 1, len, fin);
+
+             if(is_m2m_enc) {
+                     if(!padding(fd, q, (unsigned char*) buf, fin, sz, len, true))
+                             return false;
+             }
+             else {
+                     sz = fread(buf, 1, len, fin);
+             }
+
              if (first && sz != len) {
                      fprintf(stderr, "Insufficient data\n");
                      return false;

@@ -908,7 +1123,7 @@ static int do_setup_out_buffers(cv4l_fd &fd, cv4l_queue &q, FILE *fin, bool qbuf
                                      tpg_fillbuffer(&tpg, stream_out_std, j, (u8 *)q.g_dataptr(i, j));
                      }
              }
-             if (fin && !fill_buffer_from_file(q, buf, fin))
+             if (fin && !fill_buffer_from_file(fd, q, buf, fin))
                      return -2;
 
              if (qbuf) {

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1178

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -960,7 +1175,7 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
              if (fd.qbuf(buf))
                      return -1;
      }
-
+

Seems to be a whitespace only change, just drop this change.

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -1

      double ts_secs = buf.g_timestamp().tv_sec + buf.g_timestamp().tv_usec / 1000000.0;
      fps_ts.add_ts(ts_secs, buf.g_sequence(), buf.g_field());

@@ -1023,8 +1238,15 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
                      }
                      if (host_fd_to >= 0)
                              sz = fwrite(comp_ptr[j] + offset, 1, used, fout);
-                     else

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1249

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +9 -1

-                             sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                     else {
+                             if(!is_m2m_enc) {
+                                     if(!padding(fd, q, (u8 *)q.g_dataptr(buf.g_index(), j) + offset, fout, sz, used, false))
+                                             return false;
+                             }
+                             else {
+                                     sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                             }
+                     }

This doesn't feel right.

Quoted 2 lines

I think a write_buffer_to_file() function should be introduced that deals with these variations.

Not sure what you meant, should I implement this if-else in another function ? The "padding" function is used both for reading and writing to/from padded buffer.
The condition for calling it here should be changed to "if(is_m2m_dec)" in which case "padding" will
write a raw frame to a file from the padded capture buffer.

Quoted 76 lines

Quoted 66 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +17 -2

                      if (sz != used)
                              fprintf(stderr, "%u != %u\n", sz, used);

@@ -1130,7 +1352,7 @@ static int do_handle_out(cv4l_fd &fd, cv4l_queue &q, FILE *fin, cv4l_buffer *cap
                      output_field = V4L2_FIELD_TOP;
      }
 
-     if (fin && !fill_buffer_from_file(q, buf, fin))
+     if (fin && !fill_buffer_from_file(fd, q, buf, fin))
              return -2;
 
      if (!fin && stream_out_refresh) {

@@ -1227,7 +1449,7 @@ static void streaming_set_cap(cv4l_fd &fd)
              }
              break;
      }
-
+
      memset(&sub, 0, sizeof(sub));
      sub.type = V4L2_EVENT_EOS;
      fd.subscribe_event(sub);

@@ -2031,6 +2253,21 @@ void streaming_set(cv4l_fd &fd, cv4l_fd &out_fd)
      int do_cap = options[OptStreamMmap] + options[OptStreamUser] + options[OptStreamDmaBuf];
      int do_out = options[OptStreamOutMmap] + options[OptStreamOutUser] + options[OptStreamOutDmaBuf];
 
+     int r = get_codec_type(fd.g_fd(), is_m2m_enc);
+     if(r) {
+             fprintf(stderr, "error checking codec type\n");
+             return;
+     }
+
+     r = get_visible_format(fd.g_fd(), visible_width, visible_height);
+
+     if(r) {
+             fprintf(stderr, "error getting the visible width\n");
+             return;
+     }
+
+     get_frame_dims(frame_width, frame_height);
+
      if (out_fd.g_fd() < 0) {
              out_capabilities = capabilities;
              out_priv_magic = priv_magic;

utils/v4l2-ctl/v4l2-ctl-vidcap.cpp +6 -0

index dc17a868..932f1fd2 100644
@@ -244,6 +244,12 @@ void vidcap_get(cv4l_fd &fd)
      }
 }
 
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}
+
+
 void vidcap_list(cv4l_fd &fd)
 {
      if (options[OptListFormats]) {

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +0 -0

index 5823df9c..05bd43ed 100644

utils/v4l2-ctl/v4l2-ctl-vidout.cpp void vidout_get(cv4l_fd &fd) · line 214

Quoted 7 lines

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +4 -0

@@ -208,6 +208,11 @@ void vidout_get(cv4l_fd &fd)
      }
 }
 
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}

Don't do this (same for vidcap_get_orig_from_set).

Quoted 3 lines

I think you just want to get the width and height from VIDIOC_G_FMT here, so why not just call that?

Those width/height are the values that are given by the user command. They are needed in order to
read raw frames line by line for the encoder.
Maybe I can implement it by calling 'parse_fmt' in 'stream_cmd' function similar to how 'vidout_cmd' do it.

https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-ctl/v4l2-ctl-vidout.cpp#n90

Quoted 26 lines

Remember that you can call v4l2-ctl without setting the output width and height if the defaults that the driver sets are already fine. In that case the width and height variables in this source are just 0.
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-vidout.cpp +1 -0
+
 void vidout_list(cv4l_fd &fd)
 {
      if (options[OptListOutFormats]) {
utils/v4l2-ctl/v4l2-ctl.h +0 -0
index 5a52a0a4..ab2994b2 100644
utils/v4l2-ctl/v4l2-ctl.h void vidout_cmd(int ch, char *optarg); · line 364
Quoted 8 lines
utils/v4l2-ctl/v4l2-ctl.h +2 -0
@@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg);
 void vidout_set(cv4l_fd &fd);
 void vidout_get(cv4l_fd &fd);
 void vidout_list(cv4l_fd &fd);
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
 
 // v4l2-ctl-overlay.cpp
 void overlay_usage(void);
This patch needs more work (not surprisingly, since it takes a bit of time to understand the v4l2-ctl source code).

Quoted 5 lines

Please stick to the kernel coding style! Using a different style makes it harder for me to review since my pattern matches routines in my brain no longer work optimally. It's like reading text with spelling mistakes, you cn stil undrstant iT, but it tekes moore teem. :-)

okei :)

Quoted 3 lines

Regards,
Hans

Hans Verkuil

2018-12-19 10:04 UTC #3 lore raw

Quoted 10 lines · Dafna Hirschfeld

Quoted 5 lines
Quoted 1 line
+bool is_m2m_enc = false;
This should be static.
I'm assuming that in a future patch we'll get a is_m2m_dec as well?
I forgot that there can be more options other than m2m_enc/_dec. So actually adding is_m2m_dec is needed. Or I can define an enum with 3 possible values
IS_M2M_ENC, IS_M2M_DEC, NOT_M2M_DEV

I think using bools will make the code easier.

Quoted 18 lines

Quoted 18 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 117
Quoted 11 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+
 #define TS_WINDOW 241
 #define FILE_HDR_ID                  v4l2_fourcc('V', 'h', 'd', 'r')
@@ -108,6 +114,84 @@ public:
      unsigned dropped();
 };
 
+static int get_codec_type(int fd, bool &is_enc) {
{ on the next line.
Quoted 3 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +3 -0
+     struct v4l2_capability vcap;
+
+     memset(&vcap,0,sizeof(vcap));

Quoted 6 lines

Quoted 4 lines
Space after ,
Please use the kernel coding style for these v4l utilities.
I ran the checkpatch script on this file and it didn't catch theses things. Do you use checkpatch for v4l-utils ?

No. As far as I can tell checkpatch skips cpp files so it can't be used for C++ files.

Quoted 115 lines

Review public: · line 122

Quoted 3 lines

(quoted diff) +2 -0

+
+     int ret = ioctl(fd, VIDIOC_QUERYCAP, &vcap);

Please use the cv4l_fd class. It comes with lots of helpers for all these ioctls and it already used in v4l2-ctl-streaming.cpp.

In this function you can just do:

        if (!fd.has_vid_m2m())
                return -1;

Quoted 13 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +13 -0

+     if(ret) {
+             fprintf(stderr, "get_codec_type: VIDIOC_QUERYCAP failed: %d\n", ret);
+             return ret;
+     }
+     unsigned int caps = vcap.capabilities;
+     if (caps & V4L2_CAP_DEVICE_CAPS)
+             caps = vcap.device_caps;
+     if(!(caps & V4L2_CAP_VIDEO_M2M) && !(caps & V4L2_CAP_VIDEO_M2M_MPLANE)) {
+             is_enc = false;
+             fprintf(stderr,"get_codec_type: not an M2M device\n");
+             return -1;
+     }
+

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 145

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+     struct v4l2_fmtdesc fmt;
+     memset(&fmt,0,sizeof(fmt));
+     fmt.index = 0;
+     fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }

These tests aren't good enough. You need to enumerate over all formats. Easiest is to keep a tally of the total number of formats and how many are compressed.

An encoder is a device where all output formats are uncompressed and all capture formats are compressed. It's the reverse for a decoder.

If you get a mix on either side, or both sides are raw or both sides are compressed, then it isn't a codec.

Quoted 61 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +49 -1

+     if (ret) {
+             is_enc = true;
+             return 0;
+     }
+     memset(&fmt,0,sizeof(fmt));
+     fmt.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }
+     if (ret) {
+             is_enc = false;
+             return 0;
+     }
+     fprintf(stderr, "get_codec_type: could no determine codec type\n");
+     return -1;
+}
+
+static void get_frame_dims(unsigned int &frame_width, unsigned int &frame_height) {
+
+     if(is_m2m_enc)
+             vidout_get_orig_from_set(frame_width, frame_height);
+     else
+             vidcap_get_orig_from_set(frame_width, frame_height);
+}
+
+static int get_visible_format(int fd, unsigned int &width, unsigned int &height) {
+     int ret = 0;
+     if(is_m2m_enc) {
+             struct v4l2_selection in_selection;
+
+             in_selection.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+             in_selection.target = V4L2_SEL_TGT_CROP;
+
+             if ( (ret = ioctl(fd, VIDIOC_G_SELECTION, &in_selection)) != 0) {
+                     fprintf(stderr,"get_visible_format: error in g_selection ioctl: %d\n",ret);
+                     return ret;
+             }
+             width = in_selection.r.width;
+             height = in_selection.r.height;
+     }
+     else { //TODO - g_selection with COMPOSE should be used here when implemented in driver
+             vidcap_get_orig_from_set(width, height);
+     }
+     return 0;
+}
+
+
 void fps_timestamps::determine_field(int fd, unsigned type)
 {
      struct v4l2_format fmt = { };

@@ -419,7 +503,6 @@ static void print_buffer(FILE *f, struct v4l2_buffer &buf)
                      fprintf(f, "\t\tData Offset: %u\n", p->data_offset);
              }
      }
-
      fprintf(f, "\n");
 }

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 743

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -657,7 +740,131 @@ void streaming_cmd(int ch, char *optarg)
      }
 }
 
-static bool fill_buffer_from_file(cv4l_queue &q, cv4l_buffer &b, FILE *fin)
+bool padding(cv4l_fd &fd, cv4l_queue &q, unsigned char* buf, FILE *fpointer, unsigned &sz, unsigned &len, bool is_read)

This should definitely be a static function. Also, this is not a very good name.

Quoted 5 lines

Quoted 1 line
Why not call it fill_padded_buffer_from_file()?
This function is used both for reading from file for the encoder and writing to file for the decoder.
Maybe it can be called read_write_padded_frame ?

That would work.

Quoted 19 lines

Quoted 9 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 745
Quoted 4 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+{
+     cv4l_fmt fmt(q.g_type());
No need to use q.g_type(). 'cv4l_fmt fmt;' is sufficient.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 746
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     fd.g_fmt(fmt, q.g_type());
Quoted 9 lines
After all, it's filled here.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 747
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     const struct v4l2_fwht_pixfmt_info *vic_fmt = v4l2_fwht_find_pixfmt(fmt.g_pixelformat());
This test should be moved to fill_buffer_from_file. If it is not an encoder and the pixelformat is not known (v4l2_fwht_find_pixfmt() returns NULL), then it should fallback to the old behavior. So this function should only be called when you have all the information about the pixelformat.

Quoted 3 lines

This function is supposed to be called only for m2m encoder on the output buffer and m2m decoder on the capture buffer
so vic_format is not NULL in those case.

Actually, handling padding is not specific to codecs. Any video device can have cropping or composing.

The generic rules are:

1) if a video output device supports TGT_CROP, then use that rectangle when reading from a file.

2) if a video capture device supports TGT_COMPOSE, then use that rectangle when writing to a file.

The problem with this is that doing this requires v4l2-ctl to understand all the pixelformats. That's a lot more work so for now just use v4l2_fwht_find_pixfmt() which has the information needed for the most common formats.

Anything not known by v4l2_fwht_find_pixfmt() can just fall back to the old behavior.

Quoted 35 lines

Quoted 22 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +21 -0

+     unsigned coded_width = fmt.g_width();
+     unsigned coded_height = fmt.g_height();
+     unsigned real_width;
+     unsigned real_height;
+     unsigned char *buf_p = (unsigned char*) buf;
+
+     if(is_read) {
+             real_width  = frame_width;
+             real_height = frame_height;
+     }
+     else {
+             real_width  = visible_width;
+             real_height = visible_height;
+     }
+     sz = 0;
+     len = real_width * real_height * vic_fmt->sizeimage_mult / vic_fmt->sizeimage_div;
+     switch(vic_fmt->id) {
+     case V4L2_PIX_FMT_YUYV:
+     case V4L2_PIX_FMT_YVYU:
+     case V4L2_PIX_FMT_UYVY:
+     case V4L2_PIX_FMT_VYUY:

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 778

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+     case V4L2_PIX_FMT_RGB24:
+     case V4L2_PIX_FMT_HSV24:
+     case V4L2_PIX_FMT_BGR24:
+     case V4L2_PIX_FMT_RGB32:
+     case V4L2_PIX_FMT_XRGB32:
+     case V4L2_PIX_FMT_HSV32:
+     case V4L2_PIX_FMT_BGR32:
+     case V4L2_PIX_FMT_XBGR32:
+     case V4L2_PIX_FMT_ARGB32:
+     case V4L2_PIX_FMT_ABGR32:

I'd put this all under a 'default' case. I think GREY can also be added here.

Quoted 8 lines

Quoted 7 lines
What I would really like to see is that only the information from v4l2_fwht_pixfmt_info can be used here without requiring a switch.
I think all that is needed to do that is that struct v4l2_fwht_pixfmt_info is extended with a 'planes_num' field, which is 1 for interleaved formats, 2 for luma/interleaved chroma planar formats and 3 for luma/cr/cb planar formats.
So I should send a patch to the kernel code, adding this field ?

Yes.

Quoted 152 lines

Quoted 149 lines

Quoted 120 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +99 -2

+             for(unsigned i=0; i < real_height; i++) {
+                     unsigned int consume_sz = vic_fmt->bytesperline_mult*real_width;
+                     unsigned int wsz = 0;
+                     if(is_read)
+                             wsz = fread(buf_p, 1, consume_sz, fpointer);
+                     else
+                             wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                     sz += wsz;
+                     if(wsz == 0 && i == 0)
+                             break;
+                     if(wsz != consume_sz) {
+                             fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                             return false;
+                     }
+                     buf_p += vic_fmt->chroma_step*coded_width;
+             }
+     break;
+
+     case V4L2_PIX_FMT_NV12:
+     case V4L2_PIX_FMT_NV16:
+     case V4L2_PIX_FMT_NV24:
+     case V4L2_PIX_FMT_NV21:
+     case V4L2_PIX_FMT_NV61:
+     case V4L2_PIX_FMT_NV42:
+             for(unsigned plane_idx = 0; plane_idx < 2; plane_idx++) {
+                     unsigned h_div = (plane_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (plane_idx == 0) ? 1 : vic_fmt->width_div;
+                     unsigned step  =  (plane_idx == 0) ? vic_fmt->luma_alpha_step : vic_fmt->chroma_step;
+
+                     for(unsigned i=0; i <  real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = step * real_width / w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1,  consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && plane_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += step*coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+     break;
+     case V4L2_PIX_FMT_YUV420:
+     case V4L2_PIX_FMT_YUV422P:
+     case V4L2_PIX_FMT_YVU420:
+     case V4L2_PIX_FMT_GREY:
+             for(unsigned comp_idx = 0; comp_idx < vic_fmt->components_num; comp_idx++) {
+                     unsigned h_div = (comp_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (comp_idx == 0) ? 1 : vic_fmt->width_div;
+
+                     for(unsigned i=0; i < real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = real_width/w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1, consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && comp_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+             break;
+     default:
+             fprintf(stderr,"the format is not supported yet\n");
+             return false;
+     }
+     return true;
+}
+
+static bool fill_buffer_from_file(cv4l_fd &fd, cv4l_queue &q, cv4l_buffer &b, FILE *fin)
 {
      static bool first = true;
      static bool is_fwht = false;

@@ -785,7 +992,15 @@ restart:
                              return false;
                      }
              }
-             sz = fread(buf, 1, len, fin);
+
+             if(is_m2m_enc) {
+                     if(!padding(fd, q, (unsigned char*) buf, fin, sz, len, true))
+                             return false;
+             }
+             else {
+                     sz = fread(buf, 1, len, fin);
+             }
+
              if (first && sz != len) {
                      fprintf(stderr, "Insufficient data\n");
                      return false;

@@ -908,7 +1123,7 @@ static int do_setup_out_buffers(cv4l_fd &fd, cv4l_queue &q, FILE *fin, bool qbuf
                                      tpg_fillbuffer(&tpg, stream_out_std, j, (u8 *)q.g_dataptr(i, j));
                      }
              }
-             if (fin && !fill_buffer_from_file(q, buf, fin))
+             if (fin && !fill_buffer_from_file(fd, q, buf, fin))
                      return -2;
 
              if (qbuf) {

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1178

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -960,7 +1175,7 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
              if (fd.qbuf(buf))
                      return -1;
      }
-
+

Seems to be a whitespace only change, just drop this change.

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -1

      double ts_secs = buf.g_timestamp().tv_sec + buf.g_timestamp().tv_usec / 1000000.0;
      fps_ts.add_ts(ts_secs, buf.g_sequence(), buf.g_field());

@@ -1023,8 +1238,15 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
                      }
                      if (host_fd_to >= 0)
                              sz = fwrite(comp_ptr[j] + offset, 1, used, fout);
-                     else

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1249

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +9 -1

-                             sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                     else {
+                             if(!is_m2m_enc) {
+                                     if(!padding(fd, q, (u8 *)q.g_dataptr(buf.g_index(), j) + offset, fout, sz, used, false))
+                                             return false;
+                             }
+                             else {
+                                     sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                             }
+                     }

This doesn't feel right.

Quoted 2 lines

I think a write_buffer_to_file() function should be introduced that deals with these variations.

Quoted 6 lines

Not sure what you meant, should I implement this if-else in another function ? The "padding" function is used both for reading and writing to/from padded buffer.
The condition for calling it here should be changed to "if(is_m2m_dec)" in which case "padding" will
write a raw frame to a file from the padded capture buffer.

static void write_buffer_to_file(cv4l_queue &q, cv4l_buffer &b, FILE *fout) {

#ifndef NO_STREAM_TO
	// code

#endif
}

And in do_handle_cap() drop the NO_STREAM_TO and replace it with a call to the new function:

        if (fout && (!stream_skip || ignore_count_skip) &&
            buf.g_bytesused(0) && !(buf.g_flags() & V4L2_BUF_FLAG_ERROR))
		write_buffer_to_file(q, buf, fout);

This can be done in a separate patch: first refactor the code, introducing the new function, then add support for handling padding.

Quoted 77 lines

Quoted 67 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +17 -2

                      if (sz != used)
                              fprintf(stderr, "%u != %u\n", sz, used);

@@ -1130,7 +1352,7 @@ static int do_handle_out(cv4l_fd &fd, cv4l_queue &q, FILE *fin, cv4l_buffer *cap
                      output_field = V4L2_FIELD_TOP;
      }
 
-     if (fin && !fill_buffer_from_file(q, buf, fin))
+     if (fin && !fill_buffer_from_file(fd, q, buf, fin))
              return -2;
 
      if (!fin && stream_out_refresh) {

@@ -1227,7 +1449,7 @@ static void streaming_set_cap(cv4l_fd &fd)
              }
              break;
      }
-
+
      memset(&sub, 0, sizeof(sub));
      sub.type = V4L2_EVENT_EOS;
      fd.subscribe_event(sub);

@@ -2031,6 +2253,21 @@ void streaming_set(cv4l_fd &fd, cv4l_fd &out_fd)
      int do_cap = options[OptStreamMmap] + options[OptStreamUser] + options[OptStreamDmaBuf];
      int do_out = options[OptStreamOutMmap] + options[OptStreamOutUser] + options[OptStreamOutDmaBuf];
 
+     int r = get_codec_type(fd.g_fd(), is_m2m_enc);
+     if(r) {
+             fprintf(stderr, "error checking codec type\n");
+             return;
+     }
+
+     r = get_visible_format(fd.g_fd(), visible_width, visible_height);
+
+     if(r) {
+             fprintf(stderr, "error getting the visible width\n");
+             return;
+     }
+
+     get_frame_dims(frame_width, frame_height);
+
      if (out_fd.g_fd() < 0) {
              out_capabilities = capabilities;
              out_priv_magic = priv_magic;

utils/v4l2-ctl/v4l2-ctl-vidcap.cpp +6 -0

index dc17a868..932f1fd2 100644
@@ -244,6 +244,12 @@ void vidcap_get(cv4l_fd &fd)
      }
 }
 
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}
+
+
 void vidcap_list(cv4l_fd &fd)
 {
      if (options[OptListFormats]) {

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +0 -0

index 5823df9c..05bd43ed 100644

utils/v4l2-ctl/v4l2-ctl-vidout.cpp void vidout_get(cv4l_fd &fd) · line 214

Quoted 7 lines

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +4 -0

@@ -208,6 +208,11 @@ void vidout_get(cv4l_fd &fd)
      }
 }
 
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}

Don't do this (same for vidcap_get_orig_from_set).

Quoted 4 lines

Quoted 3 lines
I think you just want to get the width and height from VIDIOC_G_FMT here, so why not just call that?
Those width/height are the values that are given by the user command.

Yes, but those values are used in ioctl calls to the driver, so rather than using those values you query the driver.

Quoted 2 lines

They are needed in order to
read raw frames line by line for the encoder.

Why not call G_FMT and G_SELECTION(TGT_CROP) to obtain that information?

Please note that all the set and get options are all processed before the streaming options. So when you start streaming the driver is fully configured.

Quoted 32 lines

Maybe I can implement it by calling 'parse_fmt' in 'stream_cmd' function similar to how 'vidout_cmd' do it.
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-ctl/v4l2-ctl-vidout.cpp#n90
Quoted 26 lines
Remember that you can call v4l2-ctl without setting the output width and height if the defaults that the driver sets are already fine. In that case the width and height variables in this source are just 0.
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-vidout.cpp +1 -0
+
 void vidout_list(cv4l_fd &fd)
 {
      if (options[OptListOutFormats]) {
utils/v4l2-ctl/v4l2-ctl.h +0 -0
index 5a52a0a4..ab2994b2 100644
utils/v4l2-ctl/v4l2-ctl.h void vidout_cmd(int ch, char *optarg); · line 364
Quoted 8 lines
utils/v4l2-ctl/v4l2-ctl.h +2 -0
@@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg);
 void vidout_set(cv4l_fd &fd);
 void vidout_get(cv4l_fd &fd);
 void vidout_list(cv4l_fd &fd);
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
 
 // v4l2-ctl-overlay.cpp
 void overlay_usage(void);
This patch needs more work (not surprisingly, since it takes a bit of time to understand the v4l2-ctl source code).

Quoted 10 lines

Quoted 5 lines
Please stick to the kernel coding style! Using a different style makes it harder for me to review since my pattern matches routines in my brain no longer work optimally. It's like reading text with spelling mistakes, you cn stil undrstant iT, but it tekes moore teem. :-)
okei :)
Quoted 3 lines
Regards,
Hans

Regards,

Hans

Dafna Hirschfeld

author

2018-12-19 13:32 UTC #4 lore raw

Quoted 196 lines · Hans Verkuil

Quoted 10 lines · Dafna Hirschfeld
Quoted 5 lines
Quoted 1 line
+bool is_m2m_enc = false;
This should be static.
I'm assuming that in a future patch we'll get a is_m2m_dec as well?
I forgot that there can be more options other than m2m_enc/_dec. So actually adding is_m2m_dec is needed. Or I can define an enum with 3 possible values
IS_M2M_ENC, IS_M2M_DEC, NOT_M2M_DEV
I think using bools will make the code easier.
Quoted 18 lines
Quoted 18 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 117
Quoted 11 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+
 #define TS_WINDOW 241
 #define FILE_HDR_ID                  v4l2_fourcc('V', 'h', 'd', 'r')
@@ -108,6 +114,84 @@ public:
      unsigned dropped();
 };
 
+static int get_codec_type(int fd, bool &is_enc) {
{ on the next line.
Quoted 3 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +3 -0
+     struct v4l2_capability vcap;
+
+     memset(&vcap,0,sizeof(vcap));
Quoted 6 lines
Quoted 4 lines
Space after ,
Please use the kernel coding style for these v4l utilities.
I ran the checkpatch script on this file and it didn't catch theses things. Do you use checkpatch for v4l-utils ?
No. As far as I can tell checkpatch skips cpp files so it can't be used for C++ files.
Quoted 115 lines
Quoted 115 lines
Review public: · line 122
Quoted 3 lines
(quoted diff) +2 -0
+
+     int ret = ioctl(fd, VIDIOC_QUERYCAP, &vcap);
Please use the cv4l_fd class. It comes with lots of helpers for all these ioctls and it already used in v4l2-ctl-streaming.cpp.
In this function you can just do:
        if (!fd.has_vid_m2m())
                return -1;
Quoted 13 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +13 -0
+     if(ret) {
+             fprintf(stderr, "get_codec_type: VIDIOC_QUERYCAP failed: %d\n", ret);
+             return ret;
+     }
+     unsigned int caps = vcap.capabilities;
+     if (caps & V4L2_CAP_DEVICE_CAPS)
+             caps = vcap.device_caps;
+     if(!(caps & V4L2_CAP_VIDEO_M2M) && !(caps & V4L2_CAP_VIDEO_M2M_MPLANE)) {
+             is_enc = false;
+             fprintf(stderr,"get_codec_type: not an M2M device\n");
+             return -1;
+     }
+
utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 145
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0
+     struct v4l2_fmtdesc fmt;
+     memset(&fmt,0,sizeof(fmt));
+     fmt.index = 0;
+     fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }
These tests aren't good enough. You need to enumerate over all formats. Easiest is to keep a tally of the total number of formats and how many are compressed.
An encoder is a device where all output formats are uncompressed and all capture formats are compressed. It's the reverse for a decoder.
If you get a mix on either side, or both sides are raw or both sides are compressed, then it isn't a codec.
Quoted 61 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +49 -1
+     if (ret) {
+             is_enc = true;
+             return 0;
+     }
+     memset(&fmt,0,sizeof(fmt));
+     fmt.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }
+     if (ret) {
+             is_enc = false;
+             return 0;
+     }
+     fprintf(stderr, "get_codec_type: could no determine codec type\n");
+     return -1;
+}
+
+static void get_frame_dims(unsigned int &frame_width, unsigned int &frame_height) {
+
+     if(is_m2m_enc)
+             vidout_get_orig_from_set(frame_width, frame_height);
+     else
+             vidcap_get_orig_from_set(frame_width, frame_height);
+}
+
+static int get_visible_format(int fd, unsigned int &width, unsigned int &height) {
+     int ret = 0;
+     if(is_m2m_enc) {
+             struct v4l2_selection in_selection;
+
+             in_selection.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+             in_selection.target = V4L2_SEL_TGT_CROP;
+
+             if ( (ret = ioctl(fd, VIDIOC_G_SELECTION, &in_selection)) != 0) {
+                     fprintf(stderr,"get_visible_format: error in g_selection ioctl: %d\n",ret);
+                     return ret;
+             }
+             width = in_selection.r.width;
+             height = in_selection.r.height;
+     }
+     else { //TODO - g_selection with COMPOSE should be used here when implemented in driver
+             vidcap_get_orig_from_set(width, height);
+     }
+     return 0;
+}
+
+
 void fps_timestamps::determine_field(int fd, unsigned type)
 {
      struct v4l2_format fmt = { };
@@ -419,7 +503,6 @@ static void print_buffer(FILE *f, struct v4l2_buffer &buf)
                      fprintf(f, "\t\tData Offset: %u\n", p->data_offset);
              }
      }
-
      fprintf(f, "\n");
 }
 
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 743
Quoted 5 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1
@@ -657,7 +740,131 @@ void streaming_cmd(int ch, char *optarg)
      }
 }
 
-static bool fill_buffer_from_file(cv4l_queue &q, cv4l_buffer &b, FILE *fin)
+bool padding(cv4l_fd &fd, cv4l_queue &q, unsigned char* buf, FILE *fpointer, unsigned &sz, unsigned &len, bool is_read)
This should definitely be a static function. Also, this is not a very good name.
Quoted 5 lines
Quoted 1 line
Why not call it fill_padded_buffer_from_file()?
This function is used both for reading from file for the encoder and writing to file for the decoder.
Maybe it can be called read_write_padded_frame ?
That would work.
Quoted 19 lines
Quoted 9 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 745
Quoted 4 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+{
+     cv4l_fmt fmt(q.g_type());
No need to use q.g_type(). 'cv4l_fmt fmt;' is sufficient.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 746
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     fd.g_fmt(fmt, q.g_type());
Quoted 9 lines
After all, it's filled here.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 747
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     const struct v4l2_fwht_pixfmt_info *vic_fmt = v4l2_fwht_find_pixfmt(fmt.g_pixelformat());
This test should be moved to fill_buffer_from_file. If it is not an encoder and the pixelformat is not known (v4l2_fwht_find_pixfmt() returns NULL), then it should fallback to the old behavior. So this function should only be called when you have all the information about the pixelformat.
Quoted 3 lines
This function is supposed to be called only for m2m encoder on the output buffer and m2m decoder on the capture buffer
so vic_format is not NULL in those case.
Actually, handling padding is not specific to codecs. Any video device can have cropping or composing.
The generic rules are:
1) if a video output device supports TGT_CROP, then use that rectangle when reading from a file.

Quoted 9 lines

2) if a video capture device supports TGT_COMPOSE, then use that rectangle when writing to a file.
The problem with this is that doing this requires v4l2-ctl to understand all the pixelformats. That's a lot more work so for now just use v4l2_fwht_find_pixfmt() which has the information needed for the most common formats.
Anything not known by v4l2_fwht_find_pixfmt() can just fall back to the old behavior.

So I'm not sure what is the correct implementation here, Currently this function suppose to be used only by m2m_enc/dec, I actually have no idea
how other types of drivers work. What exactly should be the conditions for calling this function ?

Dafna

Quoted 361 lines

Quoted 35 lines

Quoted 22 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +21 -0

+     unsigned coded_width = fmt.g_width();
+     unsigned coded_height = fmt.g_height();
+     unsigned real_width;
+     unsigned real_height;
+     unsigned char *buf_p = (unsigned char*) buf;
+
+     if(is_read) {
+             real_width  = frame_width;
+             real_height = frame_height;
+     }
+     else {
+             real_width  = visible_width;
+             real_height = visible_height;
+     }
+     sz = 0;
+     len = real_width * real_height * vic_fmt->sizeimage_mult / vic_fmt->sizeimage_div;
+     switch(vic_fmt->id) {
+     case V4L2_PIX_FMT_YUYV:
+     case V4L2_PIX_FMT_YVYU:
+     case V4L2_PIX_FMT_UYVY:
+     case V4L2_PIX_FMT_VYUY:

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 778

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+     case V4L2_PIX_FMT_RGB24:
+     case V4L2_PIX_FMT_HSV24:
+     case V4L2_PIX_FMT_BGR24:
+     case V4L2_PIX_FMT_RGB32:
+     case V4L2_PIX_FMT_XRGB32:
+     case V4L2_PIX_FMT_HSV32:
+     case V4L2_PIX_FMT_BGR32:
+     case V4L2_PIX_FMT_XBGR32:
+     case V4L2_PIX_FMT_ARGB32:
+     case V4L2_PIX_FMT_ABGR32:

I'd put this all under a 'default' case. I think GREY can also be added here.

Quoted 8 lines

Quoted 7 lines
What I would really like to see is that only the information from v4l2_fwht_pixfmt_info can be used here without requiring a switch.
I think all that is needed to do that is that struct v4l2_fwht_pixfmt_info is extended with a 'planes_num' field, which is 1 for interleaved formats, 2 for luma/interleaved chroma planar formats and 3 for luma/cr/cb planar formats.
So I should send a patch to the kernel code, adding this field ?

Yes.

Quoted 152 lines

Quoted 149 lines

Quoted 120 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +99 -2

+             for(unsigned i=0; i < real_height; i++) {
+                     unsigned int consume_sz = vic_fmt->bytesperline_mult*real_width;
+                     unsigned int wsz = 0;
+                     if(is_read)
+                             wsz = fread(buf_p, 1, consume_sz, fpointer);
+                     else
+                             wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                     sz += wsz;
+                     if(wsz == 0 && i == 0)
+                             break;
+                     if(wsz != consume_sz) {
+                             fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                             return false;
+                     }
+                     buf_p += vic_fmt->chroma_step*coded_width;
+             }
+     break;
+
+     case V4L2_PIX_FMT_NV12:
+     case V4L2_PIX_FMT_NV16:
+     case V4L2_PIX_FMT_NV24:
+     case V4L2_PIX_FMT_NV21:
+     case V4L2_PIX_FMT_NV61:
+     case V4L2_PIX_FMT_NV42:
+             for(unsigned plane_idx = 0; plane_idx < 2; plane_idx++) {
+                     unsigned h_div = (plane_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (plane_idx == 0) ? 1 : vic_fmt->width_div;
+                     unsigned step  =  (plane_idx == 0) ? vic_fmt->luma_alpha_step : vic_fmt->chroma_step;
+
+                     for(unsigned i=0; i <  real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = step * real_width / w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1,  consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && plane_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += step*coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+     break;
+     case V4L2_PIX_FMT_YUV420:
+     case V4L2_PIX_FMT_YUV422P:
+     case V4L2_PIX_FMT_YVU420:
+     case V4L2_PIX_FMT_GREY:
+             for(unsigned comp_idx = 0; comp_idx < vic_fmt->components_num; comp_idx++) {
+                     unsigned h_div = (comp_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (comp_idx == 0) ? 1 : vic_fmt->width_div;
+
+                     for(unsigned i=0; i < real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = real_width/w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1, consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && comp_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+             break;
+     default:
+             fprintf(stderr,"the format is not supported yet\n");
+             return false;
+     }
+     return true;
+}
+
+static bool fill_buffer_from_file(cv4l_fd &fd, cv4l_queue &q, cv4l_buffer &b, FILE *fin)
 {
      static bool first = true;
      static bool is_fwht = false;

@@ -785,7 +992,15 @@ restart:
                              return false;
                      }
              }
-             sz = fread(buf, 1, len, fin);
+
+             if(is_m2m_enc) {
+                     if(!padding(fd, q, (unsigned char*) buf, fin, sz, len, true))
+                             return false;
+             }
+             else {
+                     sz = fread(buf, 1, len, fin);
+             }
+
              if (first && sz != len) {
                      fprintf(stderr, "Insufficient data\n");
                      return false;

@@ -908,7 +1123,7 @@ static int do_setup_out_buffers(cv4l_fd &fd, cv4l_queue &q, FILE *fin, bool qbuf
                                      tpg_fillbuffer(&tpg, stream_out_std, j, (u8 *)q.g_dataptr(i, j));
                      }
              }
-             if (fin && !fill_buffer_from_file(q, buf, fin))
+             if (fin && !fill_buffer_from_file(fd, q, buf, fin))
                      return -2;
 
              if (qbuf) {

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1178

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -960,7 +1175,7 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
              if (fd.qbuf(buf))
                      return -1;
      }
-
+

Seems to be a whitespace only change, just drop this change.

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -1

      double ts_secs = buf.g_timestamp().tv_sec + buf.g_timestamp().tv_usec / 1000000.0;
      fps_ts.add_ts(ts_secs, buf.g_sequence(), buf.g_field());

@@ -1023,8 +1238,15 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
                      }
                      if (host_fd_to >= 0)
                              sz = fwrite(comp_ptr[j] + offset, 1, used, fout);
-                     else

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1249

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +9 -1

-                             sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                     else {
+                             if(!is_m2m_enc) {
+                                     if(!padding(fd, q, (u8 *)q.g_dataptr(buf.g_index(), j) + offset, fout, sz, used, false))
+                                             return false;
+                             }
+                             else {
+                                     sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                             }
+                     }

This doesn't feel right.

Quoted 2 lines

I think a write_buffer_to_file() function should be introduced that deals with these variations.

Quoted 6 lines

Not sure what you meant, should I implement this if-else in another function ? The "padding" function is used both for reading and writing to/from padded buffer.
The condition for calling it here should be changed to "if(is_m2m_dec)" in which case "padding" will
write a raw frame to a file from the padded capture buffer.

static void write_buffer_to_file(cv4l_queue &q, cv4l_buffer &b, FILE *fout) {

#ifndef NO_STREAM_TO
        // code

#endif
}

And in do_handle_cap() drop the NO_STREAM_TO and replace it with a call to the new function:

        if (fout && (!stream_skip || ignore_count_skip) &&
            buf.g_bytesused(0) && !(buf.g_flags() & V4L2_BUF_FLAG_ERROR))
                write_buffer_to_file(q, buf, fout);

This can be done in a separate patch: first refactor the code, introducing the new function, then add support for handling padding.

Quoted 77 lines

Quoted 67 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +17 -2

                      if (sz != used)
                              fprintf(stderr, "%u != %u\n", sz, used);

@@ -1130,7 +1352,7 @@ static int do_handle_out(cv4l_fd &fd, cv4l_queue &q, FILE *fin, cv4l_buffer *cap
                      output_field = V4L2_FIELD_TOP;
      }
 
-     if (fin && !fill_buffer_from_file(q, buf, fin))
+     if (fin && !fill_buffer_from_file(fd, q, buf, fin))
              return -2;
 
      if (!fin && stream_out_refresh) {

@@ -1227,7 +1449,7 @@ static void streaming_set_cap(cv4l_fd &fd)
              }
              break;
      }
-
+
      memset(&sub, 0, sizeof(sub));
      sub.type = V4L2_EVENT_EOS;
      fd.subscribe_event(sub);

@@ -2031,6 +2253,21 @@ void streaming_set(cv4l_fd &fd, cv4l_fd &out_fd)
      int do_cap = options[OptStreamMmap] + options[OptStreamUser] + options[OptStreamDmaBuf];
      int do_out = options[OptStreamOutMmap] + options[OptStreamOutUser] + options[OptStreamOutDmaBuf];
 
+     int r = get_codec_type(fd.g_fd(), is_m2m_enc);
+     if(r) {
+             fprintf(stderr, "error checking codec type\n");
+             return;
+     }
+
+     r = get_visible_format(fd.g_fd(), visible_width, visible_height);
+
+     if(r) {
+             fprintf(stderr, "error getting the visible width\n");
+             return;
+     }
+
+     get_frame_dims(frame_width, frame_height);
+
      if (out_fd.g_fd() < 0) {
              out_capabilities = capabilities;
              out_priv_magic = priv_magic;

utils/v4l2-ctl/v4l2-ctl-vidcap.cpp +6 -0

index dc17a868..932f1fd2 100644
@@ -244,6 +244,12 @@ void vidcap_get(cv4l_fd &fd)
      }
 }
 
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}
+
+
 void vidcap_list(cv4l_fd &fd)
 {
      if (options[OptListFormats]) {

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +0 -0

index 5823df9c..05bd43ed 100644

utils/v4l2-ctl/v4l2-ctl-vidout.cpp void vidout_get(cv4l_fd &fd) · line 214

Quoted 7 lines

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +4 -0

@@ -208,6 +208,11 @@ void vidout_get(cv4l_fd &fd)
      }
 }
 
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}

Don't do this (same for vidcap_get_orig_from_set).

Quoted 4 lines

Quoted 3 lines
I think you just want to get the width and height from VIDIOC_G_FMT here, so why not just call that?
Those width/height are the values that are given by the user command.

Yes, but those values are used in ioctl calls to the driver, so rather than using those values you query the driver.

Quoted 2 lines

They are needed in order to
read raw frames line by line for the encoder.

Why not call G_FMT and G_SELECTION(TGT_CROP) to obtain that information?

Please note that all the set and get options are all processed before the streaming options. So when you start streaming the driver is fully configured.

Quoted 32 lines

Maybe I can implement it by calling 'parse_fmt' in 'stream_cmd' function similar to how 'vidout_cmd' do it.
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-ctl/v4l2-ctl-vidout.cpp#n90
Quoted 26 lines
Remember that you can call v4l2-ctl without setting the output width and height if the defaults that the driver sets are already fine. In that case the width and height variables in this source are just 0.
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-vidout.cpp +1 -0
+
 void vidout_list(cv4l_fd &fd)
 {
      if (options[OptListOutFormats]) {
utils/v4l2-ctl/v4l2-ctl.h +0 -0
index 5a52a0a4..ab2994b2 100644
utils/v4l2-ctl/v4l2-ctl.h void vidout_cmd(int ch, char *optarg); · line 364
Quoted 8 lines
utils/v4l2-ctl/v4l2-ctl.h +2 -0
@@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg);
 void vidout_set(cv4l_fd &fd);
 void vidout_get(cv4l_fd &fd);
 void vidout_list(cv4l_fd &fd);
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
 
 // v4l2-ctl-overlay.cpp
 void overlay_usage(void);
This patch needs more work (not surprisingly, since it takes a bit of time to understand the v4l2-ctl source code).

Quoted 10 lines

Quoted 5 lines
Please stick to the kernel coding style! Using a different style makes it harder for me to review since my pattern matches routines in my brain no longer work optimally. It's like reading text with spelling mistakes, you cn stil undrstant iT, but it tekes moore teem. :-)
okei :)
Quoted 3 lines
Regards,
Hans

Regards,

Hans

Hans Verkuil

2018-12-20 09:45 UTC #5 lore raw

Quoted 204 lines · Dafna Hirschfeld

Quoted 203 lines · Hans Verkuil
Quoted 10 lines · Dafna Hirschfeld
Quoted 5 lines
Quoted 1 line
+bool is_m2m_enc = false;
This should be static.
I'm assuming that in a future patch we'll get a is_m2m_dec as well?
I forgot that there can be more options other than m2m_enc/_dec. So actually adding is_m2m_dec is needed. Or I can define an enum with 3 possible values
IS_M2M_ENC, IS_M2M_DEC, NOT_M2M_DEV
I think using bools will make the code easier.
Quoted 18 lines
Quoted 18 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 117
Quoted 11 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+
 #define TS_WINDOW 241
 #define FILE_HDR_ID                  v4l2_fourcc('V', 'h', 'd', 'r')
@@ -108,6 +114,84 @@ public:
      unsigned dropped();
 };
 
+static int get_codec_type(int fd, bool &is_enc) {
{ on the next line.
Quoted 3 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +3 -0
+     struct v4l2_capability vcap;
+
+     memset(&vcap,0,sizeof(vcap));
Quoted 6 lines
Quoted 4 lines
Space after ,
Please use the kernel coding style for these v4l utilities.
I ran the checkpatch script on this file and it didn't catch theses things. Do you use checkpatch for v4l-utils ?
No. As far as I can tell checkpatch skips cpp files so it can't be used for C++ files.
Quoted 115 lines
Quoted 115 lines
Review public: · line 122
Quoted 3 lines
(quoted diff) +2 -0
+
+     int ret = ioctl(fd, VIDIOC_QUERYCAP, &vcap);
Please use the cv4l_fd class. It comes with lots of helpers for all these ioctls and it already used in v4l2-ctl-streaming.cpp.
In this function you can just do:
        if (!fd.has_vid_m2m())
                return -1;
Quoted 13 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +13 -0
+     if(ret) {
+             fprintf(stderr, "get_codec_type: VIDIOC_QUERYCAP failed: %d\n", ret);
+             return ret;
+     }
+     unsigned int caps = vcap.capabilities;
+     if (caps & V4L2_CAP_DEVICE_CAPS)
+             caps = vcap.device_caps;
+     if(!(caps & V4L2_CAP_VIDEO_M2M) && !(caps & V4L2_CAP_VIDEO_M2M_MPLANE)) {
+             is_enc = false;
+             fprintf(stderr,"get_codec_type: not an M2M device\n");
+             return -1;
+     }
+
utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 145
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0
+     struct v4l2_fmtdesc fmt;
+     memset(&fmt,0,sizeof(fmt));
+     fmt.index = 0;
+     fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }
These tests aren't good enough. You need to enumerate over all formats. Easiest is to keep a tally of the total number of formats and how many are compressed.
An encoder is a device where all output formats are uncompressed and all capture formats are compressed. It's the reverse for a decoder.
If you get a mix on either side, or both sides are raw or both sides are compressed, then it isn't a codec.
Quoted 61 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +49 -1
+     if (ret) {
+             is_enc = true;
+             return 0;
+     }
+     memset(&fmt,0,sizeof(fmt));
+     fmt.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }
+     if (ret) {
+             is_enc = false;
+             return 0;
+     }
+     fprintf(stderr, "get_codec_type: could no determine codec type\n");
+     return -1;
+}
+
+static void get_frame_dims(unsigned int &frame_width, unsigned int &frame_height) {
+
+     if(is_m2m_enc)
+             vidout_get_orig_from_set(frame_width, frame_height);
+     else
+             vidcap_get_orig_from_set(frame_width, frame_height);
+}
+
+static int get_visible_format(int fd, unsigned int &width, unsigned int &height) {
+     int ret = 0;
+     if(is_m2m_enc) {
+             struct v4l2_selection in_selection;
+
+             in_selection.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+             in_selection.target = V4L2_SEL_TGT_CROP;
+
+             if ( (ret = ioctl(fd, VIDIOC_G_SELECTION, &in_selection)) != 0) {
+                     fprintf(stderr,"get_visible_format: error in g_selection ioctl: %d\n",ret);
+                     return ret;
+             }
+             width = in_selection.r.width;
+             height = in_selection.r.height;
+     }
+     else { //TODO - g_selection with COMPOSE should be used here when implemented in driver
+             vidcap_get_orig_from_set(width, height);
+     }
+     return 0;
+}
+
+
 void fps_timestamps::determine_field(int fd, unsigned type)
 {
      struct v4l2_format fmt = { };
@@ -419,7 +503,6 @@ static void print_buffer(FILE *f, struct v4l2_buffer &buf)
                      fprintf(f, "\t\tData Offset: %u\n", p->data_offset);
              }
      }
-
      fprintf(f, "\n");
 }
 
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 743
Quoted 5 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1
@@ -657,7 +740,131 @@ void streaming_cmd(int ch, char *optarg)
      }
 }
 
-static bool fill_buffer_from_file(cv4l_queue &q, cv4l_buffer &b, FILE *fin)
+bool padding(cv4l_fd &fd, cv4l_queue &q, unsigned char* buf, FILE *fpointer, unsigned &sz, unsigned &len, bool is_read)
This should definitely be a static function. Also, this is not a very good name.
Quoted 5 lines
Quoted 1 line
Why not call it fill_padded_buffer_from_file()?
This function is used both for reading from file for the encoder and writing to file for the decoder.
Maybe it can be called read_write_padded_frame ?
That would work.
Quoted 19 lines
Quoted 9 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 745
Quoted 4 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+{
+     cv4l_fmt fmt(q.g_type());
No need to use q.g_type(). 'cv4l_fmt fmt;' is sufficient.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 746
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     fd.g_fmt(fmt, q.g_type());
Quoted 9 lines
After all, it's filled here.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 747
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     const struct v4l2_fwht_pixfmt_info *vic_fmt = v4l2_fwht_find_pixfmt(fmt.g_pixelformat());
This test should be moved to fill_buffer_from_file. If it is not an encoder and the pixelformat is not known (v4l2_fwht_find_pixfmt() returns NULL), then it should fallback to the old behavior. So this function should only be called when you have all the information about the pixelformat.
Quoted 3 lines
This function is supposed to be called only for m2m encoder on the output buffer and m2m decoder on the capture buffer
so vic_format is not NULL in those case.
Actually, handling padding is not specific to codecs. Any video device can have cropping or composing.
The generic rules are:
1) if a video output device supports TGT_CROP, then use that rectangle when reading from a file.
2) if a video capture device supports TGT_COMPOSE, then use that rectangle when writing to a file.
The problem with this is that doing this requires v4l2-ctl to understand all the pixelformats. That's a lot more work so for now just use v4l2_fwht_find_pixfmt() which has the information needed for the most common formats.

Quoted 7 lines

Quoted 2 lines
Anything not known by v4l2_fwht_find_pixfmt() can just fall back to the old behavior.
So I'm not sure what is the correct implementation here, Currently this function suppose to be used only by m2m_enc/dec, I actually have no idea
how other types of drivers work. What exactly should be the conditions for calling this function ?

Any driver that supports TGT_CROP for video output or TGT_COMPOSE for video capture should use that information when reading from a file or writing to a file in v4l2-ctl.

This is independent of the codec implementation or the FWHT codec in particular.

But implementing this requires knowledge of the memory layout each pixelformat represents. But there are many, and some are quite complicated. Rather than trying to support all defined raw pixelformats my suggestion was to use the v4l2_fwht_find_pixfmt() function since that returns the needed information for most of the common pixelformats in use.

So this really has nothing to do with FWHT, it has to do with easily obtaining memory layout information for most of the common pixelformats.

And if v4l2_fwht_find_pixfmt() returns NULL, then just skip the selection handling in v4l2-ctl.

Hope this helps,

Hans

Quoted 364 lines

Dafna

Quoted 361 lines

Quoted 35 lines

Quoted 22 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +21 -0

+     unsigned coded_width = fmt.g_width();
+     unsigned coded_height = fmt.g_height();
+     unsigned real_width;
+     unsigned real_height;
+     unsigned char *buf_p = (unsigned char*) buf;
+
+     if(is_read) {
+             real_width  = frame_width;
+             real_height = frame_height;
+     }
+     else {
+             real_width  = visible_width;
+             real_height = visible_height;
+     }
+     sz = 0;
+     len = real_width * real_height * vic_fmt->sizeimage_mult / vic_fmt->sizeimage_div;
+     switch(vic_fmt->id) {
+     case V4L2_PIX_FMT_YUYV:
+     case V4L2_PIX_FMT_YVYU:
+     case V4L2_PIX_FMT_UYVY:
+     case V4L2_PIX_FMT_VYUY:

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 778

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+     case V4L2_PIX_FMT_RGB24:
+     case V4L2_PIX_FMT_HSV24:
+     case V4L2_PIX_FMT_BGR24:
+     case V4L2_PIX_FMT_RGB32:
+     case V4L2_PIX_FMT_XRGB32:
+     case V4L2_PIX_FMT_HSV32:
+     case V4L2_PIX_FMT_BGR32:
+     case V4L2_PIX_FMT_XBGR32:
+     case V4L2_PIX_FMT_ARGB32:
+     case V4L2_PIX_FMT_ABGR32:

I'd put this all under a 'default' case. I think GREY can also be added here.

Quoted 8 lines

Quoted 7 lines
What I would really like to see is that only the information from v4l2_fwht_pixfmt_info can be used here without requiring a switch.
I think all that is needed to do that is that struct v4l2_fwht_pixfmt_info is extended with a 'planes_num' field, which is 1 for interleaved formats, 2 for luma/interleaved chroma planar formats and 3 for luma/cr/cb planar formats.
So I should send a patch to the kernel code, adding this field ?

Yes.

Quoted 152 lines

Quoted 149 lines

Quoted 120 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +99 -2

+             for(unsigned i=0; i < real_height; i++) {
+                     unsigned int consume_sz = vic_fmt->bytesperline_mult*real_width;
+                     unsigned int wsz = 0;
+                     if(is_read)
+                             wsz = fread(buf_p, 1, consume_sz, fpointer);
+                     else
+                             wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                     sz += wsz;
+                     if(wsz == 0 && i == 0)
+                             break;
+                     if(wsz != consume_sz) {
+                             fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                             return false;
+                     }
+                     buf_p += vic_fmt->chroma_step*coded_width;
+             }
+     break;
+
+     case V4L2_PIX_FMT_NV12:
+     case V4L2_PIX_FMT_NV16:
+     case V4L2_PIX_FMT_NV24:
+     case V4L2_PIX_FMT_NV21:
+     case V4L2_PIX_FMT_NV61:
+     case V4L2_PIX_FMT_NV42:
+             for(unsigned plane_idx = 0; plane_idx < 2; plane_idx++) {
+                     unsigned h_div = (plane_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (plane_idx == 0) ? 1 : vic_fmt->width_div;
+                     unsigned step  =  (plane_idx == 0) ? vic_fmt->luma_alpha_step : vic_fmt->chroma_step;
+
+                     for(unsigned i=0; i <  real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = step * real_width / w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1,  consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && plane_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += step*coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+     break;
+     case V4L2_PIX_FMT_YUV420:
+     case V4L2_PIX_FMT_YUV422P:
+     case V4L2_PIX_FMT_YVU420:
+     case V4L2_PIX_FMT_GREY:
+             for(unsigned comp_idx = 0; comp_idx < vic_fmt->components_num; comp_idx++) {
+                     unsigned h_div = (comp_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (comp_idx == 0) ? 1 : vic_fmt->width_div;
+
+                     for(unsigned i=0; i < real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = real_width/w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1, consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && comp_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+             break;
+     default:
+             fprintf(stderr,"the format is not supported yet\n");
+             return false;
+     }
+     return true;
+}
+
+static bool fill_buffer_from_file(cv4l_fd &fd, cv4l_queue &q, cv4l_buffer &b, FILE *fin)
 {
      static bool first = true;
      static bool is_fwht = false;

@@ -785,7 +992,15 @@ restart:
                              return false;
                      }
              }
-             sz = fread(buf, 1, len, fin);
+
+             if(is_m2m_enc) {
+                     if(!padding(fd, q, (unsigned char*) buf, fin, sz, len, true))
+                             return false;
+             }
+             else {
+                     sz = fread(buf, 1, len, fin);
+             }
+
              if (first && sz != len) {
                      fprintf(stderr, "Insufficient data\n");
                      return false;

@@ -908,7 +1123,7 @@ static int do_setup_out_buffers(cv4l_fd &fd, cv4l_queue &q, FILE *fin, bool qbuf
                                      tpg_fillbuffer(&tpg, stream_out_std, j, (u8 *)q.g_dataptr(i, j));
                      }
              }
-             if (fin && !fill_buffer_from_file(q, buf, fin))
+             if (fin && !fill_buffer_from_file(fd, q, buf, fin))
                      return -2;
 
              if (qbuf) {

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1178

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -960,7 +1175,7 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
              if (fd.qbuf(buf))
                      return -1;
      }
-
+

Seems to be a whitespace only change, just drop this change.

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -1

      double ts_secs = buf.g_timestamp().tv_sec + buf.g_timestamp().tv_usec / 1000000.0;
      fps_ts.add_ts(ts_secs, buf.g_sequence(), buf.g_field());

@@ -1023,8 +1238,15 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
                      }
                      if (host_fd_to >= 0)
                              sz = fwrite(comp_ptr[j] + offset, 1, used, fout);
-                     else

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1249

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +9 -1

-                             sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                     else {
+                             if(!is_m2m_enc) {
+                                     if(!padding(fd, q, (u8 *)q.g_dataptr(buf.g_index(), j) + offset, fout, sz, used, false))
+                                             return false;
+                             }
+                             else {
+                                     sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                             }
+                     }

This doesn't feel right.

Quoted 2 lines

I think a write_buffer_to_file() function should be introduced that deals with these variations.

Quoted 6 lines

Not sure what you meant, should I implement this if-else in another function ? The "padding" function is used both for reading and writing to/from padded buffer.
The condition for calling it here should be changed to "if(is_m2m_dec)" in which case "padding" will
write a raw frame to a file from the padded capture buffer.

static void write_buffer_to_file(cv4l_queue &q, cv4l_buffer &b, FILE *fout) {

#ifndef NO_STREAM_TO
        // code

#endif
}

And in do_handle_cap() drop the NO_STREAM_TO and replace it with a call to the new function:

        if (fout && (!stream_skip || ignore_count_skip) &&
            buf.g_bytesused(0) && !(buf.g_flags() & V4L2_BUF_FLAG_ERROR))
                write_buffer_to_file(q, buf, fout);

This can be done in a separate patch: first refactor the code, introducing the new function, then add support for handling padding.

Quoted 77 lines

Quoted 67 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +17 -2

                      if (sz != used)
                              fprintf(stderr, "%u != %u\n", sz, used);

@@ -1130,7 +1352,7 @@ static int do_handle_out(cv4l_fd &fd, cv4l_queue &q, FILE *fin, cv4l_buffer *cap
                      output_field = V4L2_FIELD_TOP;
      }
 
-     if (fin && !fill_buffer_from_file(q, buf, fin))
+     if (fin && !fill_buffer_from_file(fd, q, buf, fin))
              return -2;
 
      if (!fin && stream_out_refresh) {

@@ -1227,7 +1449,7 @@ static void streaming_set_cap(cv4l_fd &fd)
              }
              break;
      }
-
+
      memset(&sub, 0, sizeof(sub));
      sub.type = V4L2_EVENT_EOS;
      fd.subscribe_event(sub);

@@ -2031,6 +2253,21 @@ void streaming_set(cv4l_fd &fd, cv4l_fd &out_fd)
      int do_cap = options[OptStreamMmap] + options[OptStreamUser] + options[OptStreamDmaBuf];
      int do_out = options[OptStreamOutMmap] + options[OptStreamOutUser] + options[OptStreamOutDmaBuf];
 
+     int r = get_codec_type(fd.g_fd(), is_m2m_enc);
+     if(r) {
+             fprintf(stderr, "error checking codec type\n");
+             return;
+     }
+
+     r = get_visible_format(fd.g_fd(), visible_width, visible_height);
+
+     if(r) {
+             fprintf(stderr, "error getting the visible width\n");
+             return;
+     }
+
+     get_frame_dims(frame_width, frame_height);
+
      if (out_fd.g_fd() < 0) {
              out_capabilities = capabilities;
              out_priv_magic = priv_magic;

utils/v4l2-ctl/v4l2-ctl-vidcap.cpp +6 -0

index dc17a868..932f1fd2 100644
@@ -244,6 +244,12 @@ void vidcap_get(cv4l_fd &fd)
      }
 }
 
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}
+
+
 void vidcap_list(cv4l_fd &fd)
 {
      if (options[OptListFormats]) {

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +0 -0

index 5823df9c..05bd43ed 100644

utils/v4l2-ctl/v4l2-ctl-vidout.cpp void vidout_get(cv4l_fd &fd) · line 214

Quoted 7 lines

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +4 -0

@@ -208,6 +208,11 @@ void vidout_get(cv4l_fd &fd)
      }
 }
 
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}

Don't do this (same for vidcap_get_orig_from_set).

Quoted 4 lines

Quoted 3 lines
I think you just want to get the width and height from VIDIOC_G_FMT here, so why not just call that?
Those width/height are the values that are given by the user command.

Yes, but those values are used in ioctl calls to the driver, so rather than using those values you query the driver.

Quoted 2 lines

They are needed in order to
read raw frames line by line for the encoder.

Why not call G_FMT and G_SELECTION(TGT_CROP) to obtain that information?

Please note that all the set and get options are all processed before the streaming options. So when you start streaming the driver is fully configured.

Quoted 32 lines

Maybe I can implement it by calling 'parse_fmt' in 'stream_cmd' function similar to how 'vidout_cmd' do it.
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-ctl/v4l2-ctl-vidout.cpp#n90
Quoted 26 lines
Remember that you can call v4l2-ctl without setting the output width and height if the defaults that the driver sets are already fine. In that case the width and height variables in this source are just 0.
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-vidout.cpp +1 -0
+
 void vidout_list(cv4l_fd &fd)
 {
      if (options[OptListOutFormats]) {
utils/v4l2-ctl/v4l2-ctl.h +0 -0
index 5a52a0a4..ab2994b2 100644
utils/v4l2-ctl/v4l2-ctl.h void vidout_cmd(int ch, char *optarg); · line 364
Quoted 8 lines
utils/v4l2-ctl/v4l2-ctl.h +2 -0
@@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg);
 void vidout_set(cv4l_fd &fd);
 void vidout_get(cv4l_fd &fd);
 void vidout_list(cv4l_fd &fd);
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
 
 // v4l2-ctl-overlay.cpp
 void overlay_usage(void);
This patch needs more work (not surprisingly, since it takes a bit of time to understand the v4l2-ctl source code).

Quoted 10 lines

Quoted 5 lines
Please stick to the kernel coding style! Using a different style makes it harder for me to review since my pattern matches routines in my brain no longer work optimally. It's like reading text with spelling mistakes, you cn stil undrstant iT, but it tekes moore teem. :-)
okei :)
Quoted 3 lines
Regards,
Hans

Regards,

Hans

A message that is not in the archive (sent off-list or not yet received): CAJ1myNSeybTSGQo9wc9FnJsYqVOdTxwtaxK4tFZrc41A4NiTjw@mail.gmail.com

Hans Verkuil

2018-12-21 12:31 UTC #7 lore raw

Quoted 220 lines · Dafna Hirschfeld

On Thu, Dec 20, 2018 at 11:45 AM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:

    On 12/19/18 2:32 PM, Dafna Hirschfeld wrote:
    > On Wed, Dec 19, 2018 at 12:03 PM Hans Verkuil <[email protected] <mailto:[email protected]>> wrote:
    >>
    >> On 12/19/18 9:34 AM, Dafna Hirschfeld wrote:
    >>>>> +bool is_m2m_enc = false;
    >>>>
    >>>> This should be static.
    >>>>
    >>>> I'm assuming that in a future patch we'll get a is_m2m_dec as well?
    >>>
    >>> I forgot that there can be more options other than m2m_enc/_dec.
    >>> So actually adding is_m2m_dec is needed. Or I can define an enum with
    >>> 3 possible values
    >>> IS_M2M_ENC, IS_M2M_DEC, NOT_M2M_DEV
    >>
    >> I think using bools will make the code easier.
    >>
    >>>
    >>>>
    >>>>> +
    >>>>>  #define TS_WINDOW 241
    >>>>>  #define FILE_HDR_ID                  v4l2_fourcc('V', 'h', 'd', 'r')
    >>>>>
    >>>>> @@ -108,6 +114,84 @@ public:
    >>>>>       unsigned dropped();
    >>>>>  };
    >>>>>
    >>>>> +static int get_codec_type(int fd, bool &is_enc) {
    >>>>
    >>>> { on the next line.
    >>>>
    >>>>> +     struct v4l2_capability vcap;
    >>>>> +
    >>>>> +     memset(&vcap,0,sizeof(vcap));
    >>>>
    >>>> Space after ,
    >>>>
    >>>> Please use the kernel coding style for these v4l utilities.
    >>>>
    >>> I ran the checkpatch script on this file and it didn't catch theses things.
    >>> Do you use checkpatch for v4l-utils ?
    >>
    >> No. As far as I can tell checkpatch skips cpp files so it can't be used for C++ files.
    >>
    >>>
    >>>>> +
    >>>>> +     int ret = ioctl(fd, VIDIOC_QUERYCAP, &vcap);
    >>>>
    >>>> Please use the cv4l_fd class. It comes with lots of helpers for all these ioctls
    >>>> and it already used in v4l2-ctl-streaming.cpp.
    >>>>
    >>>> In this function you can just do:
    >>>>
    >>>>         if (!fd.has_vid_m2m())
    >>>>                 return -1;
    >>>>
    >>>>> +     if(ret) {
    >>>>> +             fprintf(stderr, "get_codec_type: VIDIOC_QUERYCAP failed: %d\n", ret);
    >>>>> +             return ret;
    >>>>> +     }
    >>>>> +     unsigned int caps = vcap.capabilities;
    >>>>> +     if (caps & V4L2_CAP_DEVICE_CAPS)
    >>>>> +             caps = vcap.device_caps;
    >>>>> +     if(!(caps & V4L2_CAP_VIDEO_M2M) && !(caps & V4L2_CAP_VIDEO_M2M_MPLANE)) {
    >>>>> +             is_enc = false;
    >>>>> +             fprintf(stderr,"get_codec_type: not an M2M device\n");
    >>>>> +             return -1;
    >>>>> +     }
    >>>>> +
    >>>>> +     struct v4l2_fmtdesc fmt;
    >>>>> +     memset(&fmt,0,sizeof(fmt));
    >>>>> +     fmt.index = 0;
    >>>>> +     fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
    >>>>> +
    >>>>> +     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
    >>>>> +             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
    >>>>> +                     break;
    >>>>> +             fmt.index++;
    >>>>> +     }
    >>>>
    >>>> These tests aren't good enough. You need to enumerate over all formats.
    >>>> Easiest is to keep a tally of the total number of formats and how many
    >>>> are compressed.
    >>>>
    >>>> An encoder is a device where all output formats are uncompressed and
    >>>> all capture formats are compressed. It's the reverse for a decoder.
    >>>>
    >>>> If you get a mix on either side, or both sides are raw or both sides
    >>>> are compressed, then it isn't a codec.
    >>>>
    >>>>> +     if (ret) {
    >>>>> +             is_enc = true;
    >>>>> +             return 0;
    >>>>> +     }
    >>>>> +     memset(&fmt,0,sizeof(fmt));
    >>>>> +     fmt.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
    >>>>> +     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
    >>>>> +             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
    >>>>> +                     break;
    >>>>> +             fmt.index++;
    >>>>> +     }
    >>>>> +     if (ret) {
    >>>>> +             is_enc = false;
    >>>>> +             return 0;
    >>>>> +     }
    >>>>> +     fprintf(stderr, "get_codec_type: could no determine codec type\n");
    >>>>> +     return -1;
    >>>>> +}
    >>>>> +
    >>>>> +static void get_frame_dims(unsigned int &frame_width, unsigned int &frame_height) {
    >>>>> +
    >>>>> +     if(is_m2m_enc)
    >>>>> +             vidout_get_orig_from_set(frame_width, frame_height);
    >>>>> +     else
    >>>>> +             vidcap_get_orig_from_set(frame_width, frame_height);
    >>>>> +}
    >>>>> +
    >>>>> +static int get_visible_format(int fd, unsigned int &width, unsigned int &height) {
    >>>>> +     int ret = 0;
    >>>>> +     if(is_m2m_enc) {
    >>>>> +             struct v4l2_selection in_selection;
    >>>>> +
    >>>>> +             in_selection.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
    >>>>> +             in_selection.target = V4L2_SEL_TGT_CROP;
    >>>>> +
    >>>>> +             if ( (ret = ioctl(fd, VIDIOC_G_SELECTION, &in_selection)) != 0) {
    >>>>> +                     fprintf(stderr,"get_visible_format: error in g_selection ioctl: %d\n",ret);
    >>>>> +                     return ret;
    >>>>> +             }
    >>>>> +             width = in_selection.r.width;
    >>>>> +             height = in_selection.r.height;
    >>>>> +     }
    >>>>> +     else { //TODO - g_selection with COMPOSE should be used here when implemented in driver
    >>>>> +             vidcap_get_orig_from_set(width, height);
    >>>>> +     }
    >>>>> +     return 0;
    >>>>> +}
    >>>>> +
    >>>>> +
    >>>>>  void fps_timestamps::determine_field(int fd, unsigned type)
    >>>>>  {
    >>>>>       struct v4l2_format fmt = { };
    >>>>> @@ -419,7 +503,6 @@ static void print_buffer(FILE *f, struct v4l2_buffer &buf)
    >>>>>                       fprintf(f, "\t\tData Offset: %u\n", p->data_offset);
    >>>>>               }
    >>>>>       }
    >>>>> -
    >>>>>       fprintf(f, "\n");
    >>>>>  }
    >>>>>
    >>>>> @@ -657,7 +740,131 @@ void streaming_cmd(int ch, char *optarg)
    >>>>>       }
    >>>>>  }
    >>>>>
    >>>>> -static bool fill_buffer_from_file(cv4l_queue &q, cv4l_buffer &b, FILE *fin)
    >>>>> +bool padding(cv4l_fd &fd, cv4l_queue &q, unsigned char* buf, FILE *fpointer, unsigned &sz, unsigned &len, bool is_read)
    >>>>
    >>>> This should definitely be a static function. Also, this is not a very good name.
    >>>>
    >>>> Why not call it fill_padded_buffer_from_file()?
    >>>
    >>> This function is used both for reading from file for the encoder and
    >>> writing to file for the decoder.
    >>> Maybe it can be called read_write_padded_frame ?
    >>
    >> That would work.
    >>
    >>>
    >>>>
    >>>>> +{
    >>>>> +     cv4l_fmt fmt(q.g_type());
    >>>>
    >>>> No need to use q.g_type(). 'cv4l_fmt fmt;' is sufficient.
    >>>>
    >>>>> +     fd.g_fmt(fmt, q.g_type());
    >>>>
    >>>> After all, it's filled here.
    >>>>
    >>>>> +     const struct v4l2_fwht_pixfmt_info *vic_fmt = v4l2_fwht_find_pixfmt(fmt.g_pixelformat());
    >>>>
    >>>> This test should be moved to fill_buffer_from_file. If it is not an encoder and
    >>>> the pixelformat is not known (v4l2_fwht_find_pixfmt() returns NULL), then it should
    >>>> fallback to the old behavior. So this function should only be called when you have
    >>>> all the information about the pixelformat.
    >>>>
    >>>
    >>> This function is supposed to be called only for m2m encoder on the
    >>> output buffer and m2m decoder on the capture buffer
    >>> so vic_format is not NULL in those case.
    >>
    >> Actually, handling padding is not specific to codecs. Any video device can have
    >> cropping or composing.
    >>
    >> The generic rules are:
    >>
    >> 1) if a video output device supports TGT_CROP, then use that rectangle when reading
    >>    from a file.
    >>
    >> 2) if a video capture device supports TGT_COMPOSE, then use that rectangle when
    >>    writing to a file.
    >>
    >> The problem with this is that doing this requires v4l2-ctl to understand all the pixelformats.
    >> That's a lot more work so for now just use v4l2_fwht_find_pixfmt() which has the information
    >> needed for the most common formats.
    >>
    >> Anything not known by v4l2_fwht_find_pixfmt() can just fall back to the old behavior.
    >>
    > So I'm not sure what is the correct implementation here,
    > Currently this function suppose to be used only by m2m_enc/dec, I
    > actually have no idea
    > how other types of drivers work. What exactly should be the conditions
    > for calling this function ?

    Any driver that supports TGT_CROP for video output or TGT_COMPOSE for video capture
    should use that information when reading from a file or writing to a file in v4l2-ctl.

Quoted 5 lines

There is the field "have_selection" in v4l_fd, which is update here https://git.linuxtv.org/hverkuil/v4l-utils.git/tree/utils/common/v4l-helpers.h#n476
The selection target is set to crop for capture buffer and compose for output buffer, this is the opposite
from what you wrote and what I implemented in vicodec.
Also, why is the returned value compared to ENOTTY ? according to the docs, if the operation is not
supported it should return EINVAL.

No, EINVAL is returned if the ioctl is supported but the given values are invalid.

If an ioctl is not supported at all by a driver, then ENOTTY is returned.

I know, it's a weird error code, but this is historical UNIX error code:
https://en.wikipedia.org/wiki/Not_a_typewriter

The 'have_selection' test is still valid for codecs since it doesn't test if the target is correct, but it tests if the ioctl is implemented.

I could have just set sel.target to any target in that test, it's a bit overkill what happens there. The test assumes that the device is either a pure capture (i.e. webcam like) device or a pure video output device, and for such devices the target is valid. For M2M devices the targets are opposite from what non-M2M devices would implement. I.e. for webcams the cropping takes place in the hardware, and they typically do not support composing into a capture buffer.

Regards,

Hans

Dafna Hirschfeld

author

2018-12-22 16:51 UTC #8 lore raw

In reply to Hans Verkuil

Quoted 509 lines · Hans Verkuil

Quoted 10 lines · Dafna Hirschfeld

Quoted 5 lines
Quoted 1 line
+bool is_m2m_enc = false;
This should be static.
I'm assuming that in a future patch we'll get a is_m2m_dec as well?
I forgot that there can be more options other than m2m_enc/_dec. So actually adding is_m2m_dec is needed. Or I can define an enum with 3 possible values
IS_M2M_ENC, IS_M2M_DEC, NOT_M2M_DEV

I think using bools will make the code easier.

Quoted 18 lines

Quoted 18 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 117
Quoted 11 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+
 #define TS_WINDOW 241
 #define FILE_HDR_ID                  v4l2_fourcc('V', 'h', 'd', 'r')
@@ -108,6 +114,84 @@ public:
      unsigned dropped();
 };
 
+static int get_codec_type(int fd, bool &is_enc) {
{ on the next line.
Quoted 3 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +3 -0
+     struct v4l2_capability vcap;
+
+     memset(&vcap,0,sizeof(vcap));

Quoted 6 lines

Quoted 4 lines
Space after ,
Please use the kernel coding style for these v4l utilities.
I ran the checkpatch script on this file and it didn't catch theses things. Do you use checkpatch for v4l-utils ?

No. As far as I can tell checkpatch skips cpp files so it can't be used for C++ files.

Quoted 115 lines

Review public: · line 122

Quoted 3 lines

(quoted diff) +2 -0

+
+     int ret = ioctl(fd, VIDIOC_QUERYCAP, &vcap);

Please use the cv4l_fd class. It comes with lots of helpers for all these ioctls and it already used in v4l2-ctl-streaming.cpp.

In this function you can just do:

        if (!fd.has_vid_m2m())
                return -1;

Quoted 13 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +13 -0

+     if(ret) {
+             fprintf(stderr, "get_codec_type: VIDIOC_QUERYCAP failed: %d\n", ret);
+             return ret;
+     }
+     unsigned int caps = vcap.capabilities;
+     if (caps & V4L2_CAP_DEVICE_CAPS)
+             caps = vcap.device_caps;
+     if(!(caps & V4L2_CAP_VIDEO_M2M) && !(caps & V4L2_CAP_VIDEO_M2M_MPLANE)) {
+             is_enc = false;
+             fprintf(stderr,"get_codec_type: not an M2M device\n");
+             return -1;
+     }
+

utils/v4l2-ctl/v4l2-ctl-streaming.cpp public: · line 145

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+     struct v4l2_fmtdesc fmt;
+     memset(&fmt,0,sizeof(fmt));
+     fmt.index = 0;
+     fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }

These tests aren't good enough. You need to enumerate over all formats. Easiest is to keep a tally of the total number of formats and how many are compressed.

An encoder is a device where all output formats are uncompressed and all capture formats are compressed. It's the reverse for a decoder.

If you get a mix on either side, or both sides are raw or both sides are compressed, then it isn't a codec.

Quoted 61 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +49 -1

+     if (ret) {
+             is_enc = true;
+             return 0;
+     }
+     memset(&fmt,0,sizeof(fmt));
+     fmt.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+     while ((ret = ioctl(fd, VIDIOC_ENUM_FMT, &fmt)) == 0) {
+             if((fmt.flags & V4L2_FMT_FLAG_COMPRESSED) == 0)
+                     break;
+             fmt.index++;
+     }
+     if (ret) {
+             is_enc = false;
+             return 0;
+     }
+     fprintf(stderr, "get_codec_type: could no determine codec type\n");
+     return -1;
+}
+
+static void get_frame_dims(unsigned int &frame_width, unsigned int &frame_height) {
+
+     if(is_m2m_enc)
+             vidout_get_orig_from_set(frame_width, frame_height);
+     else
+             vidcap_get_orig_from_set(frame_width, frame_height);
+}
+
+static int get_visible_format(int fd, unsigned int &width, unsigned int &height) {
+     int ret = 0;
+     if(is_m2m_enc) {
+             struct v4l2_selection in_selection;
+
+             in_selection.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+             in_selection.target = V4L2_SEL_TGT_CROP;
+
+             if ( (ret = ioctl(fd, VIDIOC_G_SELECTION, &in_selection)) != 0) {
+                     fprintf(stderr,"get_visible_format: error in g_selection ioctl: %d\n",ret);
+                     return ret;
+             }
+             width = in_selection.r.width;
+             height = in_selection.r.height;
+     }
+     else { //TODO - g_selection with COMPOSE should be used here when implemented in driver
+             vidcap_get_orig_from_set(width, height);
+     }
+     return 0;
+}
+
+
 void fps_timestamps::determine_field(int fd, unsigned type)
 {
      struct v4l2_format fmt = { };

@@ -419,7 +503,6 @@ static void print_buffer(FILE *f, struct v4l2_buffer &buf)
                      fprintf(f, "\t\tData Offset: %u\n", p->data_offset);
              }
      }
-
      fprintf(f, "\n");
 }

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 743

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -657,7 +740,131 @@ void streaming_cmd(int ch, char *optarg)
      }
 }
 
-static bool fill_buffer_from_file(cv4l_queue &q, cv4l_buffer &b, FILE *fin)
+bool padding(cv4l_fd &fd, cv4l_queue &q, unsigned char* buf, FILE *fpointer, unsigned &sz, unsigned &len, bool is_read)

This should definitely be a static function. Also, this is not a very good name.

Quoted 5 lines

Quoted 1 line
Why not call it fill_padded_buffer_from_file()?
This function is used both for reading from file for the encoder and writing to file for the decoder.
Maybe it can be called read_write_padded_frame ?

That would work.

Quoted 19 lines

Quoted 9 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 745
Quoted 4 lines
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +2 -0
+{
+     cv4l_fmt fmt(q.g_type());
No need to use q.g_type(). 'cv4l_fmt fmt;' is sufficient.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 746
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     fd.g_fmt(fmt, q.g_type());
Quoted 9 lines
After all, it's filled here.
utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 747
Quoted 1 line
utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -0
+     const struct v4l2_fwht_pixfmt_info *vic_fmt = v4l2_fwht_find_pixfmt(fmt.g_pixelformat());
This test should be moved to fill_buffer_from_file. If it is not an encoder and the pixelformat is not known (v4l2_fwht_find_pixfmt() returns NULL), then it should fallback to the old behavior. So this function should only be called when you have all the information about the pixelformat.

Quoted 3 lines

This function is supposed to be called only for m2m encoder on the output buffer and m2m decoder on the capture buffer
so vic_format is not NULL in those case.

Actually, handling padding is not specific to codecs. Any video device can have cropping or composing.

The generic rules are:

1) if a video output device supports TGT_CROP, then use that rectangle when reading from a file.

2) if a video capture device supports TGT_COMPOSE, then use that rectangle when writing to a file.

Anything not known by v4l2_fwht_find_pixfmt() can just fall back to the old behavior.

Quoted 35 lines

Quoted 22 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +21 -0

+     unsigned coded_width = fmt.g_width();
+     unsigned coded_height = fmt.g_height();
+     unsigned real_width;
+     unsigned real_height;
+     unsigned char *buf_p = (unsigned char*) buf;
+
+     if(is_read) {
+             real_width  = frame_width;
+             real_height = frame_height;
+     }
+     else {
+             real_width  = visible_width;
+             real_height = visible_height;
+     }
+     sz = 0;
+     len = real_width * real_height * vic_fmt->sizeimage_mult / vic_fmt->sizeimage_div;
+     switch(vic_fmt->id) {
+     case V4L2_PIX_FMT_YUYV:
+     case V4L2_PIX_FMT_YVYU:
+     case V4L2_PIX_FMT_UYVY:
+     case V4L2_PIX_FMT_VYUY:

utils/v4l2-ctl/v4l2-ctl-streaming.cpp void streaming_cmd(int ch, char *optarg) · line 778

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +10 -0

+     case V4L2_PIX_FMT_RGB24:
+     case V4L2_PIX_FMT_HSV24:
+     case V4L2_PIX_FMT_BGR24:
+     case V4L2_PIX_FMT_RGB32:
+     case V4L2_PIX_FMT_XRGB32:
+     case V4L2_PIX_FMT_HSV32:
+     case V4L2_PIX_FMT_BGR32:
+     case V4L2_PIX_FMT_XBGR32:
+     case V4L2_PIX_FMT_ARGB32:
+     case V4L2_PIX_FMT_ABGR32:

I'd put this all under a 'default' case. I think GREY can also be added here.

Quoted 8 lines

Quoted 7 lines
What I would really like to see is that only the information from v4l2_fwht_pixfmt_info can be used here without requiring a switch.
I think all that is needed to do that is that struct v4l2_fwht_pixfmt_info is extended with a 'planes_num' field, which is 1 for interleaved formats, 2 for luma/interleaved chroma planar formats and 3 for luma/cr/cb planar formats.
So I should send a patch to the kernel code, adding this field ?

Yes.

Quoted 152 lines

Quoted 149 lines

Quoted 120 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +99 -2

+             for(unsigned i=0; i < real_height; i++) {
+                     unsigned int consume_sz = vic_fmt->bytesperline_mult*real_width;
+                     unsigned int wsz = 0;
+                     if(is_read)
+                             wsz = fread(buf_p, 1, consume_sz, fpointer);
+                     else
+                             wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                     sz += wsz;
+                     if(wsz == 0 && i == 0)
+                             break;
+                     if(wsz != consume_sz) {
+                             fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                             return false;
+                     }
+                     buf_p += vic_fmt->chroma_step*coded_width;
+             }
+     break;
+
+     case V4L2_PIX_FMT_NV12:
+     case V4L2_PIX_FMT_NV16:
+     case V4L2_PIX_FMT_NV24:
+     case V4L2_PIX_FMT_NV21:
+     case V4L2_PIX_FMT_NV61:
+     case V4L2_PIX_FMT_NV42:
+             for(unsigned plane_idx = 0; plane_idx < 2; plane_idx++) {
+                     unsigned h_div = (plane_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (plane_idx == 0) ? 1 : vic_fmt->width_div;
+                     unsigned step  =  (plane_idx == 0) ? vic_fmt->luma_alpha_step : vic_fmt->chroma_step;
+
+                     for(unsigned i=0; i <  real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = step * real_width / w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1,  consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && plane_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += step*coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+     break;
+     case V4L2_PIX_FMT_YUV420:
+     case V4L2_PIX_FMT_YUV422P:
+     case V4L2_PIX_FMT_YVU420:
+     case V4L2_PIX_FMT_GREY:
+             for(unsigned comp_idx = 0; comp_idx < vic_fmt->components_num; comp_idx++) {
+                     unsigned h_div = (comp_idx == 0) ? 1 : vic_fmt->height_div;
+                     unsigned w_div = (comp_idx == 0) ? 1 : vic_fmt->width_div;
+
+                     for(unsigned i=0; i < real_height/h_div; i++) {
+                             unsigned int wsz = 0;
+                             unsigned int consume_sz = real_width/w_div;
+                             if(is_read)
+                                     wsz = fread(buf_p, 1, consume_sz, fpointer);
+                             else
+                                     wsz = fwrite(buf_p, 1, consume_sz, fpointer);
+                             if(wsz == 0 && i == 0 && comp_idx == 0)
+                                     break;
+                             if(wsz != consume_sz) {
+                                     fprintf(stderr, "padding: needed %u bytes, got %u\n",consume_sz, wsz);
+                                     return true;
+                             }
+                             sz += wsz;
+                             buf_p += coded_width/w_div;
+                     }
+                     buf_p += (coded_width / w_div) * (coded_height - real_height) / h_div;
+
+                     if(sz == 0)
+                             break;
+             }
+             break;
+     default:
+             fprintf(stderr,"the format is not supported yet\n");
+             return false;
+     }
+     return true;
+}
+
+static bool fill_buffer_from_file(cv4l_fd &fd, cv4l_queue &q, cv4l_buffer &b, FILE *fin)
 {
      static bool first = true;
      static bool is_fwht = false;

@@ -785,7 +992,15 @@ restart:
                              return false;
                      }
              }
-             sz = fread(buf, 1, len, fin);
+
+             if(is_m2m_enc) {
+                     if(!padding(fd, q, (unsigned char*) buf, fin, sz, len, true))
+                             return false;
+             }
+             else {
+                     sz = fread(buf, 1, len, fin);
+             }
+
              if (first && sz != len) {
                      fprintf(stderr, "Insufficient data\n");
                      return false;

@@ -908,7 +1123,7 @@ static int do_setup_out_buffers(cv4l_fd &fd, cv4l_queue &q, FILE *fin, bool qbuf
                                      tpg_fillbuffer(&tpg, stream_out_std, j, (u8 *)q.g_dataptr(i, j));
                      }
              }
-             if (fin && !fill_buffer_from_file(q, buf, fin))
+             if (fin && !fill_buffer_from_file(fd, q, buf, fin))
                      return -2;
 
              if (qbuf) {

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1178

Quoted 5 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +1 -1

@@ -960,7 +1175,7 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
              if (fd.qbuf(buf))
                      return -1;
      }
-
+

Seems to be a whitespace only change, just drop this change.

Quoted 8 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +0 -1

      double ts_secs = buf.g_timestamp().tv_sec + buf.g_timestamp().tv_usec / 1000000.0;
      fps_ts.add_ts(ts_secs, buf.g_sequence(), buf.g_field());

@@ -1023,8 +1238,15 @@ static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index,
                      }
                      if (host_fd_to >= 0)
                              sz = fwrite(comp_ptr[j] + offset, 1, used, fout);
-                     else

utils/v4l2-ctl/v4l2-ctl-streaming.cpp static int do_handle_cap(cv4l_fd &fd, cv4l_queue &q, FILE *fout, int *index, · line 1249

Quoted 10 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +9 -1

-                             sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                     else {
+                             if(!is_m2m_enc) {
+                                     if(!padding(fd, q, (u8 *)q.g_dataptr(buf.g_index(), j) + offset, fout, sz, used, false))
+                                             return false;
+                             }
+                             else {
+                                     sz = fwrite((u8 *)q.g_dataptr(buf.g_index(), j) + offset, 1, used, fout);
+                             }
+                     }

This doesn't feel right.

Quoted 2 lines

I think a write_buffer_to_file() function should be introduced that deals with these variations.

Quoted 6 lines

Not sure what you meant, should I implement this if-else in another function ? The "padding" function is used both for reading and writing to/from padded buffer.
The condition for calling it here should be changed to "if(is_m2m_dec)" in which case "padding" will
write a raw frame to a file from the padded capture buffer.

static void write_buffer_to_file(cv4l_queue &q, cv4l_buffer &b, FILE *fout) {

#ifndef NO_STREAM_TO
        // code

#endif
}

And in do_handle_cap() drop the NO_STREAM_TO and replace it with a call to the new function:

        if (fout && (!stream_skip || ignore_count_skip) &&
            buf.g_bytesused(0) && !(buf.g_flags() & V4L2_BUF_FLAG_ERROR))
                write_buffer_to_file(q, buf, fout);

This can be done in a separate patch: first refactor the code, introducing the new function, then add support for handling padding.

Quoted 77 lines

Quoted 67 lines

utils/v4l2-ctl/v4l2-ctl-streaming.cpp +17 -2

                      if (sz != used)
                              fprintf(stderr, "%u != %u\n", sz, used);

@@ -1130,7 +1352,7 @@ static int do_handle_out(cv4l_fd &fd, cv4l_queue &q, FILE *fin, cv4l_buffer *cap
                      output_field = V4L2_FIELD_TOP;
      }
 
-     if (fin && !fill_buffer_from_file(q, buf, fin))
+     if (fin && !fill_buffer_from_file(fd, q, buf, fin))
              return -2;
 
      if (!fin && stream_out_refresh) {

@@ -1227,7 +1449,7 @@ static void streaming_set_cap(cv4l_fd &fd)
              }
              break;
      }
-
+
      memset(&sub, 0, sizeof(sub));
      sub.type = V4L2_EVENT_EOS;
      fd.subscribe_event(sub);

@@ -2031,6 +2253,21 @@ void streaming_set(cv4l_fd &fd, cv4l_fd &out_fd)
      int do_cap = options[OptStreamMmap] + options[OptStreamUser] + options[OptStreamDmaBuf];
      int do_out = options[OptStreamOutMmap] + options[OptStreamOutUser] + options[OptStreamOutDmaBuf];
 
+     int r = get_codec_type(fd.g_fd(), is_m2m_enc);
+     if(r) {
+             fprintf(stderr, "error checking codec type\n");
+             return;
+     }
+
+     r = get_visible_format(fd.g_fd(), visible_width, visible_height);
+
+     if(r) {
+             fprintf(stderr, "error getting the visible width\n");
+             return;
+     }
+
+     get_frame_dims(frame_width, frame_height);
+
      if (out_fd.g_fd() < 0) {
              out_capabilities = capabilities;
              out_priv_magic = priv_magic;

utils/v4l2-ctl/v4l2-ctl-vidcap.cpp +6 -0

index dc17a868..932f1fd2 100644
@@ -244,6 +244,12 @@ void vidcap_get(cv4l_fd &fd)
      }
 }
 
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}
+
+
 void vidcap_list(cv4l_fd &fd)
 {
      if (options[OptListFormats]) {

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +0 -0

index 5823df9c..05bd43ed 100644

utils/v4l2-ctl/v4l2-ctl-vidout.cpp void vidout_get(cv4l_fd &fd) · line 214

Quoted 7 lines

utils/v4l2-ctl/v4l2-ctl-vidout.cpp +4 -0

@@ -208,6 +208,11 @@ void vidout_get(cv4l_fd &fd)
      }
 }
 
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height) {
+     r_height = height;
+     r_width = width;
+}

Don't do this (same for vidcap_get_orig_from_set).

Quoted 4 lines

Quoted 3 lines
I think you just want to get the width and height from VIDIOC_G_FMT here, so why not just call that?
Those width/height are the values that are given by the user command.

Quoted 8 lines

Yes, but those values are used in ioctl calls to the driver, so rather than using those values you query the driver.
Quoted 2 lines
They are needed in order to
read raw frames line by line for the encoder.
Why not call G_FMT and G_SELECTION(TGT_CROP) to obtain that information?

The way vicodec is now implemented is that it does not hold the original width/height given in S_FMT but the coded once. But the userspace still needs these values in order to read raw frames from the file.
Not sure how to solve it.
Why does the userspace encoder need to query this values from the driver ? Since these are raw frames the userspace
should already know the dimensions.

Dafna

Quoted 49 lines

Please note that all the set and get options are all processed before the streaming options. So when you start streaming the driver is fully configured.
Quoted 32 lines
Maybe I can implement it by calling 'parse_fmt' in 'stream_cmd' function similar to how 'vidout_cmd' do it.
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-ctl/v4l2-ctl-vidout.cpp#n90
Quoted 26 lines
Remember that you can call v4l2-ctl without setting the output width and height if the defaults that the driver sets are already fine. In that case the width and height variables in this source are just 0.
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-vidout.cpp +1 -0
+
 void vidout_list(cv4l_fd &fd)
 {
      if (options[OptListOutFormats]) {
utils/v4l2-ctl/v4l2-ctl.h +0 -0
index 5a52a0a4..ab2994b2 100644
utils/v4l2-ctl/v4l2-ctl.h void vidout_cmd(int ch, char *optarg); · line 364
Quoted 8 lines
utils/v4l2-ctl/v4l2-ctl.h +2 -0
@@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg);
 void vidout_set(cv4l_fd &fd);
 void vidout_get(cv4l_fd &fd);
 void vidout_list(cv4l_fd &fd);
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
 
 // v4l2-ctl-overlay.cpp
 void overlay_usage(void);
This patch needs more work (not surprisingly, since it takes a bit of time to understand the v4l2-ctl source code).
Quoted 10 lines
Quoted 5 lines
Please stick to the kernel coding style! Using a different style makes it harder for me to review since my pattern matches routines in my brain no longer work optimally. It's like reading text with spelling mistakes, you cn stil undrstant iT, but it tekes moore teem. :-)
okei :)
Quoted 3 lines
Regards,
Hans
Regards,
Hans

Hans Verkuil

2018-12-22 17:11 UTC #9 lore raw

Quoted 8 lines · Dafna Hirschfeld

Quoted 7 lines · Hans Verkuil
Yes, but those values are used in ioctl calls to the driver, so rather than using those values you query the driver.
Quoted 2 lines
They are needed in order to
read raw frames line by line for the encoder.

Quoted 7 lines

Quoted 2 lines
Why not call G_FMT and G_SELECTION(TGT_CROP) to obtain that information?
The way vicodec is now implemented is that it does not hold the original width/height given in S_FMT but the coded once. But the userspace still needs these values in order to read raw frames from the file.
Not sure how to solve it.

When running v4l2-ctl you have to set both the width and height with the -x option, AND you set the selection crop rectangle to that same width and height.

So the selection crop rectangle has the actual visible width and height.

In fact, for a video output device in general you can call G_SELECTION(TGT_CROP) and use the returned width and height when reading from the file, or (if not supported) you use the width and height returned by G_FMT.

Quoted 3 lines

Why does the userspace encoder need to query this values from the driver ? Since these are raw frames the userspace should already know the dimensions.

This assumes that the user explicitly provides the visible width and height, but that doesn't have to be the case. In which case the drivers values should be used.

Also, m2m devices are special in that the configuration is stored per-filehandle, but that is not case for regular capture and output devices: the configuration is per device in that case, so you can set the width and height with v4l2-ctl and then in a new v4l2-ctl command query the width and height and you'll get back the width and height you set previously. Just try this with e.g. vivid and vim2m and see the difference in behavior.

BTW, right now when we set the output width and height for the encoder with the -x option we do not set the CROP rectangle with that width and height, we only clamp any existing rectangle.

This is right according to the current V4L2 spec, but it is not clear if this behavior makes sense for codecs. In the new year this is something we should discuss with others. Perhaps S_FMT for an encoder should automatically set the CROP target for the output as well using the original S_FMT width and height.

Regards,

Hans

Quoted 52 lines

Dafna
Quoted 49 lines
Please note that all the set and get options are all processed before the streaming options. So when you start streaming the driver is fully configured.
Quoted 32 lines
Maybe I can implement it by calling 'parse_fmt' in 'stream_cmd' function similar to how 'vidout_cmd' do it.
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-ctl/v4l2-ctl-vidout.cpp#n90
Quoted 26 lines
Remember that you can call v4l2-ctl without setting the output width and height if the defaults that the driver sets are already fine. In that case the width and height variables in this source are just 0.
Quoted 10 lines
utils/v4l2-ctl/v4l2-ctl-vidout.cpp +1 -0
+
 void vidout_list(cv4l_fd &fd)
 {
      if (options[OptListOutFormats]) {
utils/v4l2-ctl/v4l2-ctl.h +0 -0
index 5a52a0a4..ab2994b2 100644
utils/v4l2-ctl/v4l2-ctl.h void vidout_cmd(int ch, char *optarg); · line 364
Quoted 8 lines
utils/v4l2-ctl/v4l2-ctl.h +2 -0
@@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg);
 void vidout_set(cv4l_fd &fd);
 void vidout_get(cv4l_fd &fd);
 void vidout_list(cv4l_fd &fd);
+void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
+void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height);
 
 // v4l2-ctl-overlay.cpp
 void overlay_usage(void);
This patch needs more work (not surprisingly, since it takes a bit of time to understand the v4l2-ctl source code).
Quoted 10 lines
Quoted 5 lines
Please stick to the kernel coding style! Using a different style makes it harder for me to review since my pattern matches routines in my brain no longer work optimally. It's like reading text with spelling mistakes, you cn stil undrstant iT, but it tekes moore teem. :-)
okei :)
Quoted 3 lines
Regards,
Hans
Regards,
Hans