diff mbox series

[FFmpeg-devel,v4] avcodec/pngdec: read colorspace info when decoding with AVDISCARD_ALL

Message ID 20230221223555.102670-1-leo.izen@gmail.com
State Accepted
Commit fadfa147f812a3fe9e68723347d37b9cccd6222d
Headers show
Series [FFmpeg-devel,v4] avcodec/pngdec: read colorspace info when decoding with AVDISCARD_ALL | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Leo Izen Feb. 21, 2023, 10:35 p.m. UTC 
These chunks are lightweight and it's useful information to have when
running ffmpeg -i or ffprobe, for example.
---
 libavcodec/pngdec.c          | 136 ++++++++++++++++++++++-------------
 tests/ref/fate/png-icc       |   8 +--
 tests/ref/fate/png-side-data |   2 +-
 3 files changed, 91 insertions(+), 55 deletions(-)

Comments

Ronald S. Bultje Feb. 22, 2023, 1:42 a.m. UTC | #1 
Hi,

On Tue, Feb 21, 2023 at 5:36 PM Leo Izen <leo.izen@gmail.com> wrote:

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

I disagree conceptually with this. If we want to skip "all" decoding, we
should also skip metadata for said frames.

I think what we're looking for here is a partial decoding stage, e.g.
"header-only" or similar, instead of "including framedata", for some or all
frames. That's orthogonal to AVDISCARD_*.

Ronald
Leo Izen Feb. 22, 2023, 1:49 p.m. UTC | #2 
On 2/21/23 20:42, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Feb 21, 2023 at 5:36 PM Leo Izen <leo.izen@gmail.com 
> <mailto:leo.izen@gmail.com>> wrote:
> 
>     These chunks are lightweight and it's useful information to have when
>     running ffmpeg -i or ffprobe, for example.
> 
> 
> I disagree conceptually with this. If we want to skip "all" decoding, we 
> should also skip metadata for said frames.
> 
> I think what we're looking for here is a partial decoding stage, e.g. 
> "header-only" or similar, instead of "including framedata", for some or 
> all frames. That's orthogonal to AVDISCARD_*.
> 
> Ronald

The issue is that libavformat and ffmpeg.c obtain metadata by calling 
the decoder with AVDISCARD_ALL. What you're proposing would require a 
structural change elsewhere, and I'm not sure it makes sense to block a 
patch like this one based on a hypothetical change elsewhere.

- Leo Izen (thebombzen)
Ronald S. Bultje Feb. 22, 2023, 4:32 p.m. UTC | #3 
On Wed, Feb 22, 2023 at 8:49 AM Leo Izen <leo.izen@gmail.com> wrote:

> On 2/21/23 20:42, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Tue, Feb 21, 2023 at 5:36 PM Leo Izen <leo.izen@gmail.com
> > <mailto:leo.izen@gmail.com>> wrote:
> >
> >     These chunks are lightweight and it's useful information to have when
> >     running ffmpeg -i or ffprobe, for example.
> >
> >
> > I disagree conceptually with this. If we want to skip "all" decoding, we
> > should also skip metadata for said frames.
> >
> > I think what we're looking for here is a partial decoding stage, e.g.
> > "header-only" or similar, instead of "including framedata", for some or
> > all frames. That's orthogonal to AVDISCARD_*.
> >
> > Ronald
>
> The issue is that libavformat and ffmpeg.c obtain metadata by calling
> the decoder with AVDISCARD_ALL. What you're proposing would require a
> structural change elsewhere, and I'm not sure it makes sense to block a
> patch like this one based on a hypothetical change elsewhere.


That’s fair. I’ll put this in the long term wishlist bucket.

Ronald
Leo Izen Feb. 27, 2023, 4:34 p.m. UTC | #4 
On 2/21/23 17:35, Leo Izen wrote:
> These chunks are lightweight and it's useful information to have when
> running ffmpeg -i or ffprobe, for example.
> ---
>   libavcodec/pngdec.c          | 136 ++++++++++++++++++++++-------------
>   tests/ref/fate/png-icc       |   8 +--
>   tests/ref/fate/png-side-data |   2 +-
>   3 files changed, 91 insertions(+), 55 deletions(-)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c

I will push this soon-ish if there's no other objections.

- Leo Izen (thebombzen)
Leo Izen Feb. 28, 2023, 7:46 p.m. UTC | #5 
On 2/27/23 11:34, Leo Izen wrote:
> On 2/21/23 17:35, Leo Izen wrote:
>> These chunks are lightweight and it's useful information to have when
>> running ffmpeg -i or ffprobe, for example.
>> ---
>>   libavcodec/pngdec.c          | 136 ++++++++++++++++++++++-------------
>>   tests/ref/fate/png-icc       |   8 +--
>>   tests/ref/fate/png-side-data |   2 +-
>>   3 files changed, 91 insertions(+), 55 deletions(-)
>>
>> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> 
> I will push this soon-ish if there's no other objections.
> 
> - Leo Izen (thebombzen)
> 
> 

Pushed as fadfa147f812a3fe9e68723347d37b9cccd6222d.

