[FFmpeg-devel] [PATCH v2 0/2] PNG cICP improvements

[FFmpeg-devel] [PATCH v2 0/2] PNG cICP improvements

[Leo Izen]

The first patch fixes a FATE issue with cICP and iCCP.

The second patch was discussed on IRC. I got a LGTM from Kasper, but sending to ML here and applying in a few days if there's no objections.

A few things discussed:
1) There's no need to expose the gamma table as another function as it's only used here.
2) This patch includes the patch on the ML sent as "avcodec/pngdec: warn when reading tv-range cICP chunks" which got a LGTM from Lynne on IRC. If this is applied that one has no need to be applied.

Changes from v1:
- The second patch broke FATE because I didn't change the fate ref to accommodate the new metadata. I've since updated the fate reference.

Leo Izen (2):
  avcodec/pngenc.c: avoid writing cICP when inappropriate
  avcodec/pngdec: read colorspace info when decoding with AVDISCARD_ALL

 libavcodec/pngdec.c          | 126 +++++++++++++++++++++--------------
 libavcodec/pngenc.c          |  21 ++++--
 tests/ref/fate/png-icc       |   8 +--
 tests/ref/fate/png-side-data |   2 +-
 4 files changed, 94 insertions(+), 63 deletions(-)

-- 
2.39.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v2 1/2] avcodec/pngenc.c: avoid writing cICP when inappropriate

[Leo Izen]

We parse the fallback cHRM on decode and correctly determine that we
have BT.709 primaries, but unknown TRC. This causes us to write cICP
where we shouldn't. Primaries without transfer can be handled entirely
by cHRM, so we should only write cICP if we actually know the transfer
function.

Additionally, we should avoid writing cICP if there's an ICC profile
because the spec says decoders must prioritize cICP over the ICC
profile.

Signed-off-by: Leo Izen <leo.izen@gmail.com>
---
 libavcodec/pngenc.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
index 2393161c3b..81b95c143d 100644
--- a/libavcodec/pngenc.c
+++ b/libavcodec/pngenc.c
@@ -412,14 +412,25 @@ static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
         }
     }
 
