[FFmpeg-devel] [PATCH] lavf/mxfenc: Use nb_components, not av_pix_fmt_count_planes()

[Marton Balint]

On Tue, 29 Oct 2024, Tomas Härdin wrote:

> tis 2024-10-29 klockan 12:21 -0300 skrev James Almer:
>>> From ce4b1dfb97530b242f899e5d1686f98fa83a7698 Mon Sep 17 00:00:00
>>> 2001
>>> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git at haerdin.se>
>>> Date: Tue, 29 Oct 2024 16:13:04 +0100
>>> Subject: [PATCH] lavf/mxfenc: Use nb_components, not
>>> av_pix_fmt_count_planes()
>>>
>>> This fixed https://trac.ffmpeg.org/ticket/11267
>>> ---
>>>  libavformat/mxfenc.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>>> index 57be9e6ef6..e66df0fba2 100644
>>> --- a/libavformat/mxfenc.c
>>> +++ b/libavformat/mxfenc.c
>>> @@ -1488,7 +1488,7 @@ static void
>>> mxf_write_jpeg2000_subdesc(AVFormatContext *s, AVStream *st)
>>>      MXFStreamContext *sc = st->priv_data;
>>>      AVIOContext *pb = s->pb;
>>>      int64_t pos;
>>> -    int component_count = av_pix_fmt_count_planes(st->codecpar-
>>>> format);
>>> +    int component_count = av_pix_fmt_desc_get(st->codecpar-
>>>> format)->nb_components;
>>
>> I don't think anything ensures that av_pix_fmt_desc_get() here will
>> not
>> return NULL, so maybe instead do:
>>
>>> const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(st->codecpar-
>>>> format);
>>> int component_count;
>>>
>>> if (!desc)
>>>     return AVERROR(EINVAL);
>>>
>>> component_count = desc->nb_components;
>
> I can't really see how that would happen, but I suppose it doesn't
> hurt.
>
> I see elsewhere in the code an assert that the returned pointer is not
> NULL (mxf_write_ffv1_desc()), and explicit checks for it

There is a similar check in your patch:

-    int component_count = av_pix_fmt_count_planes(st->codecpar->format);
+    const AVPixFmtDescriptor *pix_desc = av_pix_fmt_desc_get(st->codecpar->format);
+
+    if (!pix_desc) {
+        av_log(s, AV_LOG_ERROR, "Pixel format not set - not writing JPEG2000SubDescriptor\n");
+        return;
+    }

You should propagate back the error, not just silently ignore it. Or if 
this cannot happen, make this an assert instead.

Thanks,
Marton