[PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Dafna Hirschfeld]

The new supported formats are
V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

The returned encoded format is chaned to support various numbers
of planes instead of assuming 3 planes.

The first patch adds new fields to structs.
The second patch adds support for V4L2_PIX_FMT_GREY.
The third patch adds support for V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

Changes from v3:

patch 1,3: - no change

patch 2:
- replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
- add TODO comment - handle the case where the encoded stream is different format
than the decoded
- allocate maximal space for the V4L2_PIX_FMT_FWHT format

with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'

Dafna Hirschfeld (3):
  media: vicodec: prepare support for various number of planes
  media: vicodec: Add support of greyscale format
  media: vicodec: Add support for 4 planes formats

 drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
 drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
 .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
 drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
 5 files changed, 182 insertions(+), 67 deletions(-)

-- 
2.17.1

[PATCH vicodec v4 1/3] media: vicodec: prepare support for various number of planes

[Dafna Hirschfeld]

Add fields to the structs `fwht_raw_frame`, `v4l2_fwht_pixfmts`
to support various number of planes - formats
with alpha channel that have 4 planes and greyscale formats
that have 1 plane.

Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
---
 drivers/media/platform/vicodec/codec-fwht.c   |  2 +-
 drivers/media/platform/vicodec/codec-fwht.h   |  5 +-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 47 ++++++++++---------
 .../media/platform/vicodec/codec-v4l2-fwht.h  |  3 +-
 drivers/media/platform/vicodec/vicodec-core.c |  2 +-
 5 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
index 36656031b295..4ee6d7e0fbe2 100644
--- a/drivers/media/platform/vicodec/codec-fwht.c
+++ b/drivers/media/platform/vicodec/codec-fwht.c
@@ -760,7 +760,7 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 	rlco_max = rlco + size / 2 - 256;
 	encoding = encode_plane(frm->luma, ref_frm->luma, &rlco, rlco_max, cf,
 				frm->height, frm->width,
-				frm->luma_step, is_intra, next_is_intra);
+				frm->luma_alpha_step, is_intra, next_is_intra);
 	if (encoding & FWHT_FRAME_UNENCODED)
 		encoding |= FWHT_LUMA_UNENCODED;
 	encoding &= ~FWHT_FRAME_UNENCODED;
diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h
index 3e9391fec5fe..743d78e112f8 100644
--- a/drivers/media/platform/vicodec/codec-fwht.h
+++ b/drivers/media/platform/vicodec/codec-fwht.h
@@ -104,9 +104,10 @@ struct fwht_raw_frame {
 	unsigned int width, height;
 	unsigned int width_div;
 	unsigned int height_div;
-	unsigned int luma_step;
+	unsigned int luma_alpha_step;
 	unsigned int chroma_step;
-	u8 *luma, *cb, *cr;
+	unsigned int components_num;
+	u8 *luma, *cb, *cr, *alpha;
 };
 
 #define FWHT_FRAME_PCODED	BIT(0)
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
index e5b68fb38aac..b842636e71a9 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
@@ -11,27 +11,28 @@
 #include "codec-v4l2-fwht.h"
 
 static const struct v4l2_fwht_pixfmt_info v4l2_fwht_pixfmts[] = {
-	{ V4L2_PIX_FMT_YUV420,  1, 3, 2, 1, 1, 2, 2 },
-	{ V4L2_PIX_FMT_YVU420,  1, 3, 2, 1, 1, 2, 2 },
-	{ V4L2_PIX_FMT_YUV422P, 1, 2, 1, 1, 1, 2, 1 },
-	{ V4L2_PIX_FMT_NV12,    1, 3, 2, 1, 2, 2, 2 },
-	{ V4L2_PIX_FMT_NV21,    1, 3, 2, 1, 2, 2, 2 },
-	{ V4L2_PIX_FMT_NV16,    1, 2, 1, 1, 2, 2, 1 },
-	{ V4L2_PIX_FMT_NV61,    1, 2, 1, 1, 2, 2, 1 },
-	{ V4L2_PIX_FMT_NV24,    1, 3, 1, 1, 2, 1, 1 },
-	{ V4L2_PIX_FMT_NV42,    1, 3, 1, 1, 2, 1, 1 },
-	{ V4L2_PIX_FMT_YUYV,    2, 2, 1, 2, 4, 2, 1 },
-	{ V4L2_PIX_FMT_YVYU,    2, 2, 1, 2, 4, 2, 1 },
-	{ V4L2_PIX_FMT_UYVY,    2, 2, 1, 2, 4, 2, 1 },
-	{ V4L2_PIX_FMT_VYUY,    2, 2, 1, 2, 4, 2, 1 },
-	{ V4L2_PIX_FMT_BGR24,   3, 3, 1, 3, 3, 1, 1 },
-	{ V4L2_PIX_FMT_RGB24,   3, 3, 1, 3, 3, 1, 1 },
-	{ V4L2_PIX_FMT_HSV24,   3, 3, 1, 3, 3, 1, 1 },
-	{ V4L2_PIX_FMT_BGR32,   4, 4, 1, 4, 4, 1, 1 },
-	{ V4L2_PIX_FMT_XBGR32,  4, 4, 1, 4, 4, 1, 1 },
-	{ V4L2_PIX_FMT_RGB32,   4, 4, 1, 4, 4, 1, 1 },
-	{ V4L2_PIX_FMT_XRGB32,  4, 4, 1, 4, 4, 1, 1 },
-	{ V4L2_PIX_FMT_HSV32,   4, 4, 1, 4, 4, 1, 1 },
+	{ V4L2_PIX_FMT_YUV420,  1, 3, 2, 1, 1, 2, 2, 3},
+	{ V4L2_PIX_FMT_YVU420,  1, 3, 2, 1, 1, 2, 2, 3},
+	{ V4L2_PIX_FMT_YUV422P, 1, 2, 1, 1, 1, 2, 1, 3},
+	{ V4L2_PIX_FMT_NV12,    1, 3, 2, 1, 2, 2, 2, 3},
+	{ V4L2_PIX_FMT_NV21,    1, 3, 2, 1, 2, 2, 2, 3},
+	{ V4L2_PIX_FMT_NV16,    1, 2, 1, 1, 2, 2, 1, 3},
+	{ V4L2_PIX_FMT_NV61,    1, 2, 1, 1, 2, 2, 1, 3},
+	{ V4L2_PIX_FMT_NV24,    1, 3, 1, 1, 2, 1, 1, 3},
+	{ V4L2_PIX_FMT_NV42,    1, 3, 1, 1, 2, 1, 1, 3},
+	{ V4L2_PIX_FMT_YUYV,    2, 2, 1, 2, 4, 2, 1, 3},
+	{ V4L2_PIX_FMT_YVYU,    2, 2, 1, 2, 4, 2, 1, 3},
+	{ V4L2_PIX_FMT_UYVY,    2, 2, 1, 2, 4, 2, 1, 3},
+	{ V4L2_PIX_FMT_VYUY,    2, 2, 1, 2, 4, 2, 1, 3},
+	{ V4L2_PIX_FMT_BGR24,   3, 3, 1, 3, 3, 1, 1, 3},
+	{ V4L2_PIX_FMT_RGB24,   3, 3, 1, 3, 3, 1, 1, 3},
+	{ V4L2_PIX_FMT_HSV24,   3, 3, 1, 3, 3, 1, 1, 3},
+	{ V4L2_PIX_FMT_BGR32,   4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_XBGR32,  4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_RGB32,   4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_XRGB32,  4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_HSV32,   4, 4, 1, 4, 4, 1, 1, 3},
+
 };
 
 const struct v4l2_fwht_pixfmt_info *v4l2_fwht_find_pixfmt(u32 pixelformat)
@@ -68,8 +69,10 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	rf.luma = p_in;
 	rf.width_div = info->width_div;
 	rf.height_div = info->height_div;
-	rf.luma_step = info->luma_step;
+	rf.luma_alpha_step = info->luma_alpha_step;
 	rf.chroma_step = info->chroma_step;
