[PATCH 0/2] Clarify H.264 loop filter offset controls and fix them for coda

[PATCH 0/2] Clarify H.264 loop filter offset controls and fix them for coda

[Philipp Zabel]

Hi,

the coda driver handles the H.264 loop filter alpha/beta offset controls
incorrectly. When trying to fix them, I noticed that the documentation
is not clear about what these values actually are.

>From the value range of -6 to +6 used in the existing drivers (s5p-mfc,
venus), it looks like they currently correspond directly to the values
stored into the slice headers: slice_alpha_c0_offset_div2 and
slice_beta_offset_div2. These are only half of the actual alpha/beta
filter offsets.

The ITU-T Rec. H.264 (02/2016) states:

  slice_alpha_c0_offset_div2 specifies the offset used in accessing the
  α [...] deblocking filter tables for filtering operations controlled
  by the macroblocks within the slice. From this value, the offset that
  shall be applied when addressing these tables shall be computed as

      FilterOffsetA = slice_alpha_c0_offset_div2 << 1             (7-32)

  The value of slice_alpha_c0_offset_div2 shall be in the range of −6 to
  +6, inclusive. When slice_alpha_c0_offset_div2 is not present in the
  slice header, the value of slice_alpha_c0_offset_div2 shall be inferred
  to be equal to 0.

And the same for slice_beta_offset_div2 / FilterOffsetB.

Do the s5p-mfc and venus drivers use the controls
V4L2_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA and _BETA directly as slice
header fields, and thus their values are to be interpreted as half of
FilterOffsetA/B defined in the H.264 spec, respectively?

regards
Philipp

Philipp Zabel (2):
  media: v4l2: clarify H.264 loop filter offset controls
  media: coda: fix H.264 deblocking filter controls

 .../media/uapi/v4l/extended-controls.rst      |  6 ++++++
 drivers/media/platform/coda/coda-bit.c        | 19 +++++++++----------
 drivers/media/platform/coda/coda-common.c     | 15 +++++++--------
 drivers/media/platform/coda/coda.h            |  6 +++---
 drivers/media/platform/coda/coda_regs.h       |  2 +-
 5 files changed, 26 insertions(+), 22 deletions(-)

-- 
2.19.1

[PATCH 1/2] media: v4l2: clarify H.264 loop filter offset controls

[Philipp Zabel]

The venus and s5p-mfc drivers add the loop filter alpha/beta offset
controls V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA/BETA with a range of
-6 to +6, inclusive. This is exactly the range specified for the slice
header fields slice_alpha_c0_offset_div2 and slice_beta_offset_div2,
which store half the actual filter offsets FilterOffsetA/B.

Clarify that this control contains the halved offsets.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
I assume that the venus and s5p-mfc drivers use the loop filter control
values directly as halved filter offsets, because of the ranges. If this
is not the case, the documentation should be changed to clarify that the
control values correspond to FilterOffsetA/B directly, instead.
---
 Documentation/media/uapi/v4l/extended-controls.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
index 65a1d873196b..8dff21391c1f 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -1110,10 +1110,16 @@ enum v4l2_mpeg_video_h264_loop_filter_mode -
 
 ``V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA (integer)``
     Loop filter alpha coefficient, defined in the H264 standard.
+    This value corresponds to the slice_alpha_c0_offset_div2 slice header
+    field, and should be in the range of -6 to +6, inclusive. The actual alpha
+    offset FilterOffsetA is twice this value.
     Applicable to the H264 encoder.
 
 ``V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_BETA (integer)``
     Loop filter beta coefficient, defined in the H264 standard.
+    This corresponds to the slice_beta_offset_div2 slice header field, and
+    should be in the range of -6 to +6, inclusive. The actual beta offset
+    FilterOffsetB is twice this value.
     Applicable to the H264 encoder.
 
 .. _v4l2-mpeg-video-h264-entropy-mode:
-- 
2.19.1

[PATCH 2/2] media: coda: fix H.264 deblocking filter controls

[Philipp Zabel]

