[FFmpeg-devel] [PATCH v3 0/3] PNG color tagging improvements

[FFmpeg-devel] [PATCH v3 0/3] PNG color tagging 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 v2:
- Split off the gAMA chunk handling into a second patch at Andreas' suggestion.

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 (3):
  avcodec/pngenc: avoid writing cICP when inappropriate
  avcodec/pngdec: read colorspace info when decoding with AVDISCARD_ALL
  avcodec/pngdec: populate enum transfer values from gAMA chunk

 libavcodec/pngdec.c          | 128 +++++++++++++++++++++--------------
 libavcodec/pngenc.c          |  21 ++++--
 tests/ref/fate/png-icc       |   8 +--
 tests/ref/fate/png-side-data |   2 +-
 4 files changed, 96 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 v3 1/3] avcodec/pngenc: 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".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/pngenc: avoid writing cICP when inappropriate

[Leo Izen]

On 2/2/23 10:09, Leo Izen wrote:
> 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.
> 

Andreas has an issue with one of the other patches, but as this first 
one is just a fix I'll apply it soon if there's no objections.

- 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".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/pngenc: avoid writing cICP when inappropriate

[Leo Izen]

On 2/7/23 14:17, Leo Izen wrote:
> On 2/2/23 10:09, Leo Izen wrote:
>> 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.
>>
> 
> Andreas has an issue with one of the other patches, but as this first 
> one is just a fix I'll apply it soon if there's no objections.
> 
> - Leo Izen (thebombzen)

Pushed just this one patch. The other two are still open for review.

- 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".

[FFmpeg-devel] [PATCH v3 2/3] 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.
---
 libavcodec/pngdec.c          | 103 ++++++++++++++++++-----------------
 tests/ref/fate/png-icc       |   8 +--
 tests/ref/fate/png-side-data |   2 +-
 3 files changed, 59 insertions(+), 54 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 0d969decf2..a80e0d15bb 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -1203,7 +1203,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 +1256,9 @@ 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'):
                 break;
             default:
                 continue;
@@ -1382,6 +1385,56 @@ 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);
+
+    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 +1552,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 v3 2/3] 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.
> ---
>  libavcodec/pngdec.c          | 103 ++++++++++++++++++-----------------
>  tests/ref/fate/png-icc       |   8 +--
>  tests/ref/fate/png-side-data |   2 +-
>  3 files changed, 59 insertions(+), 54 deletions(-)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 0d969decf2..a80e0d15bb 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -1203,7 +1203,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 +1256,9 @@ 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'):
>                  break;
>              default:
>                  continue;
> @@ -1382,6 +1385,56 @@ 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;

This code (both the new code as well as the old code in output_frame()
added in 6f79f0971e8 and f7bab37c8e6 and 2548c32cc10) modify avctx
fields after ff_thread_finish_setup() and are racy with respect to the
generic copying of AVCodecContext fields in update_context_from_thread()
in pthread_frame.c.

> +        /* we don't support tv-range RGB */
> +        avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;

You set this unconditionally below, too.

> +        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);
> +
> +    avctx->colorspace = p->colorspace = AVCOL_SPC_RGB;
> +    avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;

This is new in this patch; it changes behaviour even when nothing is
discarded. Is this intended?

> +
>      if (avctx->codec_id == AV_CODEC_ID_PNG &&
>          avctx->skip_frame == AVDISCARD_ALL) {
>          return 0;
> @@ -1499,56 +1552,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 v3 2/3] avcodec/pngdec: read colorspace info when decoding with AVDISCARD_ALL

[Leo Izen]

On 2/4/23 22:47, Andreas Rheinhardt wrote:
> Leo Izen:
>> These chunks are lightweight and it's useful information to have when
>> running ffmpeg -i or ffprobe, for example.
>> ---
>>   libavcodec/pngdec.c          | 103 ++++++++++++++++++-----------------
>>   tests/ref/fate/png-icc       |   8 +--
>>   tests/ref/fate/png-side-data |   2 +-
>>   3 files changed, 59 insertions(+), 54 deletions(-)
>>
>> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
>> index 0d969decf2..a80e0d15bb 100644
>> --- a/libavcodec/pngdec.c
>> +++ b/libavcodec/pngdec.c
>> @@ -1203,7 +1203,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 +1256,9 @@ 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'):
>>                   break;
>>               default:
>>                   continue;
>> @@ -1382,6 +1385,56 @@ 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;
> 
> This code (both the new code as well as the old code in output_frame()
> added in 6f79f0971e8 and f7bab37c8e6 and 2548c32cc10) modify avctx
> fields after ff_thread_finish_setup() and are racy with respect to the
> generic copying of AVCodecContext fields in update_context_from_thread()
> in pthread_frame.c.
> 

Where should I update these fields instead to prevent races? I'm not 
sure where else to set it.


>> +        /* we don't support tv-range RGB */
>> +        avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;
> 
> You set this unconditionally below, too.

Shouldn't be set twice, good catch. Can remove this one.

> 
>> +        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);
>> +
>> +    avctx->colorspace = p->colorspace = AVCOL_SPC_RGB;
>> +    avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;
> 
> This is new in this patch; it changes behaviour even when nothing is
> discarded. Is this intended?

Yes. These are the only values supported by our PNG decoder so it makes 
sense to set them unconditionally.

The color range is already set in the init function, but it's also set 
here for clarity. I could possibly move the unconditional colorspace 
setting to the init function as well and remove both of these lines.

- 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".

[FFmpeg-devel] [PATCH v3 3/3] avcodec/pngdec: populate enum transfer values from gAMA chunk

[Leo Izen]

If the gAMA chunk is present, the gamma-transfer described by it often
matches one of our enum values (e.g. gamma = 2.2). If so, we should
populate the corresponding fields in AVCodecContext and AVFrame,
provided that some other chunk isn't already providing that info.
---
 libavcodec/pngdec.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index a80e0d15bb..b7751c64d8 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;
@@ -1259,6 +1260,7 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *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;
@@ -1361,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;
@@ -1429,8 +1431,27 @@ exit_loop:
     }
 
     /* these chunks override gAMA */
-    if (s->iccp_data || s->have_srgb || s->have_cicp)
+    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, 100000/2.6, and
+         * 100000/1.0 respectively. 45455, 35714, and 38462, and 100000.
+         * 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;
+        else if (s->gamma > 99900 && s->gamma < 100100)
+            avctx->color_trc = p->color_trc = AVCOL_TRC_LINEAR;
+    }
 
     avctx->colorspace = p->colorspace = AVCOL_SPC_RGB;
     avctx->color_range = p->color_range = AVCOL_RANGE_JPEG;
-- 
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".