+    side_data = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
+    if ((ret = png_write_iccp(s, side_data)))
+        return ret;
+
     /* write colorspace information */
     if (pict->color_primaries == AVCOL_PRI_BT709 &&
         pict->color_trc == AVCOL_TRC_IEC61966_2_1) {
         s->buf[0] = 1; /* rendering intent, relative colorimetric by default */
         png_write_chunk(&s->bytestream, MKTAG('s', 'R', 'G', 'B'), s->buf, 1);
-    } else if (pict->color_primaries != AVCOL_PRI_UNSPECIFIED ||
-        pict->color_trc != AVCOL_TRC_UNSPECIFIED) {
-        /* these values match H.273 so no translation is needed */
+    } else if (pict->color_trc != AVCOL_TRC_UNSPECIFIED && !side_data) {
+        /*
+         * Avoid writing cICP if the transfer is unknown. Known primaries
+         * with unknown transfer can be handled by cHRM.
+         *
+         * We also avoid writing cICP if an ICC Profile is present, because
+         * the standard requires that cICP overrides iCCP.
+         *
+         * These values match H.273 so no translation is needed.
+         */
         s->buf[0] = pict->color_primaries;
         s->buf[1] = pict->color_trc;
         s->buf[2] = 0; /* colorspace = RGB */
@@ -432,10 +443,6 @@ static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
     if (png_get_gama(pict->color_trc, s->buf))
         png_write_chunk(&s->bytestream, MKTAG('g', 'A', 'M', 'A'), s->buf, 4);
 
-    side_data = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
-    if ((ret = png_write_iccp(s, side_data)))
-        return ret;
-
     /* put the palette if needed, must be after colorspace information */
     if (s->color_type == PNG_COLOR_TYPE_PALETTE) {
         int has_alpha, alpha, i;
-- 
2.39.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v2 2/2] avcodec/pngdec: read colorspace info when decoding with AVDISCARD_ALL

[Leo Izen]

These chunks are lightweight and it's useful information to have when
running ffmpeg -i or ffprobe, for example.

Signed-off-by: Leo Izen <leo.izen@gmail.com>
---
 libavcodec/pngdec.c          | 126 +++++++++++++++++++++--------------
 tests/ref/fate/png-icc       |   8 +--
 tests/ref/fate/png-side-data |   2 +-
 3 files changed, 80 insertions(+), 56 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 0d969decf2..c83da08eb9 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -75,6 +75,7 @@ typedef struct PNGDecContext {
     int have_chrm;
     uint32_t white_point[2];
     uint32_t display_primaries[3][2];
+    int gamma;
     int have_srgb;
     int have_cicp;
     enum AVColorPrimaries cicp_primaries;
@@ -1203,7 +1204,7 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
 
             if (avctx->codec_id == AV_CODEC_ID_PNG &&
                 avctx->skip_frame == AVDISCARD_ALL) {
-                return 0;
+                goto exit_loop;
             }
 
             if (CONFIG_APNG_DECODER && avctx->codec_id == AV_CODEC_ID_APNG && length == 0) {
@@ -1256,6 +1257,10 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
             case MKTAG('t', 'E', 'X', 't'):
             case MKTAG('I', 'D', 'A', 'T'):
             case MKTAG('t', 'R', 'N', 'S'):
+            case MKTAG('s', 'R', 'G', 'B'):
+            case MKTAG('c', 'I', 'C', 'P'):
+            case MKTAG('c', 'H', 'R', 'M'):
+            case MKTAG('g', 'A', 'M', 'A'):
                 break;
             default:
                 continue;
@@ -1358,10 +1363,10 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
         case MKTAG('g', 'A', 'M', 'A'): {
             AVBPrint bp;
             char *gamma_str;
-            int num = bytestream2_get_be32(&gb_chunk);
+            s->gamma = bytestream2_get_be32(&gb_chunk);
 
             av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
-            av_bprintf(&bp, "%i/%i", num, 100000);
+            av_bprintf(&bp, "%i/%i", s->gamma, 100000);
             ret = av_bprint_finalize(&bp, &gamma_str);
             if (ret < 0)
                 return ret;
@@ -1382,6 +1387,73 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
     }
 exit_loop:
 
+    if (s->have_cicp) {
+        if (s->cicp_primaries >= AVCOL_PRI_NB)
+            av_log(avctx, AV_LOG_WARNING, "unrecognized cICP primaries\n");
+        else
+            avctx->color_primaries = p->color_primaries = s->cicp_primaries;
+        if (s->cicp_trc >= AVCOL_TRC_NB)
+            av_log(avctx, AV_LOG_WARNING, "unrecognized cICP transfer\n");
+        else
+            avctx->color_trc = p->color_trc = s->cicp_trc;
+        /* we don't support tv-range RGB */
+        avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;
+        if (s->cicp_range == 0)
+            av_log(avctx, AV_LOG_WARNING, "unsupported tv-range cICP chunk\n");
+    } else if (s->iccp_data) {
+        AVFrameSideData *sd = av_frame_new_side_data(p, AV_FRAME_DATA_ICC_PROFILE, s->iccp_data_len);
+        if (!sd) {
+            ret = AVERROR(ENOMEM);
+            goto fail;
+        }
+        memcpy(sd->data, s->iccp_data, s->iccp_data_len);
+
+        av_dict_set(&sd->metadata, "name", s->iccp_name, 0);
+    } else if (s->have_srgb) {
+        avctx->color_primaries = p->color_primaries = AVCOL_PRI_BT709;
+        avctx->color_trc = p->color_trc = AVCOL_TRC_IEC61966_2_1;
+    } else if (s->have_chrm) {
+        AVColorPrimariesDesc desc;
+        enum AVColorPrimaries prim;
+        desc.wp.x = av_make_q(s->white_point[0], 100000);
+        desc.wp.y = av_make_q(s->white_point[1], 100000);
+        desc.prim.r.x = av_make_q(s->display_primaries[0][0], 100000);
+        desc.prim.r.y = av_make_q(s->display_primaries[0][1], 100000);
+        desc.prim.g.x = av_make_q(s->display_primaries[1][0], 100000);
+        desc.prim.g.y = av_make_q(s->display_primaries[1][1], 100000);
+        desc.prim.b.x = av_make_q(s->display_primaries[2][0], 100000);
+        desc.prim.b.y = av_make_q(s->display_primaries[2][1], 100000);
+        prim = av_csp_primaries_id_from_desc(&desc);
+        if (prim != AVCOL_PRI_UNSPECIFIED)
+            avctx->color_primaries = p->color_primaries = prim;
+        else
+            av_log(avctx, AV_LOG_WARNING, "unknown cHRM primaries\n");
+    }
+
+    /* these chunks override gAMA */
+    if (s->iccp_data || s->have_srgb || s->have_cicp) {
+        av_dict_set(&s->frame_metadata, "gamma", NULL, 0);
+    } else if (s->gamma) {
+        /*
+         * these values are 100000/2.2, 100000/2.8, and 100000/2.6
+         * respectively. 45455, 35714, and 38462. There's a 0.001
+         * gamma tolerance here in case of floating point issues
+         * when the PNG was written.
+         *
+         * None of the other enums have a pure gamma curve so it makes
+         * sense to leave those to sRGB and cICP.
+         */
+        if (s->gamma > 45355 && s->gamma < 45555)
+            avctx->color_trc = p->color_trc = AVCOL_TRC_GAMMA22;
+        else if (s->gamma > 35614 && s->gamma < 35814)
+            avctx->color_trc = p->color_trc = AVCOL_TRC_GAMMA28;
+        else if (s->gamma > 38362 && s->gamma < 38562)
+            avctx->color_trc = p->color_trc = AVCOL_TRC_SMPTE428;
+    }
+
+    avctx->colorspace = p->colorspace = AVCOL_SPC_RGB;
+    avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;
+
     if (avctx->codec_id == AV_CODEC_ID_PNG &&
         avctx->skip_frame == AVDISCARD_ALL) {
         return 0;
@@ -1499,56 +1571,8 @@ static void clear_frame_metadata(PNGDecContext *s)
 
 static int output_frame(PNGDecContext *s, AVFrame *f)
 {
-    AVCodecContext *avctx = s->avctx;
     int ret;
 
-    if (s->have_cicp) {
-        if (s->cicp_primaries >= AVCOL_PRI_NB)
-            av_log(avctx, AV_LOG_WARNING, "unrecognized cICP primaries\n");
-        else
-            avctx->color_primaries = f->color_primaries = s->cicp_primaries;
-        if (s->cicp_trc >= AVCOL_TRC_NB)
-            av_log(avctx, AV_LOG_WARNING, "unrecognized cICP transfer\n");
-        else
-            avctx->color_trc = f->color_trc = s->cicp_trc;
-        avctx->color_range = f->color_range =
-            s->cicp_range == 0 ? AVCOL_RANGE_MPEG : AVCOL_RANGE_JPEG;
-    } else if (s->iccp_data) {
-        AVFrameSideData *sd = av_frame_new_side_data(f, AV_FRAME_DATA_ICC_PROFILE, s->iccp_data_len);
-        if (!sd) {
-            ret = AVERROR(ENOMEM);
-            goto fail;
-        }
-        memcpy(sd->data, s->iccp_data, s->iccp_data_len);
-
-        av_dict_set(&sd->metadata, "name", s->iccp_name, 0);
-    } else if (s->have_srgb) {
-        avctx->color_primaries = f->color_primaries = AVCOL_PRI_BT709;
-        avctx->color_trc = f->color_trc = AVCOL_TRC_IEC61966_2_1;
-    } else if (s->have_chrm) {
-        AVColorPrimariesDesc desc;
-        enum AVColorPrimaries prim;
-        desc.wp.x = av_make_q(s->white_point[0], 100000);
-        desc.wp.y = av_make_q(s->white_point[1], 100000);
-        desc.prim.r.x = av_make_q(s->display_primaries[0][0], 100000);
-        desc.prim.r.y = av_make_q(s->display_primaries[0][1], 100000);
-        desc.prim.g.x = av_make_q(s->display_primaries[1][0], 100000);
-        desc.prim.g.y = av_make_q(s->display_primaries[1][1], 100000);
-        desc.prim.b.x = av_make_q(s->display_primaries[2][0], 100000);
-        desc.prim.b.y = av_make_q(s->display_primaries[2][1], 100000);
-        prim = av_csp_primaries_id_from_desc(&desc);
-        if (prim != AVCOL_PRI_UNSPECIFIED)
-            avctx->color_primaries = f->color_primaries = prim;
-        else
-            av_log(avctx, AV_LOG_WARNING, "unknown cHRM primaries\n");
-    }
-
-    /* these chunks override gAMA */
-    if (s->iccp_data || s->have_srgb || s->have_cicp)
-        av_dict_set(&s->frame_metadata, "gamma", NULL, 0);
-
-    avctx->colorspace = f->colorspace = AVCOL_SPC_RGB;
-
     if (s->stereo_mode >= 0) {
         AVStereo3D *stereo3d = av_stereo3d_create_side_data(f);
         if (!stereo3d) {
diff --git a/tests/ref/fate/png-icc b/tests/ref/fate/png-icc
index 14de544639..1f4eab1fb0 100644
--- a/tests/ref/fate/png-icc
+++ b/tests/ref/fate/png-icc
@@ -1,5 +1,5 @@
-a50d37a0e72bddea2fcbba6fb773e2a0 *tests/data/fate/png-icc.image2
-49397 tests/data/fate/png-icc.image2
+c460cd06a88ace94d7c76a6309aa8fb8 *tests/data/fate/png-icc.image2
+49441 tests/data/fate/png-icc.image2
 #tb 0: 1/25
 #media_type 0: video
 #codec_id 0: rawvideo
@@ -21,7 +21,7 @@ pkt_duration_time=0.040000
 duration=1
 duration_time=0.040000
 pkt_pos=0
-pkt_size=49397
+pkt_size=49441
 width=128
 height=128
 pix_fmt=rgb24
@@ -34,7 +34,7 @@ top_field_first=0
 repeat_pict=0
 color_range=pc
 color_space=gbr
-color_primaries=unknown
+color_primaries=bt709
 color_transfer=unknown
 chroma_location=unspecified
 [SIDE_DATA]
diff --git a/tests/ref/fate/png-side-data b/tests/ref/fate/png-side-data
index a4c16eb395..8f955f76bc 100644
--- a/tests/ref/fate/png-side-data
+++ b/tests/ref/fate/png-side-data
@@ -26,7 +26,7 @@ top_field_first=0
 repeat_pict=0
 color_range=pc
 color_space=gbr
-color_primaries=unknown
+color_primaries=bt709
 color_transfer=unknown
 chroma_location=unspecified
 [SIDE_DATA]
-- 
2.39.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 2/2] avcodec/pngdec: read colorspace info when decoding with AVDISCARD_ALL

[Andreas Rheinhardt]

Leo Izen:
> These chunks are lightweight and it's useful information to have when
> running ffmpeg -i or ffprobe, for example.
> 
> Signed-off-by: Leo Izen <leo.izen@gmail.com>
> ---
>  libavcodec/pngdec.c          | 126 +++++++++++++++++++++--------------
>  tests/ref/fate/png-icc       |   8 +--
>  tests/ref/fate/png-side-data |   2 +-
>  3 files changed, 80 insertions(+), 56 deletions(-)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 0d969decf2..c83da08eb9 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -75,6 +75,7 @@ typedef struct PNGDecContext {
>      int have_chrm;
>      uint32_t white_point[2];
>      uint32_t display_primaries[3][2];
> +    int gamma;
>      int have_srgb;
>      int have_cicp;
>      enum AVColorPrimaries cicp_primaries;
> @@ -1203,7 +1204,7 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
>  
>              if (avctx->codec_id == AV_CODEC_ID_PNG &&
>                  avctx->skip_frame == AVDISCARD_ALL) {
> -                return 0;
> +                goto exit_loop;
>              }
>  
>              if (CONFIG_APNG_DECODER && avctx->codec_id == AV_CODEC_ID_APNG && length == 0) {
> @@ -1256,6 +1257,10 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
>              case MKTAG('t', 'E', 'X', 't'):
>              case MKTAG('I', 'D', 'A', 'T'):
>              case MKTAG('t', 'R', 'N', 'S'):
> +            case MKTAG('s', 'R', 'G', 'B'):
> +            case MKTAG('c', 'I', 'C', 'P'):
> +            case MKTAG('c', 'H', 'R', 'M'):
> +            case MKTAG('g', 'A', 'M', 'A'):
>                  break;
>              default:
>                  continue;
> @@ -1358,10 +1363,10 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
>          case MKTAG('g', 'A', 'M', 'A'): {
>              AVBPrint bp;
>              char *gamma_str;
> -            int num = bytestream2_get_be32(&gb_chunk);
> +            s->gamma = bytestream2_get_be32(&gb_chunk);
>  
>              av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
> -            av_bprintf(&bp, "%i/%i", num, 100000);
> +            av_bprintf(&bp, "%i/%i", s->gamma, 100000);
>              ret = av_bprint_finalize(&bp, &gamma_str);
>              if (ret < 0)
>                  return ret;
> @@ -1382,6 +1387,73 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
>      }
>  exit_loop:
>  
> +    if (s->have_cicp) {
> +        if (s->cicp_primaries >= AVCOL_PRI_NB)
> +            av_log(avctx, AV_LOG_WARNING, "unrecognized cICP primaries\n");
> +        else
> +            avctx->color_primaries = p->color_primaries = s->cicp_primaries;
> +        if (s->cicp_trc >= AVCOL_TRC_NB)
> +            av_log(avctx, AV_LOG_WARNING, "unrecognized cICP transfer\n");
> +        else
> +            avctx->color_trc = p->color_trc = s->cicp_trc;
> +        /* we don't support tv-range RGB */
> +        avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;
> +        if (s->cicp_range == 0)
> +            av_log(avctx, AV_LOG_WARNING, "unsupported tv-range cICP chunk\n");
> +    } else if (s->iccp_data) {
> +        AVFrameSideData *sd = av_frame_new_side_data(p, AV_FRAME_DATA_ICC_PROFILE, s->iccp_data_len);
> +        if (!sd) {
> +            ret = AVERROR(ENOMEM);
> +            goto fail;
> +        }
> +        memcpy(sd->data, s->iccp_data, s->iccp_data_len);
> +
> +        av_dict_set(&sd->metadata, "name", s->iccp_name, 0);
> +    } else if (s->have_srgb) {
> +        avctx->color_primaries = p->color_primaries = AVCOL_PRI_BT709;
> +        avctx->color_trc = p->color_trc = AVCOL_TRC_IEC61966_2_1;
> +    } else if (s->have_chrm) {
> +        AVColorPrimariesDesc desc;
> +        enum AVColorPrimaries prim;
> +        desc.wp.x = av_make_q(s->white_point[0], 100000);
> +        desc.wp.y = av_make_q(s->white_point[1], 100000);
> +        desc.prim.r.x = av_make_q(s->display_primaries[0][0], 100000);
> +        desc.prim.r.y = av_make_q(s->display_primaries[0][1], 100000);
> +        desc.prim.g.x = av_make_q(s->display_primaries[1][0], 100000);
> +        desc.prim.g.y = av_make_q(s->display_primaries[1][1], 100000);
> +        desc.prim.b.x = av_make_q(s->display_primaries[2][0], 100000);
> +        desc.prim.b.y = av_make_q(s->display_primaries[2][1], 100000);
> +        prim = av_csp_primaries_id_from_desc(&desc);
> +        if (prim != AVCOL_PRI_UNSPECIFIED)
> +            avctx->color_primaries = p->color_primaries = prim;
> +        else
> +            av_log(avctx, AV_LOG_WARNING, "unknown cHRM primaries\n");
> +    }
> +
> +    /* these chunks override gAMA */
> +    if (s->iccp_data || s->have_srgb || s->have_cicp) {
> +        av_dict_set(&s->frame_metadata, "gamma", NULL, 0);
> +    } else if (s->gamma) {
> +        /*
> +         * these values are 100000/2.2, 100000/2.8, and 100000/2.6
> +         * respectively. 45455, 35714, and 38462. There's a 0.001
> +         * gamma tolerance here in case of floating point issues
> +         * when the PNG was written.
> +         *
> +         * None of the other enums have a pure gamma curve so it makes
> +         * sense to leave those to sRGB and cICP.
> +         */
> +        if (s->gamma > 45355 && s->gamma < 45555)
> +            avctx->color_trc = p->color_trc = AVCOL_TRC_GAMMA22;
> +        else if (s->gamma > 35614 && s->gamma < 35814)
> +            avctx->color_trc = p->color_trc = AVCOL_TRC_GAMMA28;
> +        else if (s->gamma > 38362 && s->gamma < 38562)
> +            avctx->color_trc = p->color_trc = AVCOL_TRC_SMPTE428;

It seems that this patch does more than the commit message claims.

> +    }
> +
> +    avctx->colorspace = p->colorspace = AVCOL_SPC_RGB;
> +    avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;
> +
>      if (avctx->codec_id == AV_CODEC_ID_PNG &&
>          avctx->skip_frame == AVDISCARD_ALL) {
>          return 0;
> @@ -1499,56 +1571,8 @@ static void clear_frame_metadata(PNGDecContext *s)
>  
>  static int output_frame(PNGDecContext *s, AVFrame *f)
>  {
> -    AVCodecContext *avctx = s->avctx;
>      int ret;
>  
> -    if (s->have_cicp) {
> -        if (s->cicp_primaries >= AVCOL_PRI_NB)
> -            av_log(avctx, AV_LOG_WARNING, "unrecognized cICP primaries\n");
> -        else
> -            avctx->color_primaries = f->color_primaries = s->cicp_primaries;
> -        if (s->cicp_trc >= AVCOL_TRC_NB)
> -            av_log(avctx, AV_LOG_WARNING, "unrecognized cICP transfer\n");
> -        else
> -            avctx->color_trc = f->color_trc = s->cicp_trc;
> -        avctx->color_range = f->color_range =
> -            s->cicp_range == 0 ? AVCOL_RANGE_MPEG : AVCOL_RANGE_JPEG;
> -    } else if (s->iccp_data) {
> -        AVFrameSideData *sd = av_frame_new_side_data(f, AV_FRAME_DATA_ICC_PROFILE, s->iccp_data_len);
> -        if (!sd) {
> -            ret = AVERROR(ENOMEM);
> -            goto fail;
> -        }
> -        memcpy(sd->data, s->iccp_data, s->iccp_data_len);
> -
> -        av_dict_set(&sd->metadata, "name", s->iccp_name, 0);
> -    } else if (s->have_srgb) {
> -        avctx->color_primaries = f->color_primaries = AVCOL_PRI_BT709;
> -        avctx->color_trc = f->color_trc = AVCOL_TRC_IEC61966_2_1;
> -    } else if (s->have_chrm) {
> -        AVColorPrimariesDesc desc;
> -        enum AVColorPrimaries prim;
> -        desc.wp.x = av_make_q(s->white_point[0], 100000);
> -        desc.wp.y = av_make_q(s->white_point[1], 100000);
> -        desc.prim.r.x = av_make_q(s->display_primaries[0][0], 100000);
> -        desc.prim.r.y = av_make_q(s->display_primaries[0][1], 100000);
> -        desc.prim.g.x = av_make_q(s->display_primaries[1][0], 100000);
> -        desc.prim.g.y = av_make_q(s->display_primaries[1][1], 100000);
> -        desc.prim.b.x = av_make_q(s->display_primaries[2][0], 100000);
> -        desc.prim.b.y = av_make_q(s->display_primaries[2][1], 100000);
> -        prim = av_csp_primaries_id_from_desc(&desc);
> -        if (prim != AVCOL_PRI_UNSPECIFIED)
> -            avctx->color_primaries = f->color_primaries = prim;
> -        else
> -            av_log(avctx, AV_LOG_WARNING, "unknown cHRM primaries\n");
> -    }
> -
> -    /* these chunks override gAMA */
> -    if (s->iccp_data || s->have_srgb || s->have_cicp)
> -        av_dict_set(&s->frame_metadata, "gamma", NULL, 0);
> -
> -    avctx->colorspace = f->colorspace = AVCOL_SPC_RGB;
> -
>      if (s->stereo_mode >= 0) {
>          AVStereo3D *stereo3d = av_stereo3d_create_side_data(f);
>          if (!stereo3d) {
> diff --git a/tests/ref/fate/png-icc b/tests/ref/fate/png-icc
> index 14de544639..1f4eab1fb0 100644
> --- a/tests/ref/fate/png-icc
> +++ b/tests/ref/fate/png-icc
> @@ -1,5 +1,5 @@
> -a50d37a0e72bddea2fcbba6fb773e2a0 *tests/data/fate/png-icc.image2
> -49397 tests/data/fate/png-icc.image2
> +c460cd06a88ace94d7c76a6309aa8fb8 *tests/data/fate/png-icc.image2
> +49441 tests/data/fate/png-icc.image2
>  #tb 0: 1/25
>  #media_type 0: video
>  #codec_id 0: rawvideo
> @@ -21,7 +21,7 @@ pkt_duration_time=0.040000
>  duration=1
>  duration_time=0.040000
>  pkt_pos=0
> -pkt_size=49397
> +pkt_size=49441
>  width=128
>  height=128
>  pix_fmt=rgb24
> @@ -34,7 +34,7 @@ top_field_first=0
>  repeat_pict=0
>  color_range=pc
>  color_space=gbr
> -color_primaries=unknown
> +color_primaries=bt709
>  color_transfer=unknown
>  chroma_location=unspecified
>  [SIDE_DATA]
> diff --git a/tests/ref/fate/png-side-data b/tests/ref/fate/png-side-data
> index a4c16eb395..8f955f76bc 100644
> --- a/tests/ref/fate/png-side-data
> +++ b/tests/ref/fate/png-side-data
> @@ -26,7 +26,7 @@ top_field_first=0
>  repeat_pict=0
>  color_range=pc
>  color_space=gbr
> -color_primaries=unknown
> +color_primaries=bt709
>  color_transfer=unknown
>  chroma_location=unspecified
>  [SIDE_DATA]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 2/2] avcodec/pngdec: read colorspace info when decoding with AVDISCARD_ALL

[Leo Izen]

On 2/1/23 14:32, Andreas Rheinhardt wrote:
> Leo Izen:
>> These chunks are lightweight and it's useful information to have when
>> running ffmpeg -i or ffprobe, for example.
>>
>> +    } else if (s->gamma) {
>> +        /*
>> +         * these values are 100000/2.2, 100000/2.8, and 100000/2.6
>> +         * respectively. 45455, 35714, and 38462. There's a 0.001
>> +         * gamma tolerance here in case of floating point issues
>> +         * when the PNG was written.
>> +         *
>> +         * None of the other enums have a pure gamma curve so it makes
>> +         * sense to leave those to sRGB and cICP.
>> +         */
>> +        if (s->gamma > 45355 && s->gamma < 45555)
>> +            avctx->color_trc = p->color_trc = AVCOL_TRC_GAMMA22;
>> +        else if (s->gamma > 35614 && s->gamma < 35814)
>> +            avctx->color_trc = p->color_trc = AVCOL_TRC_GAMMA28;
>> +        else if (s->gamma > 38362 && s->gamma < 38562)
>> +            avctx->color_trc = p->color_trc = AVCOL_TRC_SMPTE428;
> 
> It seems that this patch does more than the commit message claims.
> 

Would it be ideal to split this off into a third patch?

- Leo Izen (thebombzen)

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".