Add support for the third loop filter mode
V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_DISABLED_AT_SLICE_BOUNDARY,
and fix V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA and
V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_BETA controls.

The filter offset controls are signed values in the -6 to 6 range and
are stored into the slice header fields slice_alpha_c0_offset_div2 and
slice_beta_offset_div2. The actual filter offsets FilterOffsetA/B are
double their value, in range of -12 to 12.

Rename variables to more closely match the nomenclature in the H.264
specification.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
This is under the assumption that loop filter alpha/beta controls should
store the slice header values in the -6 to 6 range. If they should store
the actual filter offset, the controls should be changed to the range
of -12 to 12, step 2, and the values halved before passing them to the
firmware.
---
 drivers/media/platform/coda/coda-bit.c    | 19 +++++++++----------
 drivers/media/platform/coda/coda-common.c | 15 +++++++--------
 drivers/media/platform/coda/coda.h        |  6 +++---
 drivers/media/platform/coda/coda_regs.h   |  2 +-
 4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index f2c0aa261c9b..8e0194993a52 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -1002,16 +1002,15 @@ static int coda_start_encoding(struct coda_ctx *ctx)
 		else
 			coda_write(dev, CODA_STD_H264,
 				   CODA_CMD_ENC_SEQ_COD_STD);
-		if (ctx->params.h264_deblk_enabled) {
-			value = ((ctx->params.h264_deblk_alpha &
-				  CODA_264PARAM_DEBLKFILTEROFFSETALPHA_MASK) <<
-				 CODA_264PARAM_DEBLKFILTEROFFSETALPHA_OFFSET) |
-				((ctx->params.h264_deblk_beta &
-				  CODA_264PARAM_DEBLKFILTEROFFSETBETA_MASK) <<
-				 CODA_264PARAM_DEBLKFILTEROFFSETBETA_OFFSET);
-		} else {
-			value = 1 << CODA_264PARAM_DISABLEDEBLK_OFFSET;
-		}
+		value = ((ctx->params.h264_disable_deblocking_filter_idc &
+			  CODA_264PARAM_DISABLEDEBLK_MASK) <<
+			 CODA_264PARAM_DISABLEDEBLK_OFFSET) |
+			((ctx->params.h264_slice_alpha_c0_offset_div2 &
+			  CODA_264PARAM_DEBLKFILTEROFFSETALPHA_MASK) <<
+			 CODA_264PARAM_DEBLKFILTEROFFSETALPHA_OFFSET) |
+			((ctx->params.h264_slice_beta_offset_div2 &
+			  CODA_264PARAM_DEBLKFILTEROFFSETBETA_MASK) <<
+			 CODA_264PARAM_DEBLKFILTEROFFSETBETA_OFFSET);
 		coda_write(dev, value, CODA_CMD_ENC_SEQ_264_PARA);
 		break;
 	case V4L2_PIX_FMT_JPEG:
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index a62c47843d2f..7518f01c48f7 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1831,14 +1831,13 @@ static int coda_s_ctrl(struct v4l2_ctrl *ctrl)
 		ctx->params.h264_max_qp = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA:
-		ctx->params.h264_deblk_alpha = ctrl->val;
+		ctx->params.h264_slice_alpha_c0_offset_div2 = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_BETA:
-		ctx->params.h264_deblk_beta = ctrl->val;
+		ctx->params.h264_slice_beta_offset_div2 = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_MODE:
-		ctx->params.h264_deblk_enabled = (ctrl->val ==
-				V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_ENABLED);
+		ctx->params.h264_disable_deblocking_filter_idc = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
 		/* TODO: switch between baseline and constrained baseline */
@@ -1919,13 +1918,13 @@ static void coda_encode_ctrls(struct coda_ctx *ctx)
 	v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
 		V4L2_CID_MPEG_VIDEO_H264_MAX_QP, 0, 51, 1, 51);
 	v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
-		V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA, 0, 15, 1, 0);
+		V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA, -6, 6, 1, 0);
 	v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