+	rf.alpha = NULL;
+	rf.components_num = info->components_num;
 
 	switch (info->id) {
 	case V4L2_PIX_FMT_YUV420:
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.h b/drivers/media/platform/vicodec/codec-v4l2-fwht.h
index 162465b78067..ed53e28d4f9c 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.h
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.h
@@ -13,11 +13,12 @@ struct v4l2_fwht_pixfmt_info {
 	unsigned int bytesperline_mult;
 	unsigned int sizeimage_mult;
 	unsigned int sizeimage_div;
-	unsigned int luma_step;
+	unsigned int luma_alpha_step;
 	unsigned int chroma_step;
 	/* Chroma plane subsampling */
 	unsigned int width_div;
 	unsigned int height_div;
+	unsigned int components_num;
 };
 
 struct v4l2_fwht_state {
diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index 1eb9132bfc85..fb6d5e9a06ab 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -61,7 +61,7 @@ struct pixfmt_info {
 };
 
 static const struct v4l2_fwht_pixfmt_info pixfmt_fwht = {
-	V4L2_PIX_FMT_FWHT, 0, 3, 1, 1, 1, 1, 1
+	V4L2_PIX_FMT_FWHT, 0, 3, 1, 1, 1, 1, 1, 0
 };
 
 static void vicodec_dev_release(struct device *dev)
-- 
2.17.1

[PATCH vicodec v4 2/3] media: vicodec: Add support of greyscale format

[Dafna Hirschfeld]

Add support for single plane greyscale format V4L2_PIX_FMT_GREY.
Also change the header of the encoded file to include the number
of components.

Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
---
 drivers/media/platform/vicodec/codec-fwht.c   | 56 +++++++++++--------
 drivers/media/platform/vicodec/codec-fwht.h   |  8 ++-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 44 ++++++++++++---
 drivers/media/platform/vicodec/vicodec-core.c | 25 +++++++--
 4 files changed, 93 insertions(+), 40 deletions(-)

diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
index 4ee6d7e0fbe2..1af9af84163e 100644
--- a/drivers/media/platform/vicodec/codec-fwht.c
+++ b/drivers/media/platform/vicodec/codec-fwht.c
@@ -753,9 +753,6 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 	__be16 *rlco = cf->rlc_data;
 	__be16 *rlco_max;
 	u32 encoding;
-	u32 chroma_h = frm->height / frm->height_div;
-	u32 chroma_w = frm->width / frm->width_div;
-	unsigned int chroma_size = chroma_h * chroma_w;
 
 	rlco_max = rlco + size / 2 - 256;
 	encoding = encode_plane(frm->luma, ref_frm->luma, &rlco, rlco_max, cf,
@@ -764,20 +761,27 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 	if (encoding & FWHT_FRAME_UNENCODED)
 		encoding |= FWHT_LUMA_UNENCODED;
 	encoding &= ~FWHT_FRAME_UNENCODED;
-	rlco_max = rlco + chroma_size / 2 - 256;
-	encoding |= encode_plane(frm->cb, ref_frm->cb, &rlco, rlco_max, cf,
+
+	if (frm->components_num >= 3) {
+		u32 chroma_h = frm->height / frm->height_div;
+		u32 chroma_w = frm->width / frm->width_div;
+		unsigned int chroma_size = chroma_h * chroma_w;
+
+		rlco_max = rlco + chroma_size / 2 - 256;
+		encoding |= encode_plane(frm->cb, ref_frm->cb, &rlco, rlco_max, cf,
 				 chroma_h, chroma_w,
 				 frm->chroma_step, is_intra, next_is_intra);
-	if (encoding & FWHT_FRAME_UNENCODED)
-		encoding |= FWHT_CB_UNENCODED;
-	encoding &= ~FWHT_FRAME_UNENCODED;
-	rlco_max = rlco + chroma_size / 2 - 256;
-	encoding |= encode_plane(frm->cr, ref_frm->cr, &rlco, rlco_max, cf,
+		if (encoding & FWHT_FRAME_UNENCODED)
+			encoding |= FWHT_CB_UNENCODED;
+		encoding &= ~FWHT_FRAME_UNENCODED;
+		rlco_max = rlco + chroma_size / 2 - 256;
+		encoding |= encode_plane(frm->cr, ref_frm->cr, &rlco, rlco_max, cf,
 				 chroma_h, chroma_w,
 				 frm->chroma_step, is_intra, next_is_intra);
-	if (encoding & FWHT_FRAME_UNENCODED)
-		encoding |= FWHT_CR_UNENCODED;
-	encoding &= ~FWHT_FRAME_UNENCODED;
+		if (encoding & FWHT_FRAME_UNENCODED)
+			encoding |= FWHT_CR_UNENCODED;
+		encoding &= ~FWHT_FRAME_UNENCODED;
+	}
 	cf->size = (rlco - cf->rlc_data) * sizeof(*rlco);
 	return encoding;
 }
@@ -836,20 +840,24 @@ static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
 }
 
 void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
-		       u32 hdr_flags)
+		       u32 hdr_flags, unsigned int components_num)
 {
 	const __be16 *rlco = cf->rlc_data;
-	u32 h = cf->height / 2;
-	u32 w = cf->width / 2;
 
-	if (hdr_flags & FWHT_FL_CHROMA_FULL_HEIGHT)
-		h *= 2;
-	if (hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH)
-		w *= 2;
 	decode_plane(cf, &rlco, ref->luma, cf->height, cf->width,
 		     hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED);
-	decode_plane(cf, &rlco, ref->cb, h, w,
-		     hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED);
-	decode_plane(cf, &rlco, ref->cr, h, w,
-		     hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
+
+	if (components_num >= 3) {
+		u32 h = cf->height;
+		u32 w = cf->width;
+
+		if (!(hdr_flags & FWHT_FL_CHROMA_FULL_HEIGHT))
+			h /= 2;
+		if (!(hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH))
+			w /= 2;
+		decode_plane(cf, &rlco, ref->cb, h, w,
+			hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED);
+		decode_plane(cf, &rlco, ref->cr, h, w,
+			hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
+	}
 }
diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h
index 743d78e112f8..bde11fb93f26 100644
--- a/drivers/media/platform/vicodec/codec-fwht.h
+++ b/drivers/media/platform/vicodec/codec-fwht.h
@@ -56,7 +56,7 @@
 #define FWHT_MAGIC1 0x4f4f4f4f
 #define FWHT_MAGIC2 0xffffffff
 
-#define FWHT_VERSION 1
+#define FWHT_VERSION 2
 
 /* Set if this is an interlaced format */
 #define FWHT_FL_IS_INTERLACED		BIT(0)
@@ -76,6 +76,10 @@
 #define FWHT_FL_CHROMA_FULL_HEIGHT	BIT(7)
 #define FWHT_FL_CHROMA_FULL_WIDTH	BIT(8)
 
+/* A 4-values flag - the number of components - 1 */
+#define FWHT_FL_COMPONENTS_NUM_MSK	GENMASK(17, 16)
+#define FWHT_FL_COMPONENTS_NUM_OFFSET	16
+
 struct fwht_cframe_hdr {
 	u32 magic1;
 	u32 magic2;
@@ -121,6 +125,6 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 		      struct fwht_cframe *cf,
 		      bool is_intra, bool next_is_intra);
 void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
-		       u32 hdr_flags);
+		       u32 hdr_flags, unsigned int components_num);
 
 #endif
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
index b842636e71a9..7dc3918a017e 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
@@ -11,6 +11,7 @@
 #include "codec-v4l2-fwht.h"
 
 static const struct v4l2_fwht_pixfmt_info v4l2_fwht_pixfmts[] = {
+	{ V4L2_PIX_FMT_GREY,    1, 1, 1, 1, 0, 1, 1, 1},
 	{ V4L2_PIX_FMT_YUV420,  1, 3, 2, 1, 1, 2, 2, 3},
 	{ V4L2_PIX_FMT_YVU420,  1, 3, 2, 1, 1, 2, 2, 3},
 	{ V4L2_PIX_FMT_YUV422P, 1, 2, 1, 1, 1, 2, 1, 3},
@@ -75,6 +76,10 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	rf.components_num = info->components_num;
 
 	switch (info->id) {
+	case V4L2_PIX_FMT_GREY:
+		rf.cb = NULL;
+		rf.cr = NULL;
+		break;
 	case V4L2_PIX_FMT_YUV420:
 		rf.cb = rf.luma + size;
 		rf.cr = rf.cb + size / 4;
@@ -165,6 +170,7 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	p_hdr->version = htonl(FWHT_VERSION);
 	p_hdr->width = htonl(cf.width);
 	p_hdr->height = htonl(cf.height);
+	flags |= (info->components_num - 1) << FWHT_FL_COMPONENTS_NUM_OFFSET;
 	if (encoding & FWHT_LUMA_UNENCODED)
 		flags |= FWHT_FL_LUMA_IS_UNCOMPRESSED;
 	if (encoding & FWHT_CB_UNENCODED)
@@ -195,6 +201,8 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	struct fwht_cframe_hdr *p_hdr;
 	struct fwht_cframe cf;
 	u8 *p;
+	unsigned int components_num = 3;
+	unsigned int version;
 
 	if (!state->info)
 		return -EINVAL;
@@ -202,16 +210,16 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	p_hdr = (struct fwht_cframe_hdr *)p_in;
 	cf.width = ntohl(p_hdr->width);
 	cf.height = ntohl(p_hdr->height);
-	flags = ntohl(p_hdr->flags);
-	state->colorspace = ntohl(p_hdr->colorspace);
-	state->xfer_func = ntohl(p_hdr->xfer_func);
-	state->ycbcr_enc = ntohl(p_hdr->ycbcr_enc);
-	state->quantization = ntohl(p_hdr->quantization);
-	cf.rlc_data = (__be16 *)(p_in + sizeof(*p_hdr));
+
+	version = ntohl(p_hdr->version);
+	if (!version || version > FWHT_VERSION) {
+		pr_err("version %d is not supported, current version is %d\n",
+				version, FWHT_VERSION);
+		return -EINVAL;
+	}
 
 	if (p_hdr->magic1 != FWHT_MAGIC1 ||
 	    p_hdr->magic2 != FWHT_MAGIC2 ||
-	    ntohl(p_hdr->version) != FWHT_VERSION ||
 	    (cf.width & 7) || (cf.height & 7))
 		return -EINVAL;
 
@@ -219,14 +227,34 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	if (cf.width != state->width || cf.height != state->height)
 		return -EINVAL;
 
+	flags = ntohl(p_hdr->flags);
+
+	if (version == FWHT_VERSION) {
+		components_num = 1 + ((flags & FWHT_FL_COMPONENTS_NUM_MSK) >>
+			FWHT_FL_COMPONENTS_NUM_OFFSET);
+	}
+
+	state->colorspace = ntohl(p_hdr->colorspace);
+	state->xfer_func = ntohl(p_hdr->xfer_func);
+	state->ycbcr_enc = ntohl(p_hdr->ycbcr_enc);
+	state->quantization = ntohl(p_hdr->quantization);
+	cf.rlc_data = (__be16 *)(p_in + sizeof(*p_hdr));
+
 	if (!(flags & FWHT_FL_CHROMA_FULL_WIDTH))
 		chroma_size /= 2;
 	if (!(flags & FWHT_FL_CHROMA_FULL_HEIGHT))
 		chroma_size /= 2;
 
-	fwht_decode_frame(&cf, &state->ref_frame, flags);
+	fwht_decode_frame(&cf, &state->ref_frame, flags, components_num);
 
+	/*
+	 * TODO - handle the case where the compressed stream encodes a different
+	 * format than the requested decoded format.
+	 */
 	switch (state->info->id) {
+	case V4L2_PIX_FMT_GREY:
+		memcpy(p_out, state->ref_frame.luma, size);
+		break;
 	case V4L2_PIX_FMT_YUV420:
 	case V4L2_PIX_FMT_YUV422P:
 		memcpy(p_out, state->ref_frame.luma, size);
diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index fb6d5e9a06ab..92bc68694a21 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -993,6 +993,16 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 	unsigned int size = q_data->width * q_data->height;
 	const struct v4l2_fwht_pixfmt_info *info = q_data->info;
 	unsigned int chroma_div = info->width_div * info->height_div;
+	unsigned int total_planes_size;
+
+	/*
+	 * we don't know ahead how many components are in the encoding type
+	 * V4L2_PIX_FMT_FWHT, so we will allocate space for 3 planes.
+	 */
+	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num >= 3)
+		total_planes_size = size + 2 * (size / chroma_div);
+	else
+		total_planes_size = size;
 
 	q_data->sequence = 0;
 
@@ -1002,10 +1012,8 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 	state->width = q_data->width;
 	state->height = q_data->height;
 	state->ref_frame.width = state->ref_frame.height = 0;
-	state->ref_frame.luma = kvmalloc(size + 2 * size / chroma_div,
-					 GFP_KERNEL);
-	ctx->comp_max_size = size + 2 * size / chroma_div +
-			     sizeof(struct fwht_cframe_hdr);
+	state->ref_frame.luma = kvmalloc(total_planes_size, GFP_KERNEL);
+	ctx->comp_max_size = total_planes_size + sizeof(struct fwht_cframe_hdr);
 	state->compressed_frame = kvmalloc(ctx->comp_max_size, GFP_KERNEL);
 	if (!state->ref_frame.luma || !state->compressed_frame) {
 		kvfree(state->ref_frame.luma);
@@ -1013,8 +1021,13 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 		vicodec_return_bufs(q, VB2_BUF_STATE_QUEUED);
 		return -ENOMEM;
 	}
-	state->ref_frame.cb = state->ref_frame.luma + size;
-	state->ref_frame.cr = state->ref_frame.cb + size / chroma_div;
+	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num >= 3) {
+		state->ref_frame.cb = state->ref_frame.luma + size;
+		state->ref_frame.cr = state->ref_frame.cb + size / chroma_div;
+	} else {
+		state->ref_frame.cb = NULL;
+		state->ref_frame.cr = NULL;
+	}
 	ctx->last_src_buf = NULL;
 	ctx->last_dst_buf = NULL;
 	state->gop_cnt = 0;
-- 
2.17.1

[PATCH vicodec v4 3/3] media: vicodec: Add support for 4 planes formats

[Dafna Hirschfeld]

Add support for formats with 4 planes: V4L2_PIX_FMT_ABGR32,
V4L2_PIX_FMT_ARGB32.
Also add alpha plane related flags to the header of the encoded file.

Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
---
 drivers/media/platform/vicodec/codec-fwht.c   | 15 +++++++++
 drivers/media/platform/vicodec/codec-fwht.h   |  2 ++
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 32 +++++++++++++++++++
 drivers/media/platform/vicodec/vicodec-core.c | 12 +++++--
 4 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
index 1af9af84163e..9513374e8f44 100644
--- a/drivers/media/platform/vicodec/codec-fwht.c
+++ b/drivers/media/platform/vicodec/codec-fwht.c
@@ -782,6 +782,17 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 			encoding |= FWHT_CR_UNENCODED;
 		encoding &= ~FWHT_FRAME_UNENCODED;
 	}
+
+	if (frm->components_num == 4) {
+		rlco_max = rlco + size / 2 - 256;
+		encoding = encode_plane(frm->alpha, ref_frm->alpha, &rlco, rlco_max, cf,
+				frm->height, frm->width,
+				frm->luma_alpha_step, is_intra, next_is_intra);
+		if (encoding & FWHT_FRAME_UNENCODED)
+			encoding |= FWHT_ALPHA_UNENCODED;
+		encoding &= ~FWHT_FRAME_UNENCODED;
+	}
+
 	cf->size = (rlco - cf->rlc_data) * sizeof(*rlco);
 	return encoding;
 }
@@ -860,4 +871,8 @@ void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
 		decode_plane(cf, &rlco, ref->cr, h, w,
 			hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
 	}
+
+	if (components_num == 4)
+		decode_plane(cf, &rlco, ref->alpha, cf->height, cf->width,
+		     hdr_flags & FWHT_FL_ALPHA_IS_UNCOMPRESSED);
 }
diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h
index bde11fb93f26..90ff8962fca7 100644
--- a/drivers/media/platform/vicodec/codec-fwht.h
+++ b/drivers/media/platform/vicodec/codec-fwht.h
@@ -75,6 +75,7 @@
 #define FWHT_FL_CR_IS_UNCOMPRESSED	BIT(6)
 #define FWHT_FL_CHROMA_FULL_HEIGHT	BIT(7)
 #define FWHT_FL_CHROMA_FULL_WIDTH	BIT(8)
+#define FWHT_FL_ALPHA_IS_UNCOMPRESSED	BIT(9)
 
 /* A 4-values flag - the number of components - 1 */
 #define FWHT_FL_COMPONENTS_NUM_MSK	GENMASK(17, 16)
@@ -119,6 +120,7 @@ struct fwht_raw_frame {
 #define FWHT_LUMA_UNENCODED	BIT(2)
 #define FWHT_CB_UNENCODED	BIT(3)
 #define FWHT_CR_UNENCODED	BIT(4)
+#define FWHT_ALPHA_UNENCODED	BIT(5)
 
 u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 		      struct fwht_raw_frame *ref_frm,
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
index 7dc3918a017e..b53655a8cef6 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
@@ -33,6 +33,8 @@ static const struct v4l2_fwht_pixfmt_info v4l2_fwht_pixfmts[] = {
 	{ V4L2_PIX_FMT_RGB32,   4, 4, 1, 4, 4, 1, 1, 3},
 	{ V4L2_PIX_FMT_XRGB32,  4, 4, 1, 4, 4, 1, 1, 3},
 	{ V4L2_PIX_FMT_HSV32,   4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_ARGB32,  4, 4, 1, 4, 4, 1, 1, 4},
+	{ V4L2_PIX_FMT_ABGR32,  4, 4, 1, 4, 4, 1, 1, 4},
 
 };
 
@@ -146,6 +148,18 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 		rf.cr = rf.cb + 2;
 		rf.luma++;
 		break;
+	case V4L2_PIX_FMT_ARGB32:
+		rf.alpha = rf.luma;
+		rf.cr = rf.luma + 1;
+		rf.cb = rf.cr + 2;
+		rf.luma += 2;
+		break;
+	case V4L2_PIX_FMT_ABGR32:
+		rf.cb = rf.luma;
+		rf.cr = rf.cb + 2;
+		rf.luma++;
+		rf.alpha = rf.cr + 1;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -177,6 +191,8 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 		flags |= FWHT_FL_CB_IS_UNCOMPRESSED;
 	if (encoding & FWHT_CR_UNENCODED)
 		flags |= FWHT_FL_CR_IS_UNCOMPRESSED;
+	if (encoding & FWHT_ALPHA_UNENCODED)
+		flags |= FWHT_FL_ALPHA_IS_UNCOMPRESSED;
 	if (rf.height_div == 1)
 		flags |= FWHT_FL_CHROMA_FULL_HEIGHT;
 	if (rf.width_div == 1)
@@ -356,6 +372,22 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 			*p++ = 0;
 		}
 		break;