- Leo Izen (thebombzen)
Kacper Michajłow Dec. 11, 2023, 4:20 a.m. UTC | #6 
On Tue, 28 Feb 2023 at 20:46, Leo Izen <leo.izen@gmail.com> wrote:
>
> On 2/27/23 11:34, Leo Izen wrote:
> > On 2/21/23 17:35, Leo Izen wrote:
> >> These chunks are lightweight and it's useful information to have when
> >> running ffmpeg -i or ffprobe, for example.
> >> ---
> >>   libavcodec/pngdec.c          | 136 ++++++++++++++++++++++-------------
> >>   tests/ref/fate/png-icc       |   8 +--
> >>   tests/ref/fate/png-side-data |   2 +-
> >>   3 files changed, 91 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> >
> > I will push this soon-ish if there's no other objections.
> >
> > - Leo Izen (thebombzen)
> >
> >
>
> Pushed as fadfa147f812a3fe9e68723347d37b9cccd6222d.

Hi, Sorry for digging this old patch, but I'm curious.

What does "unsupported tv-range cICP chunk\n" and "we only support
pc-range RGB" mean?

Before this patch the range was properly set:
avctx->color_range = f->color_range =
            s->cicp_range == 0 ? AVCOL_RANGE_MPEG : AVCOL_RANGE_JPEG;

Now it is ignored with a warning. Neither commit message, nor the
comments in the code explain why the cICP range is now ignored in the
decoder and the frames are no longer tagged with proper color range.

As I understand the goal of the commit was to expose colorspace info
when AVDISCARD_ALL. Was cICP range change intentional?

Thanks,
Kacper
Leo Izen Dec. 11, 2023, 11:54 a.m. UTC | #7 
On 12/10/23 23:20, Kacper Michajlow wrote:
> On Tue, 28 Feb 2023 at 20:46, Leo Izen <leo.izen@gmail.com> wrote:
>>
>> On 2/27/23 11:34, Leo Izen wrote:
>>> On 2/21/23 17:35, Leo Izen wrote:
>>>> These chunks are lightweight and it's useful information to have when
>>>> running ffmpeg -i or ffprobe, for example.
>>>> ---
>>>>    libavcodec/pngdec.c          | 136 ++++++++++++++++++++++-------------
>>>>    tests/ref/fate/png-icc       |   8 +--
>>>>    tests/ref/fate/png-side-data |   2 +-
>>>>    3 files changed, 91 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
>>>
>>> I will push this soon-ish if there's no other objections.
>>>
>>> - Leo Izen (thebombzen)
>>>
>>>
>>
>> Pushed as fadfa147f812a3fe9e68723347d37b9cccd6222d.
> 
> Hi, Sorry for digging this old patch, but I'm curious.
> 
> What does "unsupported tv-range cICP chunk\n" and "we only support
> pc-range RGB" mean?
> 
> Before this patch the range was properly set:
> avctx->color_range = f->color_range =
>              s->cicp_range == 0 ? AVCOL_RANGE_MPEG : AVCOL_RANGE_JPEG;
> 
> Now it is ignored with a warning. Neither commit message, nor the
> comments in the code explain why the cICP range is now ignored in the
> decoder and the frames are no longer tagged with proper color range.
> 

It means FFmpeg itself doesn't support tv-range RGB. The PNG format 
supports it but FFmpeg does not. Since the PNG spec specifies the matrix 
must equal zero (aka RGB), we can't support tv-range here and print a 
warning.

- Leo Izen (Traneptora)
diff mbox series

Patch

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 0d969decf2..d1aa29978a 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;
@@ -641,6 +642,83 @@  static int decode_phys_chunk(AVCodecContext *avctx, PNGDecContext *s,
     return 0;
 }
 
+/*
+ * This populates AVCodecContext fields so it must be called before
+ * ff_thread_finish_setup() to avoid a race condition with respect to the
+ * generic copying of avctx fields.
+ */
+static int populate_avctx_color_fields(AVCodecContext *avctx, AVFrame *frame)
+{
+    PNGDecContext *s = avctx->priv_data;
+
+    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 = frame->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 = frame->color_trc = s->cicp_trc;
+        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(frame, AV_FRAME_DATA_ICC_PROFILE, s->iccp_data_len);
+        if (!sd)
+            return AVERROR(ENOMEM);
+        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 = frame->color_primaries = AVCOL_PRI_BT709;
+        avctx->color_trc = frame->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 = frame->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, 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 = frame->color_trc = AVCOL_TRC_GAMMA22;
+        else if (s->gamma > 35614 && s->gamma < 35814)
+            avctx->color_trc = frame->color_trc = AVCOL_TRC_GAMMA28;
+        else if (s->gamma > 38362 && s->gamma < 38562)
+            avctx->color_trc = frame->color_trc = AVCOL_TRC_SMPTE428;
+        else if (s->gamma > 99900 && s->gamma < 100100)
+            avctx->color_trc = frame->color_trc = AVCOL_TRC_LINEAR;
+    }
+
+    /* we only support pc-range RGB */
+    avctx->colorspace = frame->colorspace = AVCOL_SPC_RGB;
+    avctx->color_range = frame->color_range = AVCOL_RANGE_JPEG;
+
+    return 0;
+}
+
 static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
                              GetByteContext *gb, AVFrame *p)
 {
@@ -756,6 +834,8 @@  static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
         p->key_frame        = 1;
         p->interlaced_frame = !!s->interlace_type;
 
+        if ((ret = populate_avctx_color_fields(avctx, p)) < 0)
+            return ret;
         ff_thread_finish_setup(avctx);
 
         /* compute the compressed row size */
@@ -1256,6 +1336,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 +1442,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;
@@ -1499,56 +1583,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]