Message ID | 20230202150938.22297-3-leo.izen@gmail.com |
---|---|
State | New |
Headers | show |
Series | PNG color tagging improvements | expand |
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 |
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]
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)
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]