+	case V4L2_PIX_FMT_ARGB32:
+		for (i = 0, p = p_out; i < size; i++) {
+			*p++ = state->ref_frame.alpha[i];
+			*p++ = state->ref_frame.cr[i];
+			*p++ = state->ref_frame.luma[i];
+			*p++ = state->ref_frame.cb[i];
+		}
+		break;
+	case V4L2_PIX_FMT_ABGR32:
+		for (i = 0, p = p_out; i < size; i++) {
+			*p++ = state->ref_frame.cb[i];
+			*p++ = state->ref_frame.luma[i];
+			*p++ = state->ref_frame.cr[i];
+			*p++ = state->ref_frame.alpha[i];
+		}
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index 92bc68694a21..5ae876238e13 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -997,9 +997,11 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 
 	/*
 	 * we don't know ahead how many components are in the encoding type
-	 * V4L2_PIX_FMT_FWHT, so we will allocate space for 3 planes.
+	 * V4L2_PIX_FMT_FWHT, so we will allocate space for 4 planes.
 	 */
-	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num >= 3)
+	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num == 4)
+		total_planes_size = 2 * size + 2 * (size / chroma_div);
+	else if (info->components_num == 3)
 		total_planes_size = size + 2 * (size / chroma_div);
 	else
 		total_planes_size = size;
@@ -1028,6 +1030,12 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 		state->ref_frame.cb = NULL;
 		state->ref_frame.cr = NULL;
 	}
+
+	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num == 4)
+		state->ref_frame.alpha = state->ref_frame.cr + size / chroma_div;
+	else
+		state->ref_frame.alpha = NULL;
+
 	ctx->last_src_buf = NULL;
 	ctx->last_dst_buf = NULL;
 	state->gop_cnt = 0;
-- 
2.17.1

Re: [Outreachy kernel] [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Ezequiel Garcia]

Hello Dafna,

Thanks for the patches.

Just out of curiosity.  Why these patches havent't been submitted to
the media mailing list?

Also, how are you testing these changes?

Thanks,
Ezequiel

On Mon, 5 Nov 2018 at 18:14, Dafna Hirschfeld <dafna3@gmail.com> wrote:
>
> The new supported formats are
> V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.
>
> The returned encoded format is chaned to support various numbers
> of planes instead of assuming 3 planes.
>
> The first patch adds new fields to structs.
> The second patch adds support for V4L2_PIX_FMT_GREY.
> The third patch adds support for V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.
>
> Changes from v3:
>
> patch 1,3: - no change
>
> patch 2:
> - replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
> - add TODO comment - handle the case where the encoded stream is different format
> than the decoded
> - allocate maximal space for the V4L2_PIX_FMT_FWHT format
>
> with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'
>
> Dafna Hirschfeld (3):
>   media: vicodec: prepare support for various number of planes
>   media: vicodec: Add support of greyscale format
>   media: vicodec: Add support for 4 planes formats
>
>  drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
>  drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
>  .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
>  .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
>  drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
>  5 files changed, 182 insertions(+), 67 deletions(-)
>
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/cover.1541451484.git.dafna3%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.



-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [Outreachy kernel] [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Dafna Hirschfeld]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

On Thu, Nov 8, 2018 at 2:51 AM Ezequiel Garcia <
ezequiel@vanguardiasur.com.ar> wrote:

> Hello Dafna,
>
> Thanks for the patches.
>
> Just out of curiosity.  Why these patches havent't been submitted to
> the media mailing list?
>
> Hi,
I wasn't sure if I should send it to the media mailing list, since this
part of outreachy application.


Also, how are you testing these changes?
>

Based on Helen's decoder:
https://gitlab.collabora.com/koike/v4l2-codec

I extended it to include encoders and decoders for the new supported
formats.

testing formats with alpha plane:
https://github.com/kamomil/outreachy/blob/master/argb-and-abgr-full-example.sh

testing greyscale:
https://github.com/kamomil/outreachy/blob/master/greyscale-full-example.sh

Dafna


> Thanks,
> Ezequiel
>
> On Mon, 5 Nov 2018 at 18:14, Dafna Hirschfeld <dafna3@gmail.com> wrote:
> >
> > The new supported formats are
> > V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.
> >
> > The returned encoded format is chaned to support various numbers
> > of planes instead of assuming 3 planes.
> >
> > The first patch adds new fields to structs.
> > The second patch adds support for V4L2_PIX_FMT_GREY.
> > The third patch adds support for V4L2_PIX_FMT_ARGB32,
> V4L2_PIX_FMT_ABGR32.
> >
> > Changes from v3:
> >
> > patch 1,3: - no change
> >
> > patch 2:
> > - replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
> > - add TODO comment - handle the case where the encoded stream is
> different format
> > than the decoded
> > - allocate maximal space for the V4L2_PIX_FMT_FWHT format
> >
> > with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'
> >
> > Dafna Hirschfeld (3):
> >   media: vicodec: prepare support for various number of planes
> >   media: vicodec: Add support of greyscale format
> >   media: vicodec: Add support for 4 planes formats
> >
> >  drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
> >  drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
> >  .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
> >  .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
> >  drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
> >  5 files changed, 182 insertions(+), 67 deletions(-)
> >
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/cover.1541451484.git.dafna3%40gmail.com
> .
> > For more options, visit https://groups.google.com/d/optout.
>
>
>
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
>

[-- Attachment #2: Type: text/html, Size: 4854 bytes --]

Re: [Outreachy kernel] [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Sasha Levin]

On Thu, Nov 08, 2018 at 10:10:10AM +0200, Dafna Hirschfeld wrote:
>On Thu, Nov 8, 2018 at 2:51 AM Ezequiel Garcia <
>ezequiel@vanguardiasur.com.ar> wrote:
>
>> Hello Dafna,
>>
>> Thanks for the patches.
>>
>> Just out of curiosity.  Why these patches havent't been submitted to
>> the media mailing list?
>>
>> Hi,
>I wasn't sure if I should send it to the media mailing list, since this
>part of outreachy application.

In general, for any patch you send to any subsystem please Cc all the
relevant mailing lists and maintainers. For Outreachy application you
already did that (by Cc'ing Greg), you just need to keep doing the same
as you continue your work on other parts of the kernel.

>Also, how are you testing these changes?
>>
>
>Based on Helen's decoder:
>https://gitlab.collabora.com/koike/v4l2-codec
>
>I extended it to include encoders and decoders for the new supported
>formats.
>
>testing formats with alpha plane:
>https://github.com/kamomil/outreachy/blob/master/argb-and-abgr-full-example.sh
>
>testing greyscale:
>https://github.com/kamomil/outreachy/blob/master/greyscale-full-example.sh

It's awesome seeing these testsuites, it gives reviewers confidence that
your patch is well tested and they can focus on other parts of the
review process rather than check for the basic correctness of the patch.

Please include links such as these and indicate how you tested your code
in your future patches.

--
Thanks,
Sasha

Re: [Outreachy kernel] [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Ezequiel Garcia]

On Thu, 8 Nov 2018 at 14:54, Sasha Levin <sashal@kernel.org> wrote:
>
> On Thu, Nov 08, 2018 at 10:10:10AM +0200, Dafna Hirschfeld wrote:
> >On Thu, Nov 8, 2018 at 2:51 AM Ezequiel Garcia <
> >ezequiel@vanguardiasur.com.ar> wrote:
> >
> >> Hello Dafna,
> >>
> >> Thanks for the patches.
> >>
> >> Just out of curiosity.  Why these patches havent't been submitted to
> >> the media mailing list?
> >>
> >> Hi,
> >I wasn't sure if I should send it to the media mailing list, since this
> >part of outreachy application.
>
> In general, for any patch you send to any subsystem please Cc all the
> relevant mailing lists and maintainers. For Outreachy application you
> already did that (by Cc'ing Greg), you just need to keep doing the same
> as you continue your work on other parts of the kernel.
>

Let's Cc the mailing list now, as these patches look good, and the
test scripts look pretty decent too ;-)

> >Also, how are you testing these changes?
> >>
> >
> >Based on Helen's decoder:
> >https://gitlab.collabora.com/koike/v4l2-codec
> >
> >I extended it to include encoders and decoders for the new supported
> >formats.
> >
> >testing formats with alpha plane:
> >https://github.com/kamomil/outreachy/blob/master/argb-and-abgr-full-example.sh
> >
> >testing greyscale:
> >https://github.com/kamomil/outreachy/blob/master/greyscale-full-example.sh
>
> It's awesome seeing these testsuites, it gives reviewers confidence that
> your patch is well tested and they can focus on other parts of the
> review process rather than check for the basic correctness of the patch.
>
> Please include links such as these and indicate how you tested your code
> in your future patches.
>

+1

Thanks!
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Hans Verkuil]

Hi Dafna,

This patch series looks very nice!

Can you repost this and add a CC to linux-media?

I'm happy to take this.

The TODO that is in the decoder code (what to do if the fwht bitstream
encodes fewer/more planes than the requested raw frame format) is
something that needs to be implemented later as part of making vicodec
compliant to the stateful codec specification.

Regards,

	Hans

On 11/05/18 22:14, Dafna Hirschfeld wrote:
> The new supported formats are
> V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.
> 
> The returned encoded format is chaned to support various numbers
> of planes instead of assuming 3 planes.
> 
> The first patch adds new fields to structs.
> The second patch adds support for V4L2_PIX_FMT_GREY.
> The third patch adds support for V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.
> 
> Changes from v3:
> 
> patch 1,3: - no change
> 
> patch 2:
> - replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
> - add TODO comment - handle the case where the encoded stream is different format
> than the decoded
> - allocate maximal space for the V4L2_PIX_FMT_FWHT format
> 
> with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'
> 
> Dafna Hirschfeld (3):
>   media: vicodec: prepare support for various number of planes
>   media: vicodec: Add support of greyscale format
>   media: vicodec: Add support for 4 planes formats
> 
>  drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
>  drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
>  .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
>  .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
>  drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
>  5 files changed, 182 insertions(+), 67 deletions(-)
> 

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Dafna Hirschfeld]

[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]

On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Hi Dafna,
>
> This patch series looks very nice!
>
> thanks:)

> Can you repost this and add a CC to linux-media?
>
> I will

I'm happy to take this.
>
The TODO that is in the decoder code (what to do if the fwht bitstream
> encodes fewer/more planes than the requested raw frame format) is
> something that needs to be implemented later as part of making vicodec
> compliant to the stateful codec specification.
>
> I tried to look in the docs what should be done in that case and didn't
find.
The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the
supported formats based on the OUTPUT buffers
but this is only optional.

Dafna

Regards,
>
>         Hans
>
> On 11/05/18 22:14, Dafna Hirschfeld wrote:
> > The new supported formats are
> > V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.
> >
> > The returned encoded format is chaned to support various numbers
> > of planes instead of assuming 3 planes.
> >
> > The first patch adds new fields to structs.
> > The second patch adds support for V4L2_PIX_FMT_GREY.
> > The third patch adds support for V4L2_PIX_FMT_ARGB32,
> V4L2_PIX_FMT_ABGR32.
> >
> > Changes from v3:
> >
> > patch 1,3: - no change
> >
> > patch 2:
> > - replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
> > - add TODO comment - handle the case where the encoded stream is
> different format
> > than the decoded
> > - allocate maximal space for the V4L2_PIX_FMT_FWHT format
> >
> > with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'
> >
> > Dafna Hirschfeld (3):
> >   media: vicodec: prepare support for various number of planes
> >   media: vicodec: Add support of greyscale format
> >   media: vicodec: Add support for 4 planes formats
> >
> >  drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
> >  drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
> >  .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
> >  .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
> >  drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
> >  5 files changed, 182 insertions(+), 67 deletions(-)
> >
>
>

