diff mbox series

[FFmpeg-devel] libaomenc: enable 8, 10 and 12 bit RGB encoding

Message ID MCMDxFK--3-2@lynne.ee
State Accepted
Commit 6a2f3f60ae02c8c3c62645b1d54ecc86bb21080d
Headers show
Series [FFmpeg-devel] libaomenc: enable 8, 10 and 12 bit RGB encoding
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lynne July 16, 2020, 10:46 a.m. UTC
RGB pixel formats are one occasion where by pixel format we mean 
pixel format, primaries, transfer characteristic, and matrix coeffs,
so we have to manually set them as they're set to unspecified by
default, despite there only being a single possible combination.

Patch attached.
Subject: [PATCH] libaomenc: enable 8, 10 and 12 bit RGB encoding

RGB pixel formats are one occasion where by pixel format we mean
pixel format, primaries, transfer characteristic, and matrix coeffs,
so we have to manually set them as they're set to unspecified by
default, despite there only being a single possible combination.
---
 libavcodec/libaomenc.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Comments

James Almer July 17, 2020, 1:12 a.m. UTC | #1
On 7/16/2020 7:46 AM, Lynne wrote:
> RGB pixel formats are one occasion where by pixel format we mean 
> pixel format, primaries, transfer characteristic, and matrix coeffs,
> so we have to manually set them as they're set to unspecified by
> default, despite there only being a single possible combination.
> 
> Patch attached.

[...]

