[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)