[-- Attachment #2: Type: text/html, Size: 3438 bytes --]

[PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Dafna Hirschfeld]

The new supported formats are
V4L2_PIX_FMT_GREY, V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

The returned encoded format is chaned to support various numbers
of planes instead of assuming 3 planes.

The first patch adds new fields to structs.
The second patch adds support for V4L2_PIX_FMT_GREY.
The third patch adds support for V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32.

The code used to test this patch is https://github.com/kamomil/outreachy
The script I used to test greyscale support:
https://github.com/kamomil/outreachy/blob/master/greyscale-full-example.sh 
The script I used to test argb/abgr:
https://github.com/kamomil/outreachy/blob/master/argb-and-abgr-full-example.sh

Changes from v3:

patch 1,3: - no change

patch 2:
- replace the 2-bit flag FWHT_FL_COMPONENTS_NUM_BIT[01] with GENMASK
- add TODO comment - handle the case where the encoded stream is different format
than the decoded
- allocate maximal space for the V4L2_PIX_FMT_FWHT format

with the test 'flags & FWHT_FL_COMPONENTS_NUM_BIT[01]'

Dafna Hirschfeld (3):
  media: vicodec: prepare support for various number of planes
  media: vicodec: Add support of greyscale format
  media: vicodec: Add support for 4 planes formats

 drivers/media/platform/vicodec/codec-fwht.c   |  73 +++++++----
 drivers/media/platform/vicodec/codec-fwht.h   |  15 ++-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 123 +++++++++++++-----
 .../media/platform/vicodec/codec-v4l2-fwht.h  |   3 +-
 drivers/media/platform/vicodec/vicodec-core.c |  35 ++++-
 5 files changed, 182 insertions(+), 67 deletions(-)

-- 
2.17.1

[PATCH vicodec v4 1/3] media: vicodec: prepare support for various number of planes

[Dafna Hirschfeld]

Add fields to the structs `fwht_raw_frame`, `v4l2_fwht_pixfmts`
to support various number of planes - formats
with alpha channel that have 4 planes and greyscale formats
that have 1 plane.

Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
---
 drivers/media/platform/vicodec/codec-fwht.c   |  2 +-
 drivers/media/platform/vicodec/codec-fwht.h   |  5 +-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 47 ++++++++++---------
 .../media/platform/vicodec/codec-v4l2-fwht.h  |  3 +-
 drivers/media/platform/vicodec/vicodec-core.c |  2 +-
 5 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
index 36656031b295..4ee6d7e0fbe2 100644
--- a/drivers/media/platform/vicodec/codec-fwht.c
+++ b/drivers/media/platform/vicodec/codec-fwht.c
@@ -760,7 +760,7 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 	rlco_max = rlco + size / 2 - 256;
 	encoding = encode_plane(frm->luma, ref_frm->luma, &rlco, rlco_max, cf,
 				frm->height, frm->width,
-				frm->luma_step, is_intra, next_is_intra);
+				frm->luma_alpha_step, is_intra, next_is_intra);
 	if (encoding & FWHT_FRAME_UNENCODED)
 		encoding |= FWHT_LUMA_UNENCODED;
 	encoding &= ~FWHT_FRAME_UNENCODED;
diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h
index 3e9391fec5fe..743d78e112f8 100644
--- a/drivers/media/platform/vicodec/codec-fwht.h
+++ b/drivers/media/platform/vicodec/codec-fwht.h
@@ -104,9 +104,10 @@ struct fwht_raw_frame {
 	unsigned int width, height;
 	unsigned int width_div;
 	unsigned int height_div;
-	unsigned int luma_step;
+	unsigned int luma_alpha_step;
 	unsigned int chroma_step;
-	u8 *luma, *cb, *cr;
+	unsigned int components_num;
+	u8 *luma, *cb, *cr, *alpha;
 };
 
 #define FWHT_FRAME_PCODED	BIT(0)
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
index e5b68fb38aac..b842636e71a9 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
@@ -11,27 +11,28 @@
 #include "codec-v4l2-fwht.h"
 
 static const struct v4l2_fwht_pixfmt_info v4l2_fwht_pixfmts[] = {
-	{ V4L2_PIX_FMT_YUV420,  1, 3, 2, 1, 1, 2, 2 },
-	{ V4L2_PIX_FMT_YVU420,  1, 3, 2, 1, 1, 2, 2 },
-	{ V4L2_PIX_FMT_YUV422P, 1, 2, 1, 1, 1, 2, 1 },
-	{ V4L2_PIX_FMT_NV12,    1, 3, 2, 1, 2, 2, 2 },
-	{ V4L2_PIX_FMT_NV21,    1, 3, 2, 1, 2, 2, 2 },
-	{ V4L2_PIX_FMT_NV16,    1, 2, 1, 1, 2, 2, 1 },
-	{ V4L2_PIX_FMT_NV61,    1, 2, 1, 1, 2, 2, 1 },
-	{ V4L2_PIX_FMT_NV24,    1, 3, 1, 1, 2, 1, 1 },
-	{ V4L2_PIX_FMT_NV42,    1, 3, 1, 1, 2, 1, 1 },
-	{ V4L2_PIX_FMT_YUYV,    2, 2, 1, 2, 4, 2, 1 },
-	{ V4L2_PIX_FMT_YVYU,    2, 2, 1, 2, 4, 2, 1 },
-	{ V4L2_PIX_FMT_UYVY,    2, 2, 1, 2, 4, 2, 1 },
-	{ V4L2_PIX_FMT_VYUY,    2, 2, 1, 2, 4, 2, 1 },
-	{ V4L2_PIX_FMT_BGR24,   3, 3, 1, 3, 3, 1, 1 },
-	{ V4L2_PIX_FMT_RGB24,   3, 3, 1, 3, 3, 1, 1 },
-	{ V4L2_PIX_FMT_HSV24,   3, 3, 1, 3, 3, 1, 1 },
-	{ V4L2_PIX_FMT_BGR32,   4, 4, 1, 4, 4, 1, 1 },
-	{ V4L2_PIX_FMT_XBGR32,  4, 4, 1, 4, 4, 1, 1 },
-	{ V4L2_PIX_FMT_RGB32,   4, 4, 1, 4, 4, 1, 1 },
-	{ V4L2_PIX_FMT_XRGB32,  4, 4, 1, 4, 4, 1, 1 },
-	{ V4L2_PIX_FMT_HSV32,   4, 4, 1, 4, 4, 1, 1 },
+	{ V4L2_PIX_FMT_YUV420,  1, 3, 2, 1, 1, 2, 2, 3},
+	{ V4L2_PIX_FMT_YVU420,  1, 3, 2, 1, 1, 2, 2, 3},
+	{ V4L2_PIX_FMT_YUV422P, 1, 2, 1, 1, 1, 2, 1, 3},
+	{ V4L2_PIX_FMT_NV12,    1, 3, 2, 1, 2, 2, 2, 3},
+	{ V4L2_PIX_FMT_NV21,    1, 3, 2, 1, 2, 2, 2, 3},
+	{ V4L2_PIX_FMT_NV16,    1, 2, 1, 1, 2, 2, 1, 3},
+	{ V4L2_PIX_FMT_NV61,    1, 2, 1, 1, 2, 2, 1, 3},
+	{ V4L2_PIX_FMT_NV24,    1, 3, 1, 1, 2, 1, 1, 3},
+	{ V4L2_PIX_FMT_NV42,    1, 3, 1, 1, 2, 1, 1, 3},
+	{ V4L2_PIX_FMT_YUYV,    2, 2, 1, 2, 4, 2, 1, 3},
+	{ V4L2_PIX_FMT_YVYU,    2, 2, 1, 2, 4, 2, 1, 3},
+	{ V4L2_PIX_FMT_UYVY,    2, 2, 1, 2, 4, 2, 1, 3},
+	{ V4L2_PIX_FMT_VYUY,    2, 2, 1, 2, 4, 2, 1, 3},
+	{ V4L2_PIX_FMT_BGR24,   3, 3, 1, 3, 3, 1, 1, 3},
+	{ V4L2_PIX_FMT_RGB24,   3, 3, 1, 3, 3, 1, 1, 3},
+	{ V4L2_PIX_FMT_HSV24,   3, 3, 1, 3, 3, 1, 1, 3},
+	{ V4L2_PIX_FMT_BGR32,   4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_XBGR32,  4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_RGB32,   4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_XRGB32,  4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_HSV32,   4, 4, 1, 4, 4, 1, 1, 3},
+
 };
 
 const struct v4l2_fwht_pixfmt_info *v4l2_fwht_find_pixfmt(u32 pixelformat)
@@ -68,8 +69,10 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	rf.luma = p_in;
 	rf.width_div = info->width_div;
 	rf.height_div = info->height_div;
-	rf.luma_step = info->luma_step;
+	rf.luma_alpha_step = info->luma_alpha_step;
 	rf.chroma_step = info->chroma_step;