> From 83652d61b7da6fea486b1533fa39d23411f7cde9 Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Thu, 16 Jul 2020 11:39:05 +0100
> Subject: [PATCH] libaomenc: enable 8, 10 and 12 bit RGB encoding
> 
> RGB pixel formats are one occasion where by pixel format we mean
> pixel format, primaries, transfer characteristic, and matrix coeffs,
> so we have to manually set them as they're set to unspecified by
> default, despite there only being a single possible combination.
> ---
>  libavcodec/libaomenc.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> index 2ecb3de3a7..0d6a376ef0 100644
> --- a/libavcodec/libaomenc.c
> +++ b/libavcodec/libaomenc.c
> @@ -310,6 +310,7 @@ static int set_pix_fmt(AVCodecContext *avctx, aom_codec_caps_t codec_caps,
>          *img_fmt = AOM_IMG_FMT_I422;
>          return 0;
>      case AV_PIX_FMT_YUV444P:
> +    case AV_PIX_FMT_GBRP:
>          enccfg->g_profile = FF_PROFILE_AV1_HIGH;
>          *img_fmt = AOM_IMG_FMT_I444;
>          return 0;
> @@ -338,9 +339,13 @@ static int set_pix_fmt(AVCodecContext *avctx, aom_codec_caps_t codec_caps,
>          break;
>      case AV_PIX_FMT_YUV444P10:
>      case AV_PIX_FMT_YUV444P12:
> +    case AV_PIX_FMT_GBRP10:
> +    case AV_PIX_FMT_GBRP12:
>          if (codec_caps & AOM_CODEC_CAP_HIGHBITDEPTH) {
> -            enccfg->g_bit_depth = enccfg->g_input_bit_depth =
> -                avctx->pix_fmt == AV_PIX_FMT_YUV444P10 ? 10 : 12;
> +            enccfg->g_bit_depth = enccfg->g_input_bit_depth = 10;
> +            if (avctx->pix_fmt == AV_PIX_FMT_YUV444P12 ||
> +                avctx->pix_fmt == AV_PIX_FMT_GBRP12)
> +                enccfg->g_bit_depth = enccfg->g_input_bit_depth = 12;
>              enccfg->g_profile =
>                  enccfg->g_bit_depth == 10 ? FF_PROFILE_AV1_HIGH : FF_PROFILE_AV1_PROFESSIONAL;
>              *img_fmt = AOM_IMG_FMT_I44416;
> @@ -749,9 +754,16 @@ static av_cold int aom_init(AVCodecContext *avctx,
>      if (ctx->tune >= 0)
>          codecctl_int(avctx, AOME_SET_TUNING, ctx->tune);
>  
> -    codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, avctx->color_primaries);
> -    codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, avctx->colorspace);
> -    codecctl_int(avctx, AV1E_SET_TRANSFER_CHARACTERISTICS, avctx->color_trc);
> +    if (avctx->pix_fmt == AV_PIX_FMT_GBRP || avctx->pix_fmt == AV_PIX_FMT_GBRP10 ||
> +        avctx->pix_fmt == AV_PIX_FMT_GBRP12) {

It may be cleaner here and above if you instead use
av_pix_fmt_desc_get() in the function then look at desc->comp[0]->depth
for bitdepth and and desc->flags & AV_PIX_FMT_FLAG_RGB for RGB, instead
of all these if checks for a dozen pixfmts.

Should be ok either way.

> +        codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, AVCOL_PRI_BT709);
> +        codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, AVCOL_SPC_RGB);
> +        codecctl_int(avctx, AV1E_SET_TRANSFER_CHARACTERISTICS, AVCOL_TRC_IEC61966_2_1);
> +    } else {
> +        codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, avctx->color_primaries);
> +        codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, avctx->colorspace);
> +        codecctl_int(avctx, AV1E_SET_TRANSFER_CHARACTERISTICS, avctx->color_trc);
> +    }
>      if (ctx->aq_mode >= 0)
>          codecctl_int(avctx, AV1E_SET_AQ_MODE, ctx->aq_mode);
>      if (ctx->frame_parallel >= 0)
> @@ -1077,6 +1089,7 @@ static const enum AVPixelFormat av1_pix_fmts[] = {
>      AV_PIX_FMT_YUV420P,
>      AV_PIX_FMT_YUV422P,
>      AV_PIX_FMT_YUV444P,
> +    AV_PIX_FMT_GBRP,
>      AV_PIX_FMT_NONE
>  };
>  
> @@ -1084,12 +1097,15 @@ static const enum AVPixelFormat av1_pix_fmts_highbd[] = {
>      AV_PIX_FMT_YUV420P,
>      AV_PIX_FMT_YUV422P,
>      AV_PIX_FMT_YUV444P,
> +    AV_PIX_FMT_GBRP,
>      AV_PIX_FMT_YUV420P10,
>      AV_PIX_FMT_YUV422P10,
>      AV_PIX_FMT_YUV444P10,
>      AV_PIX_FMT_YUV420P12,
>      AV_PIX_FMT_YUV422P12,
>      AV_PIX_FMT_YUV444P12,
> +    AV_PIX_FMT_GBRP10,
> +    AV_PIX_FMT_GBRP12,
>      AV_PIX_FMT_NONE
>  };
>  
> -- 
> 2.28.0.rc0
>
Lynne July 17, 2020, 8:19 p.m. UTC | #2
Jul 17, 2020, 02:12 by jamrial@gmail.com:

> On 7/16/2020 7:46 AM, Lynne wrote: 
>
>> -    codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, avctx->color_primaries);
>> -    codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, avctx->colorspace);
>> -    codecctl_int(avctx, AV1E_SET_TRANSFER_CHARACTERISTICS, avctx->color_trc);
>> +    if (avctx->pix_fmt == AV_PIX_FMT_GBRP || avctx->pix_fmt == AV_PIX_FMT_GBRP10 ||
>> +        avctx->pix_fmt == AV_PIX_FMT_GBRP12) {
>>
>
> It may be cleaner here and above if you instead use
> av_pix_fmt_desc_get() in the function then look at desc->comp[0]->depth
> for bitdepth and and desc->flags & AV_PIX_FMT_FLAG_RGB for RGB, instead
> of all these if checks for a dozen pixfmts.
>

I tried that but it didn't look cleaner. The pixdesc API is unfortunately necessarily cumbersome,
since 565 RGB formats exist, so we can't rely on all components having the same bit depth
to have a separate av_pix_fmt_get_depth(pixfmt) function.



> Should be ok either way.
>

Thanks for the review, pushed.
Might write a similar patch for the librav1e wrapper which has a similar problem.
diff mbox series

Patch

diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
index 2ecb3de3a7..0d6a376ef0 100644
--- a/libavcodec/libaomenc.c
+++ b/libavcodec/libaomenc.c
@@ -310,6 +310,7 @@  static int set_pix_fmt(AVCodecContext *avctx, aom_codec_caps_t codec_caps,
         *img_fmt = AOM_IMG_FMT_I422;
         return 0;
     case AV_PIX_FMT_YUV444P:
+    case AV_PIX_FMT_GBRP:
         enccfg->g_profile = FF_PROFILE_AV1_HIGH;
         *img_fmt = AOM_IMG_FMT_I444;
         return 0;
@@ -338,9 +339,13 @@  static int set_pix_fmt(AVCodecContext *avctx, aom_codec_caps_t codec_caps,
         break;
     case AV_PIX_FMT_YUV444P10:
     case AV_PIX_FMT_YUV444P12:
+    case AV_PIX_FMT_GBRP10:
+    case AV_PIX_FMT_GBRP12:
         if (codec_caps & AOM_CODEC_CAP_HIGHBITDEPTH) {
-            enccfg->g_bit_depth = enccfg->g_input_bit_depth =
-                avctx->pix_fmt == AV_PIX_FMT_YUV444P10 ? 10 : 12;
+            enccfg->g_bit_depth = enccfg->g_input_bit_depth = 10;
+            if (avctx->pix_fmt == AV_PIX_FMT_YUV444P12 ||
+                avctx->pix_fmt == AV_PIX_FMT_GBRP12)
+                enccfg->g_bit_depth = enccfg->g_input_bit_depth = 12;
             enccfg->g_profile =
                 enccfg->g_bit_depth == 10 ? FF_PROFILE_AV1_HIGH : FF_PROFILE_AV1_PROFESSIONAL;
             *img_fmt = AOM_IMG_FMT_I44416;
@@ -749,9 +754,16 @@  static av_cold int aom_init(AVCodecContext *avctx,
     if (ctx->tune >= 0)
         codecctl_int(avctx, AOME_SET_TUNING, ctx->tune);
 
-    codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, avctx->color_primaries);
-    codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, avctx->colorspace);
-    codecctl_int(avctx, AV1E_SET_TRANSFER_CHARACTERISTICS, avctx->color_trc);
+    if (avctx->pix_fmt == AV_PIX_FMT_GBRP || avctx->pix_fmt == AV_PIX_FMT_GBRP10 ||
+        avctx->pix_fmt == AV_PIX_FMT_GBRP12) {
+        codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, AVCOL_PRI_BT709);
+        codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, AVCOL_SPC_RGB);
+        codecctl_int(avctx, AV1E_SET_TRANSFER_CHARACTERISTICS, AVCOL_TRC_IEC61966_2_1);
+    } else {
+        codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, avctx->color_primaries);
+        codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, avctx->colorspace);
+        codecctl_int(avctx, AV1E_SET_TRANSFER_CHARACTERISTICS, avctx->color_trc);
+    }
     if (ctx->aq_mode >= 0)
         codecctl_int(avctx, AV1E_SET_AQ_MODE, ctx->aq_mode);
     if (ctx->frame_parallel >= 0)
@@ -1077,6 +1089,7 @@  static const enum AVPixelFormat av1_pix_fmts[] = {
     AV_PIX_FMT_YUV420P,
     AV_PIX_FMT_YUV422P,
     AV_PIX_FMT_YUV444P,
+    AV_PIX_FMT_GBRP,
     AV_PIX_FMT_NONE
 };
 
@@ -1084,12 +1097,15 @@  static const enum AVPixelFormat av1_pix_fmts_highbd[] = {
     AV_PIX_FMT_YUV420P,
     AV_PIX_FMT_YUV422P,
     AV_PIX_FMT_YUV444P,
+    AV_PIX_FMT_GBRP,
     AV_PIX_FMT_YUV420P10,
     AV_PIX_FMT_YUV422P10,
     AV_PIX_FMT_YUV444P10,
     AV_PIX_FMT_YUV420P12,
     AV_PIX_FMT_YUV422P12,
     AV_PIX_FMT_YUV444P12,
+    AV_PIX_FMT_GBRP10,
+    AV_PIX_FMT_GBRP12,
     AV_PIX_FMT_NONE
 };