-		V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_BETA, 0, 15, 1, 0);
+		V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_BETA, -6, 6, 1, 0);
 	v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
 		V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_MODE,
-		V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_DISABLED, 0x0,
-		V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_ENABLED);
+		V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_DISABLED_AT_SLICE_BOUNDARY,
+		0x0, V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_ENABLED);
 	v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
 		V4L2_CID_MPEG_VIDEO_H264_PROFILE,
 		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index 22f6efaf9510..31cea72f5b2a 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -115,9 +115,9 @@ struct coda_params {
 	u8			h264_inter_qp;
 	u8			h264_min_qp;
 	u8			h264_max_qp;
-	u8			h264_deblk_enabled;
-	u8			h264_deblk_alpha;
-	u8			h264_deblk_beta;
+	u8			h264_disable_deblocking_filter_idc;
+	s8			h264_slice_alpha_c0_offset_div2;
+	s8			h264_slice_beta_offset_div2;
 	u8			h264_profile_idc;
 	u8			h264_level_idc;
 	u8			mpeg4_intra_qp;
diff --git a/drivers/media/platform/coda/coda_regs.h b/drivers/media/platform/coda/coda_regs.h
index 5e7b00a97671..e675e38f3475 100644
--- a/drivers/media/platform/coda/coda_regs.h
+++ b/drivers/media/platform/coda/coda_regs.h
@@ -292,7 +292,7 @@
 #define		CODA_264PARAM_DEBLKFILTEROFFSETALPHA_OFFSET	8
 #define		CODA_264PARAM_DEBLKFILTEROFFSETALPHA_MASK	0x0f
 #define		CODA_264PARAM_DISABLEDEBLK_OFFSET		6
-#define		CODA_264PARAM_DISABLEDEBLK_MASK		0x01
+#define		CODA_264PARAM_DISABLEDEBLK_MASK		0x03
 #define		CODA_264PARAM_CONSTRAINEDINTRAPREDFLAG_OFFSET	5
 #define		CODA_264PARAM_CONSTRAINEDINTRAPREDFLAG_MASK	0x01
 #define		CODA_264PARAM_CHROMAQPOFFSET_OFFSET		0
-- 
2.19.1

Re: [PATCH 0/2] Clarify H.264 loop filter offset controls and fix them for coda

[Stanimir Varbanov]

Hi Philipp,

On 11/28/18 3:01 PM, Philipp Zabel wrote:
> Hi,
> 
> the coda driver handles the H.264 loop filter alpha/beta offset controls
> incorrectly. When trying to fix them, I noticed that the documentation
> is not clear about what these values actually are.
> 
> From the value range of -6 to +6 used in the existing drivers (s5p-mfc,
> venus), it looks like they currently correspond directly to the values
> stored into the slice headers: slice_alpha_c0_offset_div2 and
> slice_beta_offset_div2. These are only half of the actual alpha/beta
> filter offsets.
> 
> The ITU-T Rec. H.264 (02/2016) states:
> 
>   slice_alpha_c0_offset_div2 specifies the offset used in accessing the
>   α [...] deblocking filter tables for filtering operations controlled
>   by the macroblocks within the slice. From this value, the offset that
>   shall be applied when addressing these tables shall be computed as
> 
>       FilterOffsetA = slice_alpha_c0_offset_div2 << 1             (7-32)
> 
>   The value of slice_alpha_c0_offset_div2 shall be in the range of −6 to
>   +6, inclusive. When slice_alpha_c0_offset_div2 is not present in the
>   slice header, the value of slice_alpha_c0_offset_div2 shall be inferred
>   to be equal to 0.
> 
> And the same for slice_beta_offset_div2 / FilterOffsetB.
> 
> Do the s5p-mfc and venus drivers use the controls
> V4L2_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA and _BETA directly as slice
> header fields, and thus their values are to be interpreted as half of
> FilterOffsetA/B defined in the H.264 spec, respectively?

That is correct for Venus encoder, it uses slice header fields directly
[-6, 6].

-- 
regards,
Stan