+	rf.alpha = NULL;
+	rf.components_num = info->components_num;
 
 	switch (info->id) {
 	case V4L2_PIX_FMT_YUV420:
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.h b/drivers/media/platform/vicodec/codec-v4l2-fwht.h
index 162465b78067..ed53e28d4f9c 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.h
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.h
@@ -13,11 +13,12 @@ struct v4l2_fwht_pixfmt_info {
 	unsigned int bytesperline_mult;
 	unsigned int sizeimage_mult;
 	unsigned int sizeimage_div;
-	unsigned int luma_step;
+	unsigned int luma_alpha_step;
 	unsigned int chroma_step;
 	/* Chroma plane subsampling */
 	unsigned int width_div;
 	unsigned int height_div;
+	unsigned int components_num;
 };
 
 struct v4l2_fwht_state {
diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index 1eb9132bfc85..fb6d5e9a06ab 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -61,7 +61,7 @@ struct pixfmt_info {
 };
 
 static const struct v4l2_fwht_pixfmt_info pixfmt_fwht = {
-	V4L2_PIX_FMT_FWHT, 0, 3, 1, 1, 1, 1, 1
+	V4L2_PIX_FMT_FWHT, 0, 3, 1, 1, 1, 1, 1, 0
 };
 
 static void vicodec_dev_release(struct device *dev)
-- 
2.17.1

[PATCH vicodec v4 2/3] media: vicodec: Add support of greyscale format

[Dafna Hirschfeld]

Add support for single plane greyscale format V4L2_PIX_FMT_GREY.
Also change the header of the encoded file to include the number
of components.

Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
---
 drivers/media/platform/vicodec/codec-fwht.c   | 56 +++++++++++--------
 drivers/media/platform/vicodec/codec-fwht.h   |  8 ++-
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 44 ++++++++++++---
 drivers/media/platform/vicodec/vicodec-core.c | 25 +++++++--
 4 files changed, 93 insertions(+), 40 deletions(-)

diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
index 4ee6d7e0fbe2..1af9af84163e 100644
--- a/drivers/media/platform/vicodec/codec-fwht.c
+++ b/drivers/media/platform/vicodec/codec-fwht.c
@@ -753,9 +753,6 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 	__be16 *rlco = cf->rlc_data;
 	__be16 *rlco_max;
 	u32 encoding;
-	u32 chroma_h = frm->height / frm->height_div;
-	u32 chroma_w = frm->width / frm->width_div;
-	unsigned int chroma_size = chroma_h * chroma_w;
 
 	rlco_max = rlco + size / 2 - 256;
 	encoding = encode_plane(frm->luma, ref_frm->luma, &rlco, rlco_max, cf,
@@ -764,20 +761,27 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 	if (encoding & FWHT_FRAME_UNENCODED)
 		encoding |= FWHT_LUMA_UNENCODED;
 	encoding &= ~FWHT_FRAME_UNENCODED;
-	rlco_max = rlco + chroma_size / 2 - 256;
-	encoding |= encode_plane(frm->cb, ref_frm->cb, &rlco, rlco_max, cf,
+
+	if (frm->components_num >= 3) {
+		u32 chroma_h = frm->height / frm->height_div;
+		u32 chroma_w = frm->width / frm->width_div;
+		unsigned int chroma_size = chroma_h * chroma_w;
+
+		rlco_max = rlco + chroma_size / 2 - 256;
+		encoding |= encode_plane(frm->cb, ref_frm->cb, &rlco, rlco_max, cf,
 				 chroma_h, chroma_w,
 				 frm->chroma_step, is_intra, next_is_intra);
-	if (encoding & FWHT_FRAME_UNENCODED)
-		encoding |= FWHT_CB_UNENCODED;
-	encoding &= ~FWHT_FRAME_UNENCODED;
-	rlco_max = rlco + chroma_size / 2 - 256;
-	encoding |= encode_plane(frm->cr, ref_frm->cr, &rlco, rlco_max, cf,
+		if (encoding & FWHT_FRAME_UNENCODED)
+			encoding |= FWHT_CB_UNENCODED;
+		encoding &= ~FWHT_FRAME_UNENCODED;
+		rlco_max = rlco + chroma_size / 2 - 256;
+		encoding |= encode_plane(frm->cr, ref_frm->cr, &rlco, rlco_max, cf,
 				 chroma_h, chroma_w,
 				 frm->chroma_step, is_intra, next_is_intra);
-	if (encoding & FWHT_FRAME_UNENCODED)
-		encoding |= FWHT_CR_UNENCODED;
-	encoding &= ~FWHT_FRAME_UNENCODED;
+		if (encoding & FWHT_FRAME_UNENCODED)
+			encoding |= FWHT_CR_UNENCODED;
+		encoding &= ~FWHT_FRAME_UNENCODED;
+	}
 	cf->size = (rlco - cf->rlc_data) * sizeof(*rlco);
 	return encoding;
 }
@@ -836,20 +840,24 @@ static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
 }
 
 void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
-		       u32 hdr_flags)
+		       u32 hdr_flags, unsigned int components_num)
 {
 	const __be16 *rlco = cf->rlc_data;
-	u32 h = cf->height / 2;
-	u32 w = cf->width / 2;
 
-	if (hdr_flags & FWHT_FL_CHROMA_FULL_HEIGHT)
-		h *= 2;
-	if (hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH)
-		w *= 2;
 	decode_plane(cf, &rlco, ref->luma, cf->height, cf->width,
 		     hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED);
-	decode_plane(cf, &rlco, ref->cb, h, w,
-		     hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED);
-	decode_plane(cf, &rlco, ref->cr, h, w,
-		     hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
+
+	if (components_num >= 3) {
+		u32 h = cf->height;
+		u32 w = cf->width;
+
+		if (!(hdr_flags & FWHT_FL_CHROMA_FULL_HEIGHT))
+			h /= 2;
+		if (!(hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH))
+			w /= 2;
+		decode_plane(cf, &rlco, ref->cb, h, w,
+			hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED);
+		decode_plane(cf, &rlco, ref->cr, h, w,
+			hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
+	}
 }
diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h
index 743d78e112f8..bde11fb93f26 100644
--- a/drivers/media/platform/vicodec/codec-fwht.h
+++ b/drivers/media/platform/vicodec/codec-fwht.h
@@ -56,7 +56,7 @@
 #define FWHT_MAGIC1 0x4f4f4f4f
 #define FWHT_MAGIC2 0xffffffff
 
-#define FWHT_VERSION 1
+#define FWHT_VERSION 2
 
 /* Set if this is an interlaced format */
 #define FWHT_FL_IS_INTERLACED		BIT(0)
@@ -76,6 +76,10 @@
 #define FWHT_FL_CHROMA_FULL_HEIGHT	BIT(7)
 #define FWHT_FL_CHROMA_FULL_WIDTH	BIT(8)
 
+/* A 4-values flag - the number of components - 1 */
+#define FWHT_FL_COMPONENTS_NUM_MSK	GENMASK(17, 16)
+#define FWHT_FL_COMPONENTS_NUM_OFFSET	16
+
 struct fwht_cframe_hdr {
 	u32 magic1;
 	u32 magic2;
@@ -121,6 +125,6 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 		      struct fwht_cframe *cf,
 		      bool is_intra, bool next_is_intra);
 void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
-		       u32 hdr_flags);
+		       u32 hdr_flags, unsigned int components_num);
 
 #endif
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
index b842636e71a9..7dc3918a017e 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
@@ -11,6 +11,7 @@
 #include "codec-v4l2-fwht.h"
 
 static const struct v4l2_fwht_pixfmt_info v4l2_fwht_pixfmts[] = {
+	{ V4L2_PIX_FMT_GREY,    1, 1, 1, 1, 0, 1, 1, 1},
 	{ V4L2_PIX_FMT_YUV420,  1, 3, 2, 1, 1, 2, 2, 3},
 	{ V4L2_PIX_FMT_YVU420,  1, 3, 2, 1, 1, 2, 2, 3},
 	{ V4L2_PIX_FMT_YUV422P, 1, 2, 1, 1, 1, 2, 1, 3},
@@ -75,6 +76,10 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	rf.components_num = info->components_num;
 
 	switch (info->id) {
+	case V4L2_PIX_FMT_GREY:
+		rf.cb = NULL;
+		rf.cr = NULL;
+		break;
 	case V4L2_PIX_FMT_YUV420:
 		rf.cb = rf.luma + size;
 		rf.cr = rf.cb + size / 4;
@@ -165,6 +170,7 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	p_hdr->version = htonl(FWHT_VERSION);
 	p_hdr->width = htonl(cf.width);
 	p_hdr->height = htonl(cf.height);
+	flags |= (info->components_num - 1) << FWHT_FL_COMPONENTS_NUM_OFFSET;
 	if (encoding & FWHT_LUMA_UNENCODED)
 		flags |= FWHT_FL_LUMA_IS_UNCOMPRESSED;
 	if (encoding & FWHT_CB_UNENCODED)
@@ -195,6 +201,8 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	struct fwht_cframe_hdr *p_hdr;
 	struct fwht_cframe cf;
 	u8 *p;
+	unsigned int components_num = 3;
+	unsigned int version;
 
 	if (!state->info)
 		return -EINVAL;
@@ -202,16 +210,16 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	p_hdr = (struct fwht_cframe_hdr *)p_in;
 	cf.width = ntohl(p_hdr->width);
 	cf.height = ntohl(p_hdr->height);
-	flags = ntohl(p_hdr->flags);
-	state->colorspace = ntohl(p_hdr->colorspace);
-	state->xfer_func = ntohl(p_hdr->xfer_func);
-	state->ycbcr_enc = ntohl(p_hdr->ycbcr_enc);
-	state->quantization = ntohl(p_hdr->quantization);
-	cf.rlc_data = (__be16 *)(p_in + sizeof(*p_hdr));
+
+	version = ntohl(p_hdr->version);
+	if (!version || version > FWHT_VERSION) {
+		pr_err("version %d is not supported, current version is %d\n",
+				version, FWHT_VERSION);
+		return -EINVAL;
+	}
 
 	if (p_hdr->magic1 != FWHT_MAGIC1 ||
 	    p_hdr->magic2 != FWHT_MAGIC2 ||
-	    ntohl(p_hdr->version) != FWHT_VERSION ||
 	    (cf.width & 7) || (cf.height & 7))
 		return -EINVAL;
 
@@ -219,14 +227,34 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	if (cf.width != state->width || cf.height != state->height)
 		return -EINVAL;
 
+	flags = ntohl(p_hdr->flags);
+
+	if (version == FWHT_VERSION) {
+		components_num = 1 + ((flags & FWHT_FL_COMPONENTS_NUM_MSK) >>
+			FWHT_FL_COMPONENTS_NUM_OFFSET);
+	}
+
+	state->colorspace = ntohl(p_hdr->colorspace);
+	state->xfer_func = ntohl(p_hdr->xfer_func);
+	state->ycbcr_enc = ntohl(p_hdr->ycbcr_enc);
+	state->quantization = ntohl(p_hdr->quantization);
+	cf.rlc_data = (__be16 *)(p_in + sizeof(*p_hdr));
+
 	if (!(flags & FWHT_FL_CHROMA_FULL_WIDTH))
 		chroma_size /= 2;
 	if (!(flags & FWHT_FL_CHROMA_FULL_HEIGHT))
 		chroma_size /= 2;
 
-	fwht_decode_frame(&cf, &state->ref_frame, flags);
+	fwht_decode_frame(&cf, &state->ref_frame, flags, components_num);
 
+	/*
+	 * TODO - handle the case where the compressed stream encodes a different
+	 * format than the requested decoded format.
+	 */
 	switch (state->info->id) {
+	case V4L2_PIX_FMT_GREY:
+		memcpy(p_out, state->ref_frame.luma, size);
+		break;
 	case V4L2_PIX_FMT_YUV420:
 	case V4L2_PIX_FMT_YUV422P:
 		memcpy(p_out, state->ref_frame.luma, size);
diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index fb6d5e9a06ab..92bc68694a21 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -993,6 +993,16 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 	unsigned int size = q_data->width * q_data->height;
 	const struct v4l2_fwht_pixfmt_info *info = q_data->info;
 	unsigned int chroma_div = info->width_div * info->height_div;
+	unsigned int total_planes_size;
+
+	/*
+	 * we don't know ahead how many components are in the encoding type
+	 * V4L2_PIX_FMT_FWHT, so we will allocate space for 3 planes.
+	 */
+	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num >= 3)
+		total_planes_size = size + 2 * (size / chroma_div);
+	else
+		total_planes_size = size;
 
 	q_data->sequence = 0;
 
@@ -1002,10 +1012,8 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 	state->width = q_data->width;
 	state->height = q_data->height;
 	state->ref_frame.width = state->ref_frame.height = 0;
-	state->ref_frame.luma = kvmalloc(size + 2 * size / chroma_div,
-					 GFP_KERNEL);
-	ctx->comp_max_size = size + 2 * size / chroma_div +
-			     sizeof(struct fwht_cframe_hdr);
+	state->ref_frame.luma = kvmalloc(total_planes_size, GFP_KERNEL);
+	ctx->comp_max_size = total_planes_size + sizeof(struct fwht_cframe_hdr);
 	state->compressed_frame = kvmalloc(ctx->comp_max_size, GFP_KERNEL);
 	if (!state->ref_frame.luma || !state->compressed_frame) {
 		kvfree(state->ref_frame.luma);
@@ -1013,8 +1021,13 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 		vicodec_return_bufs(q, VB2_BUF_STATE_QUEUED);
 		return -ENOMEM;
 	}
-	state->ref_frame.cb = state->ref_frame.luma + size;
-	state->ref_frame.cr = state->ref_frame.cb + size / chroma_div;
+	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num >= 3) {
+		state->ref_frame.cb = state->ref_frame.luma + size;
+		state->ref_frame.cr = state->ref_frame.cb + size / chroma_div;
+	} else {
+		state->ref_frame.cb = NULL;
+		state->ref_frame.cr = NULL;
+	}
 	ctx->last_src_buf = NULL;
 	ctx->last_dst_buf = NULL;
 	state->gop_cnt = 0;
-- 
2.17.1

[PATCH vicodec v4 3/3] media: vicodec: Add support for 4 planes formats

[Dafna Hirschfeld]

Add support for formats with 4 planes: V4L2_PIX_FMT_ABGR32,
V4L2_PIX_FMT_ARGB32.
Also add alpha plane related flags to the header of the encoded file.

Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
---
 drivers/media/platform/vicodec/codec-fwht.c   | 15 +++++++++
 drivers/media/platform/vicodec/codec-fwht.h   |  2 ++
 .../media/platform/vicodec/codec-v4l2-fwht.c  | 32 +++++++++++++++++++
 drivers/media/platform/vicodec/vicodec-core.c | 12 +++++--
 4 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
index 1af9af84163e..9513374e8f44 100644
--- a/drivers/media/platform/vicodec/codec-fwht.c
+++ b/drivers/media/platform/vicodec/codec-fwht.c
@@ -782,6 +782,17 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 			encoding |= FWHT_CR_UNENCODED;
 		encoding &= ~FWHT_FRAME_UNENCODED;
 	}
+
+	if (frm->components_num == 4) {
+		rlco_max = rlco + size / 2 - 256;
+		encoding = encode_plane(frm->alpha, ref_frm->alpha, &rlco, rlco_max, cf,
+				frm->height, frm->width,
+				frm->luma_alpha_step, is_intra, next_is_intra);
+		if (encoding & FWHT_FRAME_UNENCODED)
+			encoding |= FWHT_ALPHA_UNENCODED;
+		encoding &= ~FWHT_FRAME_UNENCODED;
+	}
+
 	cf->size = (rlco - cf->rlc_data) * sizeof(*rlco);
 	return encoding;
 }
@@ -860,4 +871,8 @@ void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
 		decode_plane(cf, &rlco, ref->cr, h, w,
 			hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
 	}
+
+	if (components_num == 4)
+		decode_plane(cf, &rlco, ref->alpha, cf->height, cf->width,
+		     hdr_flags & FWHT_FL_ALPHA_IS_UNCOMPRESSED);
 }
diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h
index bde11fb93f26..90ff8962fca7 100644
--- a/drivers/media/platform/vicodec/codec-fwht.h
+++ b/drivers/media/platform/vicodec/codec-fwht.h
@@ -75,6 +75,7 @@
 #define FWHT_FL_CR_IS_UNCOMPRESSED	BIT(6)
 #define FWHT_FL_CHROMA_FULL_HEIGHT	BIT(7)
 #define FWHT_FL_CHROMA_FULL_WIDTH	BIT(8)
+#define FWHT_FL_ALPHA_IS_UNCOMPRESSED	BIT(9)
 
 /* A 4-values flag - the number of components - 1 */
 #define FWHT_FL_COMPONENTS_NUM_MSK	GENMASK(17, 16)
@@ -119,6 +120,7 @@ struct fwht_raw_frame {
 #define FWHT_LUMA_UNENCODED	BIT(2)
 #define FWHT_CB_UNENCODED	BIT(3)
 #define FWHT_CR_UNENCODED	BIT(4)
+#define FWHT_ALPHA_UNENCODED	BIT(5)
 
 u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 		      struct fwht_raw_frame *ref_frm,
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
index 7dc3918a017e..b53655a8cef6 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
@@ -33,6 +33,8 @@ static const struct v4l2_fwht_pixfmt_info v4l2_fwht_pixfmts[] = {
 	{ V4L2_PIX_FMT_RGB32,   4, 4, 1, 4, 4, 1, 1, 3},
 	{ V4L2_PIX_FMT_XRGB32,  4, 4, 1, 4, 4, 1, 1, 3},
 	{ V4L2_PIX_FMT_HSV32,   4, 4, 1, 4, 4, 1, 1, 3},
+	{ V4L2_PIX_FMT_ARGB32,  4, 4, 1, 4, 4, 1, 1, 4},
+	{ V4L2_PIX_FMT_ABGR32,  4, 4, 1, 4, 4, 1, 1, 4},
 
 };
 
@@ -146,6 +148,18 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 		rf.cr = rf.cb + 2;
 		rf.luma++;
 		break;
+	case V4L2_PIX_FMT_ARGB32:
+		rf.alpha = rf.luma;
+		rf.cr = rf.luma + 1;
+		rf.cb = rf.cr + 2;
+		rf.luma += 2;
+		break;
+	case V4L2_PIX_FMT_ABGR32:
+		rf.cb = rf.luma;
+		rf.cr = rf.cb + 2;
+		rf.luma++;
+		rf.alpha = rf.cr + 1;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -177,6 +191,8 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 		flags |= FWHT_FL_CB_IS_UNCOMPRESSED;
 	if (encoding & FWHT_CR_UNENCODED)
 		flags |= FWHT_FL_CR_IS_UNCOMPRESSED;
+	if (encoding & FWHT_ALPHA_UNENCODED)
+		flags |= FWHT_FL_ALPHA_IS_UNCOMPRESSED;
 	if (rf.height_div == 1)
 		flags |= FWHT_FL_CHROMA_FULL_HEIGHT;
 	if (rf.width_div == 1)
@@ -356,6 +372,22 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 			*p++ = 0;
 		}
 		break;
+	case V4L2_PIX_FMT_ARGB32:
+		for (i = 0, p = p_out; i < size; i++) {
+			*p++ = state->ref_frame.alpha[i];
+			*p++ = state->ref_frame.cr[i];
+			*p++ = state->ref_frame.luma[i];
+			*p++ = state->ref_frame.cb[i];
+		}
+		break;
+	case V4L2_PIX_FMT_ABGR32:
+		for (i = 0, p = p_out; i < size; i++) {
+			*p++ = state->ref_frame.cb[i];
+			*p++ = state->ref_frame.luma[i];
+			*p++ = state->ref_frame.cr[i];
+			*p++ = state->ref_frame.alpha[i];
+		}
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index 92bc68694a21..5ae876238e13 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -997,9 +997,11 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 
 	/*
 	 * we don't know ahead how many components are in the encoding type
-	 * V4L2_PIX_FMT_FWHT, so we will allocate space for 3 planes.
+	 * V4L2_PIX_FMT_FWHT, so we will allocate space for 4 planes.
 	 */
-	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num >= 3)
+	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num == 4)
+		total_planes_size = 2 * size + 2 * (size / chroma_div);
+	else if (info->components_num == 3)
 		total_planes_size = size + 2 * (size / chroma_div);
 	else
 		total_planes_size = size;
@@ -1028,6 +1030,12 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 		state->ref_frame.cb = NULL;
 		state->ref_frame.cr = NULL;
 	}
+
+	if (info->id == V4L2_PIX_FMT_FWHT || info->components_num == 4)
+		state->ref_frame.alpha = state->ref_frame.cr + size / chroma_div;
+	else
+		state->ref_frame.alpha = NULL;
+
 	ctx->last_src_buf = NULL;
 	ctx->last_dst_buf = NULL;
 	state->gop_cnt = 0;
-- 
2.17.1

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Hans Verkuil]

On 11/15/18 12:14, Dafna Hirschfeld wrote:
> 
> 
> On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
> 
>     Hi Dafna,
> 
>     This patch series looks very nice!
> 
> thanks:) 
> 
>     Can you repost this and add a CC to linux-media?
> 
> I will
> 
>     I'm happy to take this.
> 
>     The TODO that is in the decoder code (what to do if the fwht bitstream
>     encodes fewer/more planes than the requested raw frame format) is
>     something that needs to be implemented later as part of making vicodec
>     compliant to the stateful codec specification.
> 
> I tried to look in the docs what should be done in that case and didn't find.
> The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
> but this is only optional.

You are referring to section 4.6.6, point 4 here:

https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html

Right?

It's optional only in the sense that if you already happen to know what
the stream contains, then you don't need to do it.

I'm CC-ing Tomasz Figa, since I think just describing a step as 'Optional'
is too vague. Something like: 'If <condition>, then you need to perform
this step' (or alternatively: 'If !<condition>, then you can skip this step'.

Regards,

	Hans

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Tomasz Figa]

Hi Hans, Dafna,

On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/15/18 12:14, Dafna Hirschfeld wrote:
> >
> >
> > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
> >
> >     Hi Dafna,
> >
> >     This patch series looks very nice!
> >
> > thanks:)
> >
> >     Can you repost this and add a CC to linux-media?
> >
> > I will
> >
> >     I'm happy to take this.
> >
> >     The TODO that is in the decoder code (what to do if the fwht bitstream
> >     encodes fewer/more planes than the requested raw frame format) is
> >     something that needs to be implemented later as part of making vicodec
> >     compliant to the stateful codec specification.
> >
> > I tried to look in the docs what should be done in that case and didn't find.
> > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
> > but this is only optional.
>
> You are referring to section 4.6.6, point 4 here:
>
> https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
>
> Right?
>
> It's optional only in the sense that if you already happen to know what
> the stream contains, then you don't need to do it.
>
> I'm CC-ing Tomasz Figa, since I think just describing a step as 'Optional'
> is too vague. Something like: 'If <condition>, then you need to perform
> this step' (or alternatively: 'If !<condition>, then you can skip this step'.

Just to clarify, the documentation is written from userspace point of
view. If something is optional for a userspace application, the
application can use it and the driver is expected to support it.

I'm okay replacing "Optional" with something more specific, like "If
the client does not need to change the default format", but I wonder
if it wouldn't effectively become "if the client does not need to
change the default format, or it wants to do it by probing a fixed
list of formats it supports by using S_FMT or TRY_FMT, or what not,
...".

Perhaps just explaining the meaning of "Optional" explicitly in the
"Conventions and notation used in this document" would be good enough?

Best regards,
Tomasz

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Dafna Hirschfeld]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org> wrote:

> Hi Hans, Dafna,
>
> On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > On 11/15/18 12:14, Dafna Hirschfeld wrote:
> > >
> > >
> > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl
> <mailto:hverkuil@xs4all.nl>> wrote:
> > >
> > >     Hi Dafna,
> > >
> > >     This patch series looks very nice!
> > >
> > > thanks:)
> > >
> > >     Can you repost this and add a CC to linux-media?
> > >
> > > I will
> > >
> > >     I'm happy to take this.
> > >
> > >     The TODO that is in the decoder code (what to do if the fwht
> bitstream
> > >     encodes fewer/more planes than the requested raw frame format) is
> > >     something that needs to be implemented later as part of making
> vicodec
> > >     compliant to the stateful codec specification.
> > >
> > > I tried to look in the docs what should be done in that case and
> didn't find.
> > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the
> supported formats based on the OUTPUT buffers
> > > but this is only optional.
> >
> > You are referring to section 4.6.6, point 4 here:
> >
> > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
> >
> > Right?
> >
>
Right,


> > It's optional only in the sense that if you already happen to know what
> > the stream contains, then you don't need to do it.
> >

It still not clear to me what should be the behavior in case the user does
give the CAPTURE format before the streaming starts
but then the actual encoded data that the user send is of different format.
Can we trust the user to be consistent ?

Dafna


> > I'm CC-ing Tomasz Figa, since I think just describing a step as
> 'Optional'
> > is too vague. Something like: 'If <condition>, then you need to perform
> > this step' (or alternatively: 'If !<condition>, then you can skip this
> step'.
>
> Just to clarify, the documentation is written from userspace point of
> view. If something is optional for a userspace application, the
> application can use it and the driver is expected to support it.
>
> I'm okay replacing "Optional" with something more specific, like "If
> the client does not need to change the default format", but I wonder
> if it wouldn't effectively become "if the client does not need to
> change the default format, or it wants to do it by probing a fixed
> list of formats it supports by using S_FMT or TRY_FMT, or what not,
> ...".
>
> Perhaps just explaining the meaning of "Optional" explicitly in the
> "Conventions and notation used in this document" would be good enough?
>
> Best regards,
> Tomasz
>

[-- Attachment #2: Type: text/html, Size: 4009 bytes --]

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Tomasz Figa]

On Thu, Nov 15, 2018 at 10:05 PM Dafna Hirschfeld <dafna3@gmail.com> wrote:
>
>
>
> On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>
>> Hi Hans, Dafna,
>>
>> On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >
>> > On 11/15/18 12:14, Dafna Hirschfeld wrote:
>> > >
>> > >
>> > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
>> > >
>> > >     Hi Dafna,
>> > >
>> > >     This patch series looks very nice!
>> > >
>> > > thanks:)
>> > >
>> > >     Can you repost this and add a CC to linux-media?
>> > >
>> > > I will
>> > >
>> > >     I'm happy to take this.
>> > >
>> > >     The TODO that is in the decoder code (what to do if the fwht bitstream
>> > >     encodes fewer/more planes than the requested raw frame format) is
>> > >     something that needs to be implemented later as part of making vicodec
>> > >     compliant to the stateful codec specification.
>> > >
>> > > I tried to look in the docs what should be done in that case and didn't find.
>> > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
>> > > but this is only optional.
>> >
>> > You are referring to section 4.6.6, point 4 here:
>> >
>> > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
>> >
>> > Right?
>> >
>
> Right,
>
>>
>> > It's optional only in the sense that if you already happen to know what
>> > the stream contains, then you don't need to do it.
>> >
>
> It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
> but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

This is a very good point. Generally the first step of the "4.6.6.
Capture setup" section is to query the format and it's mandatory for
the client. So when the driver determines the supported formats
(possibly after parsing the bitstream headers), it will reset the
CAPTURE format to whatever suitable default and signal a source change
event. If the client ignores this and doesn't check the real format,
it's buggy and correct operation shouldn't be expected.

Best regards,
Tomasz

>
> Dafna
>
>>
>> > I'm CC-ing Tomasz Figa, since I think just describing a step as 'Optional'
>> > is too vague. Something like: 'If <condition>, then you need to perform
>> > this step' (or alternatively: 'If !<condition>, then you can skip this step'.
>>
>> Just to clarify, the documentation is written from userspace point of
>> view. If something is optional for a userspace application, the
>> application can use it and the driver is expected to support it.
>>
>> I'm okay replacing "Optional" with something more specific, like "If
>> the client does not need to change the default format", but I wonder
>> if it wouldn't effectively become "if the client does not need to
>> change the default format, or it wants to do it by probing a fixed
>> list of formats it supports by using S_FMT or TRY_FMT, or what not,
>> ...".
>>
>> Perhaps just explaining the meaning of "Optional" explicitly in the
>> "Conventions and notation used in this document" would be good enough?
>>
>> Best regards,
>> Tomasz

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Hans Verkuil]

On 11/15/18 14:04, Dafna Hirschfeld wrote:
> 
> 
> On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org <mailto:tfiga@chromium.org>> wrote:
> 
>     Hi Hans, Dafna,
> 
>     On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
>     >
>     > On 11/15/18 12:14, Dafna Hirschfeld wrote:
>     > >
>     > >
>     > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl> <mailto:hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>>> wrote:
>     > >
>     > >     Hi Dafna,
>     > >
>     > >     This patch series looks very nice!
>     > >
>     > > thanks:)
>     > >
>     > >     Can you repost this and add a CC to linux-media?
>     > >
>     > > I will
>     > >
>     > >     I'm happy to take this.
>     > >
>     > >     The TODO that is in the decoder code (what to do if the fwht bitstream
>     > >     encodes fewer/more planes than the requested raw frame format) is
>     > >     something that needs to be implemented later as part of making vicodec
>     > >     compliant to the stateful codec specification.
>     > >
>     > > I tried to look in the docs what should be done in that case and didn't find.
>     > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
>     > > but this is only optional.
>     >
>     > You are referring to section 4.6.6, point 4 here:
>     >
>     > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
>     >
>     > Right?
>     >
> 
> Right,
>  
> 
>     > It's optional only in the sense that if you already happen to know what
>     > the stream contains, then you don't need to do it.
>     > 
> 
> It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
> but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?

No, we can't trust users. At all :-)

So the driver would have to act the same as if there was a resolution change.

Tomasz, this is actually something to improve: it is not just resolution changes,
but also incompatible capture formats. In this case, the stream contains RGB, but
the capture format is YUV, and vicodec cannot convert from RGB to YUV.

I think sending a SOURCE_CHANGE event makes sense. One thing though: this would
take place at the very start of the decoding process, so step 2 in section 4.6.9
(Dynamic resolution change) says "The decoder will then process and decode all
remaining buffers from before the resolution change point.". But no buffers have
been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?

It is probably the right thing to do.

Regards,

	Hans

> 
> Dafna
>  
> 
>     > I'm CC-ing Tomasz Figa, since I think just describing a step as 'Optional'
>     > is too vague. Something like: 'If <condition>, then you need to perform
>     > this step' (or alternatively: 'If !<condition>, then you can skip this step'.
> 
>     Just to clarify, the documentation is written from userspace point of
>     view. If something is optional for a userspace application, the
>     application can use it and the driver is expected to support it.
> 
>     I'm okay replacing "Optional" with something more specific, like "If
>     the client does not need to change the default format", but I wonder
>     if it wouldn't effectively become "if the client does not need to
>     change the default format, or it wants to do it by probing a fixed
>     list of formats it supports by using S_FMT or TRY_FMT, or what not,
>     ...".
> 
>     Perhaps just explaining the meaning of "Optional" explicitly in the
>     "Conventions and notation used in this document" would be good enough?
> 
>     Best regards,
>     Tomasz
> 

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Tomasz Figa]

On Thu, Nov 15, 2018 at 10:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/15/18 14:04, Dafna Hirschfeld wrote:
> >
> >
> > On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org <mailto:tfiga@chromium.org>> wrote:
> >
> >     Hi Hans, Dafna,
> >
> >     On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
> >     >
> >     > On 11/15/18 12:14, Dafna Hirschfeld wrote:
> >     > >
> >     > >
> >     > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl> <mailto:hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>>> wrote:
> >     > >
> >     > >     Hi Dafna,
> >     > >
> >     > >     This patch series looks very nice!
> >     > >
> >     > > thanks:)
> >     > >
> >     > >     Can you repost this and add a CC to linux-media?
> >     > >
> >     > > I will
> >     > >
> >     > >     I'm happy to take this.
> >     > >
> >     > >     The TODO that is in the decoder code (what to do if the fwht bitstream
> >     > >     encodes fewer/more planes than the requested raw frame format) is
> >     > >     something that needs to be implemented later as part of making vicodec
> >     > >     compliant to the stateful codec specification.
> >     > >
> >     > > I tried to look in the docs what should be done in that case and didn't find.
> >     > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
> >     > > but this is only optional.
> >     >
> >     > You are referring to section 4.6.6, point 4 here:
> >     >
> >     > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
> >     >
> >     > Right?
> >     >
> >
> > Right,
> >
> >
> >     > It's optional only in the sense that if you already happen to know what
> >     > the stream contains, then you don't need to do it.
> >     >
> >
> > It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
> > but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?
>
> No, we can't trust users. At all :-)
>
> So the driver would have to act the same as if there was a resolution change.
>
> Tomasz, this is actually something to improve: it is not just resolution changes,
> but also incompatible capture formats. In this case, the stream contains RGB, but
> the capture format is YUV, and vicodec cannot convert from RGB to YUV.
>
> I think sending a SOURCE_CHANGE event makes sense. One thing though: this would
> take place at the very start of the decoding process, so step 2 in section 4.6.9
> (Dynamic resolution change) says "The decoder will then process and decode all
> remaining buffers from before the resolution change point.". But no buffers have
> been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?
>
> It is probably the right thing to do.

There is already a source change event specified to be signaled in the
Initialization sequence. It's defined to always happen after the
driver establishes the format. Given that, is there any change still
needed?

Best regards,
Tomasz

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Hans Verkuil]

On 11/15/18 14:33, Tomasz Figa wrote:
> On Thu, Nov 15, 2018 at 10:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/15/18 14:04, Dafna Hirschfeld wrote:
>>>
>>>
>>> On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org <mailto:tfiga@chromium.org>> wrote:
>>>
>>>     Hi Hans, Dafna,
>>>
>>>     On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
>>>     >
>>>     > On 11/15/18 12:14, Dafna Hirschfeld wrote:
>>>     > >
>>>     > >
>>>     > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl> <mailto:hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>>> wrote:
>>>     > >
>>>     > >     Hi Dafna,
>>>     > >
>>>     > >     This patch series looks very nice!
>>>     > >
>>>     > > thanks:)
>>>     > >
>>>     > >     Can you repost this and add a CC to linux-media?
>>>     > >
>>>     > > I will
>>>     > >
>>>     > >     I'm happy to take this.
>>>     > >
>>>     > >     The TODO that is in the decoder code (what to do if the fwht bitstream
>>>     > >     encodes fewer/more planes than the requested raw frame format) is
>>>     > >     something that needs to be implemented later as part of making vicodec
>>>     > >     compliant to the stateful codec specification.
>>>     > >
>>>     > > I tried to look in the docs what should be done in that case and didn't find.
>>>     > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
>>>     > > but this is only optional.
>>>     >
>>>     > You are referring to section 4.6.6, point 4 here:
>>>     >
>>>     > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
>>>     >
>>>     > Right?
>>>     >
>>>
>>> Right,
>>>
>>>
>>>     > It's optional only in the sense that if you already happen to know what
>>>     > the stream contains, then you don't need to do it.
>>>     >
>>>
>>> It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
>>> but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?
>>
>> No, we can't trust users. At all :-)
>>
>> So the driver would have to act the same as if there was a resolution change.
>>
>> Tomasz, this is actually something to improve: it is not just resolution changes,
>> but also incompatible capture formats. In this case, the stream contains RGB, but
>> the capture format is YUV, and vicodec cannot convert from RGB to YUV.
>>
>> I think sending a SOURCE_CHANGE event makes sense. One thing though: this would
>> take place at the very start of the decoding process, so step 2 in section 4.6.9
>> (Dynamic resolution change) says "The decoder will then process and decode all
>> remaining buffers from before the resolution change point.". But no buffers have
>> been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?
>>
>> It is probably the right thing to do.
> 
> There is already a source change event specified to be signaled in the
> Initialization sequence. It's defined to always happen after the
> driver establishes the format. Given that, is there any change still
> needed?

Ah, you're right. It has to be documented that this happens as part of the
pixel format documentation, right? So this should be added to the FWHT format
doc once the proper behavior is implemented.

Regards.

	Hans

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Tomasz Figa]

On Thu, Nov 15, 2018 at 10:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/15/18 14:33, Tomasz Figa wrote:
> > On Thu, Nov 15, 2018 at 10:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 11/15/18 14:04, Dafna Hirschfeld wrote:
> >>>
> >>>
> >>> On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org <mailto:tfiga@chromium.org>> wrote:
> >>>
> >>>     Hi Hans, Dafna,
> >>>
> >>>     On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
> >>>     >
> >>>     > On 11/15/18 12:14, Dafna Hirschfeld wrote:
> >>>     > >
> >>>     > >
> >>>     > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl> <mailto:hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>>> wrote:
> >>>     > >
> >>>     > >     Hi Dafna,
> >>>     > >
> >>>     > >     This patch series looks very nice!
> >>>     > >
> >>>     > > thanks:)
> >>>     > >
> >>>     > >     Can you repost this and add a CC to linux-media?
> >>>     > >
> >>>     > > I will
> >>>     > >
> >>>     > >     I'm happy to take this.
> >>>     > >
> >>>     > >     The TODO that is in the decoder code (what to do if the fwht bitstream
> >>>     > >     encodes fewer/more planes than the requested raw frame format) is
> >>>     > >     something that needs to be implemented later as part of making vicodec
> >>>     > >     compliant to the stateful codec specification.
> >>>     > >
> >>>     > > I tried to look in the docs what should be done in that case and didn't find.
> >>>     > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
> >>>     > > but this is only optional.
> >>>     >
> >>>     > You are referring to section 4.6.6, point 4 here:
> >>>     >
> >>>     > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
> >>>     >
> >>>     > Right?
> >>>     >
> >>>
> >>> Right,
> >>>
> >>>
> >>>     > It's optional only in the sense that if you already happen to know what
> >>>     > the stream contains, then you don't need to do it.
> >>>     >
> >>>
> >>> It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
> >>> but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?
> >>
> >> No, we can't trust users. At all :-)
> >>
> >> So the driver would have to act the same as if there was a resolution change.
> >>
> >> Tomasz, this is actually something to improve: it is not just resolution changes,
> >> but also incompatible capture formats. In this case, the stream contains RGB, but
> >> the capture format is YUV, and vicodec cannot convert from RGB to YUV.
> >>
> >> I think sending a SOURCE_CHANGE event makes sense. One thing though: this would
> >> take place at the very start of the decoding process, so step 2 in section 4.6.9
> >> (Dynamic resolution change) says "The decoder will then process and decode all
> >> remaining buffers from before the resolution change point.". But no buffers have
> >> been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?
> >>
> >> It is probably the right thing to do.
> >
> > There is already a source change event specified to be signaled in the
> > Initialization sequence. It's defined to always happen after the
> > driver establishes the format. Given that, is there any change still
> > needed?
>
> Ah, you're right. It has to be documented that this happens as part of the
> pixel format documentation, right? So this should be added to the FWHT format
> doc once the proper behavior is implemented.

I wonder if it wouldn't be more consistent if we just made the driver
send this event even for streams that don't include format data in the
headers. In that case the driver would already have the information
about the format from the OUTPUT queue (and internal constraints), so
it could just signal the event instantly. WDYT?

Best regards,
Tomasz

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Hans Verkuil]

On 11/15/18 14:43, Tomasz Figa wrote:
> On Thu, Nov 15, 2018 at 10:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/15/18 14:33, Tomasz Figa wrote:
>>> On Thu, Nov 15, 2018 at 10:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 11/15/18 14:04, Dafna Hirschfeld wrote:
>>>>>
>>>>>
>>>>> On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org <mailto:tfiga@chromium.org>> wrote:
>>>>>
>>>>>     Hi Hans, Dafna,
>>>>>
>>>>>     On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
>>>>>     >
>>>>>     > On 11/15/18 12:14, Dafna Hirschfeld wrote:
>>>>>     > >
>>>>>     > >
>>>>>     > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl> <mailto:hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>>> wrote:
>>>>>     > >
>>>>>     > >     Hi Dafna,
>>>>>     > >
>>>>>     > >     This patch series looks very nice!
>>>>>     > >
>>>>>     > > thanks:)
>>>>>     > >
>>>>>     > >     Can you repost this and add a CC to linux-media?
>>>>>     > >
>>>>>     > > I will
>>>>>     > >
>>>>>     > >     I'm happy to take this.
>>>>>     > >
>>>>>     > >     The TODO that is in the decoder code (what to do if the fwht bitstream
>>>>>     > >     encodes fewer/more planes than the requested raw frame format) is
>>>>>     > >     something that needs to be implemented later as part of making vicodec
>>>>>     > >     compliant to the stateful codec specification.
>>>>>     > >
>>>>>     > > I tried to look in the docs what should be done in that case and didn't find.
>>>>>     > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
>>>>>     > > but this is only optional.
>>>>>     >
>>>>>     > You are referring to section 4.6.6, point 4 here:
>>>>>     >
>>>>>     > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
>>>>>     >
>>>>>     > Right?
>>>>>     >
>>>>>
>>>>> Right,
>>>>>
>>>>>
>>>>>     > It's optional only in the sense that if you already happen to know what
>>>>>     > the stream contains, then you don't need to do it.
>>>>>     >
>>>>>
>>>>> It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
>>>>> but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?
>>>>
>>>> No, we can't trust users. At all :-)
>>>>
>>>> So the driver would have to act the same as if there was a resolution change.
>>>>
>>>> Tomasz, this is actually something to improve: it is not just resolution changes,
>>>> but also incompatible capture formats. In this case, the stream contains RGB, but
>>>> the capture format is YUV, and vicodec cannot convert from RGB to YUV.
>>>>
>>>> I think sending a SOURCE_CHANGE event makes sense. One thing though: this would
>>>> take place at the very start of the decoding process, so step 2 in section 4.6.9
>>>> (Dynamic resolution change) says "The decoder will then process and decode all
>>>> remaining buffers from before the resolution change point.". But no buffers have
>>>> been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?
>>>>
>>>> It is probably the right thing to do.
>>>
>>> There is already a source change event specified to be signaled in the
>>> Initialization sequence. It's defined to always happen after the
>>> driver establishes the format. Given that, is there any change still
>>> needed?
>>
>> Ah, you're right. It has to be documented that this happens as part of the
>> pixel format documentation, right? So this should be added to the FWHT format
>> doc once the proper behavior is implemented.
> 
> I wonder if it wouldn't be more consistent if we just made the driver
> send this event even for streams that don't include format data in the
> headers. In that case the driver would already have the information
> about the format from the OUTPUT queue (and internal constraints), so
> it could just signal the event instantly. WDYT?

Can't we leave this open? Are there codecs where the stream doesn't have
meta data? I'm not aware of any.

I'm not sure if we need to mention codecs without embedded meta data at all.

Perhaps just mention that such codecs are unsupported at the moment.

Regards,

	Hans

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Tomasz Figa]

On Thu, Nov 15, 2018 at 10:48 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/15/18 14:43, Tomasz Figa wrote:
> > On Thu, Nov 15, 2018 at 10:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 11/15/18 14:33, Tomasz Figa wrote:
> >>> On Thu, Nov 15, 2018 at 10:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>
> >>>> On 11/15/18 14:04, Dafna Hirschfeld wrote:
> >>>>>
> >>>>>
> >>>>> On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org <mailto:tfiga@chromium.org>> wrote:
> >>>>>
> >>>>>     Hi Hans, Dafna,
> >>>>>
> >>>>>     On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
> >>>>>     >
> >>>>>     > On 11/15/18 12:14, Dafna Hirschfeld wrote:
> >>>>>     > >
> >>>>>     > >
> >>>>>     > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl> <mailto:hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>>> wrote:
> >>>>>     > >
> >>>>>     > >     Hi Dafna,
> >>>>>     > >
> >>>>>     > >     This patch series looks very nice!
> >>>>>     > >
> >>>>>     > > thanks:)
> >>>>>     > >
> >>>>>     > >     Can you repost this and add a CC to linux-media?
> >>>>>     > >
> >>>>>     > > I will
> >>>>>     > >
> >>>>>     > >     I'm happy to take this.
> >>>>>     > >
> >>>>>     > >     The TODO that is in the decoder code (what to do if the fwht bitstream
> >>>>>     > >     encodes fewer/more planes than the requested raw frame format) is
> >>>>>     > >     something that needs to be implemented later as part of making vicodec
> >>>>>     > >     compliant to the stateful codec specification.
> >>>>>     > >
> >>>>>     > > I tried to look in the docs what should be done in that case and didn't find.
> >>>>>     > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
> >>>>>     > > but this is only optional.
> >>>>>     >
> >>>>>     > You are referring to section 4.6.6, point 4 here:
> >>>>>     >
> >>>>>     > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
> >>>>>     >
> >>>>>     > Right?
> >>>>>     >
> >>>>>
> >>>>> Right,
> >>>>>
> >>>>>
> >>>>>     > It's optional only in the sense that if you already happen to know what
> >>>>>     > the stream contains, then you don't need to do it.
> >>>>>     >
> >>>>>
> >>>>> It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
> >>>>> but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?
> >>>>
> >>>> No, we can't trust users. At all :-)
> >>>>
> >>>> So the driver would have to act the same as if there was a resolution change.
> >>>>
> >>>> Tomasz, this is actually something to improve: it is not just resolution changes,
> >>>> but also incompatible capture formats. In this case, the stream contains RGB, but
> >>>> the capture format is YUV, and vicodec cannot convert from RGB to YUV.
> >>>>
> >>>> I think sending a SOURCE_CHANGE event makes sense. One thing though: this would
> >>>> take place at the very start of the decoding process, so step 2 in section 4.6.9
> >>>> (Dynamic resolution change) says "The decoder will then process and decode all
> >>>> remaining buffers from before the resolution change point.". But no buffers have
> >>>> been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?
> >>>>
> >>>> It is probably the right thing to do.
> >>>
> >>> There is already a source change event specified to be signaled in the
> >>> Initialization sequence. It's defined to always happen after the
> >>> driver establishes the format. Given that, is there any change still
> >>> needed?
> >>
> >> Ah, you're right. It has to be documented that this happens as part of the
> >> pixel format documentation, right? So this should be added to the FWHT format
> >> doc once the proper behavior is implemented.
> >
> > I wonder if it wouldn't be more consistent if we just made the driver
> > send this event even for streams that don't include format data in the
> > headers. In that case the driver would already have the information
> > about the format from the OUTPUT queue (and internal constraints), so
> > it could just signal the event instantly. WDYT?
>
> Can't we leave this open? Are there codecs where the stream doesn't have
> meta data? I'm not aware of any.
>
> I'm not sure if we need to mention codecs without embedded meta data at all.
>
> Perhaps just mention that such codecs are unsupported at the moment.

That would indeed simplify things. I don't know too much about codecs
other than H.264 and VPx, so can't really tell if none of the codecs
we have FourCCs for would qualify as such, though...

Best regards,
Tomasz

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Hans Verkuil]

On 11/15/2018 02:50 PM, Tomasz Figa wrote:
> On Thu, Nov 15, 2018 at 10:48 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/15/18 14:43, Tomasz Figa wrote:
>>> On Thu, Nov 15, 2018 at 10:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 11/15/18 14:33, Tomasz Figa wrote:
>>>>> On Thu, Nov 15, 2018 at 10:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>
>>>>>> On 11/15/18 14:04, Dafna Hirschfeld wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org <mailto:tfiga@chromium.org>> wrote:
>>>>>>>
>>>>>>>     Hi Hans, Dafna,
>>>>>>>
>>>>>>>     On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
>>>>>>>     >
>>>>>>>     > On 11/15/18 12:14, Dafna Hirschfeld wrote:
>>>>>>>     > >
>>>>>>>     > >
>>>>>>>     > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl> <mailto:hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>>> wrote:
>>>>>>>     > >
>>>>>>>     > >     Hi Dafna,
>>>>>>>     > >
>>>>>>>     > >     This patch series looks very nice!
>>>>>>>     > >
>>>>>>>     > > thanks:)
>>>>>>>     > >
>>>>>>>     > >     Can you repost this and add a CC to linux-media?
>>>>>>>     > >
>>>>>>>     > > I will
>>>>>>>     > >
>>>>>>>     > >     I'm happy to take this.
>>>>>>>     > >
>>>>>>>     > >     The TODO that is in the decoder code (what to do if the fwht bitstream
>>>>>>>     > >     encodes fewer/more planes than the requested raw frame format) is
>>>>>>>     > >     something that needs to be implemented later as part of making vicodec
>>>>>>>     > >     compliant to the stateful codec specification.
>>>>>>>     > >
>>>>>>>     > > I tried to look in the docs what should be done in that case and didn't find.
>>>>>>>     > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
>>>>>>>     > > but this is only optional.
>>>>>>>     >
>>>>>>>     > You are referring to section 4.6.6, point 4 here:
>>>>>>>     >
>>>>>>>     > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
>>>>>>>     >
>>>>>>>     > Right?
>>>>>>>     >
>>>>>>>
>>>>>>> Right,
>>>>>>>
>>>>>>>
>>>>>>>     > It's optional only in the sense that if you already happen to know what
>>>>>>>     > the stream contains, then you don't need to do it.
>>>>>>>     >
>>>>>>>
>>>>>>> It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
>>>>>>> but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?
>>>>>>
>>>>>> No, we can't trust users. At all :-)
>>>>>>
>>>>>> So the driver would have to act the same as if there was a resolution change.
>>>>>>
>>>>>> Tomasz, this is actually something to improve: it is not just resolution changes,
>>>>>> but also incompatible capture formats. In this case, the stream contains RGB, but
>>>>>> the capture format is YUV, and vicodec cannot convert from RGB to YUV.
>>>>>>
>>>>>> I think sending a SOURCE_CHANGE event makes sense. One thing though: this would
>>>>>> take place at the very start of the decoding process, so step 2 in section 4.6.9
>>>>>> (Dynamic resolution change) says "The decoder will then process and decode all
>>>>>> remaining buffers from before the resolution change point.". But no buffers have
>>>>>> been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?
>>>>>>
>>>>>> It is probably the right thing to do.
>>>>>
>>>>> There is already a source change event specified to be signaled in the
>>>>> Initialization sequence. It's defined to always happen after the
>>>>> driver establishes the format. Given that, is there any change still
>>>>> needed?
>>>>
>>>> Ah, you're right. It has to be documented that this happens as part of the
>>>> pixel format documentation, right? So this should be added to the FWHT format
>>>> doc once the proper behavior is implemented.
>>>
>>> I wonder if it wouldn't be more consistent if we just made the driver
>>> send this event even for streams that don't include format data in the
>>> headers. In that case the driver would already have the information
>>> about the format from the OUTPUT queue (and internal constraints), so
>>> it could just signal the event instantly. WDYT?
>>
>> Can't we leave this open? Are there codecs where the stream doesn't have
>> meta data? I'm not aware of any.
>>
>> I'm not sure if we need to mention codecs without embedded meta data at all.
>>
>> Perhaps just mention that such codecs are unsupported at the moment.
> 
> That would indeed simplify things. I don't know too much about codecs
> other than H.264 and VPx, so can't really tell if none of the codecs
> we have FourCCs for would qualify as such, though...

As far as I am aware, all of them do have metadata in the bitstream. So
I'd just leave it out from the spec. Or perhaps leave a single mention
that such codecs are not supported at the moment.

Regards,

	Hans

Re: [PATCH vicodec v4 0/3] Add support to more pixel formats in vicodec

[Tomasz Figa]

On Thu, Nov 15, 2018 at 11:22 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/15/2018 02:50 PM, Tomasz Figa wrote:
> > On Thu, Nov 15, 2018 at 10:48 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 11/15/18 14:43, Tomasz Figa wrote:
> >>> On Thu, Nov 15, 2018 at 10:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>
> >>>> On 11/15/18 14:33, Tomasz Figa wrote:
> >>>>> On Thu, Nov 15, 2018 at 10:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>>
> >>>>>> On 11/15/18 14:04, Dafna Hirschfeld wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On Thu, Nov 15, 2018 at 2:09 PM Tomasz Figa <tfiga@chromium.org <mailto:tfiga@chromium.org>> wrote:
> >>>>>>>
> >>>>>>>     Hi Hans, Dafna,
> >>>>>>>
> >>>>>>>     On Thu, Nov 15, 2018 at 8:58 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
> >>>>>>>     >
> >>>>>>>     > On 11/15/18 12:14, Dafna Hirschfeld wrote:
> >>>>>>>     > >
> >>>>>>>     > >
> >>>>>>>     > > On Tue, Nov 13, 2018 at 1:48 PM Hans Verkuil <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl> <mailto:hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>>> wrote:
> >>>>>>>     > >
> >>>>>>>     > >     Hi Dafna,
> >>>>>>>     > >
> >>>>>>>     > >     This patch series looks very nice!
> >>>>>>>     > >
> >>>>>>>     > > thanks:)
> >>>>>>>     > >
> >>>>>>>     > >     Can you repost this and add a CC to linux-media?
> >>>>>>>     > >
> >>>>>>>     > > I will
> >>>>>>>     > >
> >>>>>>>     > >     I'm happy to take this.
> >>>>>>>     > >
> >>>>>>>     > >     The TODO that is in the decoder code (what to do if the fwht bitstream
> >>>>>>>     > >     encodes fewer/more planes than the requested raw frame format) is
> >>>>>>>     > >     something that needs to be implemented later as part of making vicodec
> >>>>>>>     > >     compliant to the stateful codec specification.
> >>>>>>>     > >
> >>>>>>>     > > I tried to look in the docs what should be done in that case and didn't find.
> >>>>>>>     > > The user can send VIDIOC_ENUM_FMT ioctl on CAPTURE to enumerate the supported formats based on the OUTPUT buffers
> >>>>>>>     > > but this is only optional.
> >>>>>>>     >
> >>>>>>>     > You are referring to section 4.6.6, point 4 here:
> >>>>>>>     >
> >>>>>>>     > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-decoder.html
> >>>>>>>     >
> >>>>>>>     > Right?
> >>>>>>>     >
> >>>>>>>
> >>>>>>> Right,
> >>>>>>>
> >>>>>>>
> >>>>>>>     > It's optional only in the sense that if you already happen to know what
> >>>>>>>     > the stream contains, then you don't need to do it.
> >>>>>>>     >
> >>>>>>>
> >>>>>>> It still not clear to me what should be the behavior in case the user does give the CAPTURE format before the streaming starts
> >>>>>>> but then the actual encoded data that the user send is of different format. Can we trust the user to be consistent ?
> >>>>>>
> >>>>>> No, we can't trust users. At all :-)
> >>>>>>
> >>>>>> So the driver would have to act the same as if there was a resolution change.
> >>>>>>
> >>>>>> Tomasz, this is actually something to improve: it is not just resolution changes,
> >>>>>> but also incompatible capture formats. In this case, the stream contains RGB, but
> >>>>>> the capture format is YUV, and vicodec cannot convert from RGB to YUV.
> >>>>>>
> >>>>>> I think sending a SOURCE_CHANGE event makes sense. One thing though: this would
> >>>>>> take place at the very start of the decoding process, so step 2 in section 4.6.9
> >>>>>> (Dynamic resolution change) says "The decoder will then process and decode all
> >>>>>> remaining buffers from before the resolution change point.". But no buffers have
> >>>>>> been decoded yet, so should it queue an empty buffer with V4L2_BUF_FLAG_LAST set?
> >>>>>>
> >>>>>> It is probably the right thing to do.
> >>>>>
> >>>>> There is already a source change event specified to be signaled in the
> >>>>> Initialization sequence. It's defined to always happen after the
> >>>>> driver establishes the format. Given that, is there any change still
> >>>>> needed?
> >>>>
> >>>> Ah, you're right. It has to be documented that this happens as part of the
> >>>> pixel format documentation, right? So this should be added to the FWHT format
> >>>> doc once the proper behavior is implemented.
> >>>
> >>> I wonder if it wouldn't be more consistent if we just made the driver
> >>> send this event even for streams that don't include format data in the
> >>> headers. In that case the driver would already have the information
> >>> about the format from the OUTPUT queue (and internal constraints), so
> >>> it could just signal the event instantly. WDYT?
> >>
> >> Can't we leave this open? Are there codecs where the stream doesn't have
> >> meta data? I'm not aware of any.
> >>
> >> I'm not sure if we need to mention codecs without embedded meta data at all.
> >>
> >> Perhaps just mention that such codecs are unsupported at the moment.
> >
> > That would indeed simplify things. I don't know too much about codecs
> > other than H.264 and VPx, so can't really tell if none of the codecs
> > we have FourCCs for would qualify as such, though...
>
> As far as I am aware, all of them do have metadata in the bitstream. So
> I'd just leave it out from the spec. Or perhaps leave a single mention
> that such codecs are not supported at the moment.

Sounds good to me. Would you be able to post a comment like this in a
reply to my patch? Thanks in advance.

Best regards,
Tomasz